buffer: add regression test for copy beyond 2 GiB#62942
Conversation
rafaelmfried
left a comment
There was a problem hiding this comment.
Thanks for taking #55422 on β it's been open a long time.
One thing that may save a review round-trip: this same fix was reviewed before, in #62500, where @Renegade334 left three concrete asks. This PR diverges from all three:
1. FastCopy types. In #62500, @Renegade334 asked to change the FastCopy argument and return types from uint32_t to uint64_t. This PR leaves FastCopy at uint32_t. I built Node and probed this: with the fast path warm (confirmed β FastCopy was being hit during warmup), a copy() whose source offset exceeds 2**32 was routed to SlowCopy, not FastCopy β so the uint32_t FastCopy isn't reached with a large offset in that path and data isn't corrupted there. Even so, widening it to uint64_t removes a latent truncation and keeps the types consistent with CopyImpl (now size_t).
2. Checked operations. @Renegade334 asked to avoid .ToChecked() and use the .To(&var) pattern with an explicit early return. This PR uses IntegerValue(...).ToChecked() + CHECK_GE.
3. Test cost. The test allocates 4+ GiB buffers in test/parallel/, gated behind NODE_TEST_LARGE_BUFFER β so it never actually runs in CI as written. #62157 suggested test/pummel/ for large-buffer tests; @Renegade334 suggested keeping the allocation minimal (one large buffer, copy a tiny slice).
The core change β CopyImpl / SlowCopy / CopyArrayBuffer widened to 64-bit β looks correct to me. Aligning with the #62500 review would probably help this land.
The 32-bit truncation in Buffer.prototype.copy for buffers larger than 2 GiB (refs: nodejs#55422) was fixed as part of routing copy through V8's CopyArrayBufferBytes API, but no regression test covers offsets and byte counts above 2**32. Add an opt-in test (NODE_TEST_LARGE_BUFFER=1) that exercises copies across the uint32 boundary. Refs: nodejs#55422
3870dee to
148b003
Compare
|
Rebased onto main. While resolving the conflict I found 4383f67 ("buffer: optimize Buffer.prototype.copy") has already fixed the underlying uint32 truncation on main β |
The 32-bit truncation in
Buffer.prototype.copyfor buffers larger than 2 GiB (refs: #55422) has since been fixed on main as part of 4383f67 ("buffer: optimize Buffer.prototype.copy"), which routes the copy through V8'sCopyArrayBufferBytesand passes offsets/lengths as doubles.This PR is therefore retargeted to what main still lacks: a regression test covering copies whose source/target offsets or byte counts exceed 2**32. The test allocates >4 GiB, so it is opt-in behind
NODE_TEST_LARGE_BUFFER=1(and skipped on 32-bit platforms) to avoid CI timeouts/OOMs.Refs: #55422