fix: guard multiBufferBuilder UnsafeAppend against short copy#896
fix: guard multiBufferBuilder UnsafeAppend against short copy#896fallintoplace wants to merge 2 commits into
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
Two issues:
1. The new test doesn't trigger the guard — it fails. multiBufferBuilder.Reserve sizes a new block to max(nbytes, blockSize) and memory.Buffer.Reserve rounds capacity up to a multiple of 64, so Reserve(4) with blockSize: 8 gives a 64-byte block. Appending 9 bytes fits, so there's no short copy and the panic never fires:
--- FAIL: TestMultiBufferBuilderUnsafeAppendPanicsOnTruncatedCopy
should panic ... Panic value: <nil>
2. Redundant guard. debug.Assert(n == len(val), ...) followed by an unconditional if n != len(val) { panic(...) } is doubled up — the debug.Assert is dead once the unconditional panic exists. And since Reserve always rounds the block up and the contract is that callers Reserve before UnsafeAppend, a short copy is effectively unreachable via the normal path — worth deciding whether an always-on branch belongs on this hot Unsafe path (vs. a debug.Assert only).
If you keep the guard, the test needs a genuinely undersized block (append a value larger than the block's rounded capacity). Verified against the PR head with go test.
|
@fallintoplace gentle nudge — still blocked on the failing test: because |
Summary
Why
This prevents silent data corruption in BinaryViewBuilder and StringViewBuilder out-of-line storage where metadata can point past the bytes actually written.
Testing