Skip to content

buffer: fix 32-bit truncation in SlowCopy for buffers larger than 4 GiB#62500

Open
crawfordxx wants to merge 1 commit intonodejs:mainfrom
crawfordxx:fix-buffer-copy-overflow-for-large-buffers
Open

buffer: fix 32-bit truncation in SlowCopy for buffers larger than 4 GiB#62500
crawfordxx wants to merge 1 commit intonodejs:mainfrom
crawfordxx:fix-buffer-copy-overflow-for-large-buffers

Conversation

@crawfordxx
Copy link
Copy Markdown

Summary

SlowCopy in src/node_buffer.cc extracted targetStart, sourceStart,
and to_copy via .As<Uint32>()->Value(), silently truncating values that
exceed 2^32 - 1 (4 GiB). This caused Buffer.prototype.copy() to copy
the wrong number of bytes or write to the wrong position when dealing with
buffers larger than 4 GiB.

Root cause was introduced in #54087 ("buffer: use native copy impl"), which
replaced the original ParseArrayIndex-based extraction (using size_t) with
Uint32Value (32-bit).

Changes

  • src/node_buffer.cc: Use IntegerValue() (returns int64_t) in SlowCopy
    instead of .As<Uint32>()->Value(), and widen CopyImpl's parameter types
    from uint32_t to size_t.
  • FastCopy is left unchanged — V8's Fast API guarantees it is only invoked
    when all arguments fit in uint32_t; for larger values V8 falls back to
    SlowCopy automatically.
  • test/parallel/test-buffer-copy-large.js: New test that allocates a buffer
    slightly larger than 4 GiB and verifies Buffer.prototype.copy() copies all
    bytes correctly. The test skips gracefully on systems with insufficient memory.

Test

// Before fix: only 5 bytes were copied (2^32 + 5 mod 2^32 = 5).
// After fix: all FOUR_GB + 5 bytes are copied correctly.
const src = Buffer.alloc(2 ** 32 + 5, 0xbb);
const dst = Buffer.alloc(src.length, 0xaa);
src.copy(dst);
assert.strictEqual(dst[dst.length - 1], 0xbb);

Fixes #55422

SlowCopy extracted targetStart, sourceStart, and to_copy using
`.As<Uint32>()->Value()`, which silently truncates values that exceed
2^32-1 (4 GiB). As a consequence, Buffer.prototype.copy() produced
incorrect results when operating on buffers larger than 4 GiB: only the
lower 32 bits of each offset/count were used, causing writes to the
wrong location or copying far fewer bytes than requested.

Fix by extracting those arguments with `IntegerValue()` (which yields
int64_t) and casting to size_t, matching the behaviour of the previous
`ParseArrayIndex`-based implementation that existed before PR nodejs#54087.
CopyImpl's parameter types are widened to size_t accordingly.

FastCopy retains uint32_t parameters because V8's Fast API contract
guarantees it is only invoked when all arguments fit in uint32_t; for
larger values V8 falls back to SlowCopy automatically.

Add test/parallel/test-buffer-copy-large.js to cover the regression.
The test allocates a buffer slightly larger than 4 GiB and verifies
that Buffer.prototype.copy() copies all bytes correctly; it skips
gracefully on systems with insufficient memory.

Fixes: nodejs#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 Mar 30, 2026
Comment on lines +607 to 614
const size_t target_start =
static_cast<size_t>(args[2]->IntegerValue(ctx).ToChecked());
const size_t source_start =
static_cast<size_t>(args[3]->IntegerValue(ctx).ToChecked());
const size_t to_copy =
static_cast<size_t>(args[4]->IntegerValue(ctx).ToChecked());

CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);
Copy link
Copy Markdown
Member

@Renegade334 Renegade334 Mar 30, 2026

Choose a reason for hiding this comment

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

Don't use checked operations here.

Suggested change
const size_t target_start =
static_cast<size_t>(args[2]->IntegerValue(ctx).ToChecked());
const size_t source_start =
static_cast<size_t>(args[3]->IntegerValue(ctx).ToChecked());
const size_t to_copy =
static_cast<size_t>(args[4]->IntegerValue(ctx).ToChecked());
CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);
int64_t target_start, source_start, to_copy;
if (!args[2]->IntegerValue(env->context()).To(&target_start) ||
!args[3]->IntegerValue(env->context()).To(&source_start) ||
!args[4]->IntegerValue(env->context()).To(&to_copy)) {
return;
}
CopyImpl(source_obj,
target_obj,
static_cast<size_t>(target_start),
static_cast<size_t>(source_start),
static_cast<size_t>(to_copy));

Comment on lines 620 to 627
uint32_t FastCopy(Local<Value> receiver,
Local<Value> source_obj,
Local<Value> target_obj,
uint32_t target_start,
uint32_t source_start,
uint32_t to_copy,
// NOLINTNEXTLINE(runtime/references)
FastApiCallbackOptions& options) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also change the FastCopy argument and return types from uint32_t to uint64_t.

// Copy the entire large buffer. The native _copy binding receives
// to_copy = FOUR_GB + 5. Before the fix, Uint32 truncation reduced this to
// 5, leaving all but the first 5 bytes of dst unchanged (still 0xaa).
src.copy(dst);
Copy link
Copy Markdown
Member

@Renegade334 Renegade334 Mar 30, 2026

Choose a reason for hiding this comment

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

Copying gigabytes of buffer memory is a massive operation. Can this just test copying a small chunk of memory with source/target offsets of >2^32?

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.

Buffer.concat and Buffer.copy silently produce invalid results when the operation involves indices equal or greater than 2^32

3 participants