ffi: reject unsafe integers as length or offset

[christianaurichzm]

GetValidatedSize() checks lengths and offsets against static_cast<double>(SIZE_MAX), which rounds up to 2^64 on 64-bit platforms. So 2 ** 64 gets through, and the static_cast<size_t>() after it is undefined behavior. With GCC 13.3 on x64 it gives 0:

const ffi = require('node:ffi');

const buf = Buffer.from([7]);
const ptr = ffi.getRawPointer(buf);

ffi.setUint8(ptr, 2 ** 64, 42);
console.log(buf[0]); // 42
console.log(ffi.toBuffer(ptr, 2 ** 64).length); // 0

The limit is now Number.MAX_SAFE_INTEGER instead of just below 2 ** 64. Anything larger may already have been rounded before it gets here (2 ** 53 + 1 === 2 ** 53), so it can't be trusted as an offset. The export*() helpers already cap their length at that value, and so does setInt64() for number values. SIZE_MAX is still the limit on 32-bit.

This changes one error code: when buffer.constants.MAX_LENGTH is Number.MAX_SAFE_INTEGER (64-bit without the V8 sandbox), toBuffer() / toArrayBuffer() with MAX_LENGTH + 1 now throw ERR_OUT_OF_RANGE instead of ERR_BUFFER_TOO_LARGE. The existing assertions for that case now only run where MAX_LENGTH is smaller, and the new test covers the other case.

On main the new test segfaults (getUint8(ptr, 2 ** 53) reads 8 PiB past the pointer). With this change test/ffi passes, parallel and sequential show no new failures, and make lint is clean.

[nodejs-github-bot]

Review requested:

• @nodejs/ffi

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.28%. Comparing base (c0ed28f) to head (0558424).
⚠️ Report is 200 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ffi/data.cc	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66216      +/-   ##
==========================================
- Coverage   90.29%   90.28%   -0.01%     
==========================================
  Files         790      789       -1     
  Lines      272883   272879       -4     
  Branches    52118    52112       -6     
==========================================
- Hits       246387   246370      -17     
- Misses      16943    16966      +23     
+ Partials     9553     9543      -10     

Files with missing lines	Coverage Δ
src/ffi/data.cc	77.24% <50.00%> (-1.05%)	⬇️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Renegade334]

My personal feeling is that we should be rejecting unsafe integers here entirely, there is no circumstance in which we should be allowing reads/writes from offsets that are being rounded.

I can't imagine a world in which someone is referencing a contiguous buffer that is more than 9 petabytes in size.

[christianaurichzm]

Makes sense, agreed. It now rejects anything above Number.MAX_SAFE_INTEGER (SIZE_MAX still applies on 32-bit), which is where the export*() helpers already cap. One side effect: on 64-bit, toBuffer(ptr, MAX_LENGTH + 1) now throws ERR_OUT_OF_RANGE instead of ERR_BUFFER_TOO_LARGE, since that value is exactly 2 ** 53, so I scoped the old assertion to builds where MAX_LENGTH is smaller. PTAL

[christianaurichzm]

@Renegade334 this now rejects unsafe integers as you suggested. If this looks good to you, could you also add request-ci? Thanks!

[Renegade334]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78084/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78097/

[nodejs-github-bot]

Landed in b9bb1bd