Skip to content

buffer: add regression test for copy beyond 2 GiB#62942

Open
maruthang wants to merge 1 commit into
nodejs:mainfrom
maruthang:fix-55422-buffer-copy-overflow
Open

buffer: add regression test for copy beyond 2 GiB#62942
maruthang wants to merge 1 commit into
nodejs:mainfrom
maruthang:fix-55422-buffer-copy-overflow

Conversation

@maruthang

@maruthang maruthang commented Apr 25, 2026

Copy link
Copy Markdown

The 32-bit truncation in Buffer.prototype.copy for 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's CopyArrayBufferBytes and 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

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 25, 2026

@rafaelmfried rafaelmfried left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@maruthang maruthang force-pushed the fix-55422-buffer-copy-overflow branch from 3870dee to 148b003 Compare July 6, 2026 06:51
@maruthang maruthang changed the title buffer: avoid 32-bit truncation in copy beyond 2 GiB buffer: add regression test for copy beyond 2 GiB Jul 6, 2026
@maruthang

Copy link
Copy Markdown
Author

Rebased onto main. While resolving the conflict I found 4383f67 ("buffer: optimize Buffer.prototype.copy") has already fixed the underlying uint32 truncation on main β€” SlowCopy/FastCopy now take doubles and V8's CopyArrayBufferBytes handles the byte ranges. I've dropped the now-superseded src/node_buffer.cc changes and retargeted this PR to just the missing piece: an opt-in (NODE_TEST_LARGE_BUFFER=1) regression test exercising copies across the 2**32 boundary, so the behavior can't silently regress. Title and description updated accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants