Fix: readUInt8Array must copy, not view the reused scan buffer#5491
Open
DexterKoelson wants to merge 1 commit into
Open
Fix: readUInt8Array must copy, not view the reused scan buffer#5491DexterKoelson wants to merge 1 commit into
DexterKoelson wants to merge 1 commit into
Conversation
- `readUInt8Array` now returns an owned copy (`readBytes(length).slice()`), so decoded column values have a lifetime independent of the reader's buffer reuse. - `readString` reads its bytes via `readBytes` directly. `TextDecoder` copies synchronously and keeps no reference to the buffer, so strings stay zero-copy and don't pay the new copy. - `readBytes` is unchanged and still returns a view, with a comment noting the view is only valid until the buffer is reused.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Fixes #5490.
Reading an
array<u8>column returned aUint8Arraythat was a view over the row iterator's scan buffer instead of an owned copy. That buffer comes from a shared pool and gets reused by the next table scan, so au8-array value that outlived the scan it was read from would silently start reflecting some later scan's bytes. No error, just wrong data. Every other column type decodes to an owned value, so only thearray<u8>fast path had this.Changes in
BinaryReader:readUInt8Arraynow returns an owned copy (readBytes(length).slice()), so decoded column values have a lifetime independent of the reader's buffer reuse.readStringreads its bytes viareadBytesdirectly.TextDecodercopies synchronously and keeps no reference to the buffer, so strings stay zero-copy and don't pay the new copy.readBytesis unchanged and still returns a view, with a comment noting the view is only valid until the buffer is reused.API and ABI breaking changes
None. The return type of
readUInt8Arrayis stillUint8Array; callers just get a copy they own instead of a view that could be invalidated under them.Expected complexity level and risk
u8-array column read, which is whyreadStringwas routed around it to avoid a double copy on the common string path.Testing
binary_read_write.test.ts: a read value survives mutation of the reader buffer, does not share the backingArrayBuffer, andreadStringstill decodes UTF-8 correctly.u8-array column after another table scan): correct before the second scan, corrupted after onmaster, correct in both cases with this change.