[v24.x] buffer: fix 32-bit truncation in Buffer.prototype.copy - #66187
aron-intframe wants to merge 1 commit into
Conversation
`SlowCopy` read `targetStart`, `sourceStart` and the byte count with `As<Uint32>()`, and `FastCopy` took them as `uint32_t`, so a value at or above 2**32 wrapped around: the copy silently read from or wrote to the wrong offset, or moved only `count % 2**32` bytes. Read them as doubles into `size_t` instead, as `main` does since 4383f67. That commit also routes the copy through a new V8 API and is labeled dont-land-on-v24.x, so this keeps the existing memmove path and only widens the argument types. Fixes: nodejs#55422 Refs: nodejs#63828 Signed-off-by: INTFRAME <hello@intframe.com>
nodejs-github-bot
added
buffer
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. Caution AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context. |
2 participants
Description
On v24.x
Buffer.prototype.copytruncatestargetStart,sourceStartand the byte count to 32 bits (As<Uint32>()inSlowCopy,uint32_tinFastCopy), so a copy touching an offset or length ≥ 2**32 silently reads from or writes to the wrong place, or copies onlycount % 2**32bytes.mainfixed this in #63828 (4383f67), which also moves to V8'sCopyArrayBufferBytesand is labeleddont-land-on-v24.x. This is the v24.x-only part of it: read the arguments as doubles intosize_tand keep thememmovepath.Buffer.concatis already unaffected on v24.x (it goes throughTypedArray.prototype.set).Measured on v24.21.0 with
buf = Buffer.alloc(2**32 + 16):buf.copy(buf, 8, 0, size - 8)moves 8 bytes instead of 4294967304, so a marker atsize - 9never reachessize - 1buf.copy(dst, 0, 2**32 + 1, 2**32 + 11)reads from offset 1The same patch applies to v22.x-staging; happy to open that as well if wanted. Only tested on linux-x64.
User-Facing Changes
None beyond the fix.
copy()keeps returning the byte count computed in JS.Tests
test/pummel/test-buffer-large-size-copy.js, the same file as onmainplus one check forsourceStart/targetStartpast 2**32. It fails on v24.21.0 (AssertionError: 0 !== 66) and passes with this change.parallel/test-buffer-*pass.