Skip to content

ffi: throw on missing memory helper arguments - #65500

Open
soulee-dev wants to merge 1 commit into
nodejs:mainfrom
soulee-dev:ffi-throw-on-missing-arguments
Open

ffi: throw on missing memory helper arguments#65500
soulee-dev wants to merge 1 commit into
nodejs:mainfrom
soulee-dev:ffi-throw-on-missing-arguments

Conversation

@soulee-dev

Copy link
Copy Markdown
Contributor

ffi.getInt8() through ffi.getFloat64(), ffi.setInt8() through ffi.setFloat64(), ffi.toBuffer() and ffi.toArrayBuffer() return undefined instead of throwing when a required argument is omitted, so a call that read or wrote nothing cannot be told apart from one that read a zero byte.

GetValidatedPointerAddress() and GetValidatedSize() already reject the same argument when it is passed explicitly as undefined. The args.Length() test in front of them short-circuits the call and returns Nothing without scheduling an exception. These six are the only tests in src/ where args.Length() can skip a call that throws; the only other Length() tests that guard a call at all guard Buffer::HasInstance(), which cannot throw.

$ # before
$ node --experimental-ffi -p "require('node:ffi').getUint8()"
undefined

$ # after
$ node --experimental-ffi -p "require('node:ffi').getUint8()"
TypeError: The pointer must be a bigint

Drop those tests. FunctionCallbackInfo::operator[] returns Undefined for an out-of-range index, which is exactly the value these validators reject, so each missing argument now produces the error its explicit undefined counterpart produces. The tests that guard an explicit throw, such as the ones in ToString() and GetRawPointer(), are left alone.

ExportBytes() carried the same two tests. They are unreachable through the public API because exportBytes is not exported and its three callers all validate len in JavaScript first, but they are the same shape.

No documentation change is needed: doc/api/ffi.md already lists these arguments as required.

Fixes: #65499
Refs: #62072
Refs: #62858

ffi.getInt8() through ffi.getFloat64(), ffi.setInt8() through
ffi.setFloat64(), ffi.toBuffer() and ffi.toArrayBuffer() return
undefined instead of throwing when a required argument is omitted, so a
call that read or wrote nothing cannot be told apart from one that read
a zero byte. All 22 helpers behave this way.

GetValidatedPointerAddress() and GetValidatedSize() already reject the
same argument when it is passed explicitly as undefined. The
args.Length() test in front of them short-circuits the call and returns
Nothing without scheduling an exception. These six are the only tests in
src/ where args.Length() can skip a call that throws; the only other
Length() tests that guard a call at all guard Buffer::HasInstance(),
which cannot throw. The remaining tests in this file guard an inline
predicate and throw in the branch, which is why setUint8(ptr) reports
"Expected an offset argument" while setUint8() reports nothing at all.

Drop those tests. FunctionCallbackInfo::operator[] returns Undefined for
an out-of-range index, which is exactly the value these validators
reject, so each missing argument now produces the error its explicit
undefined counterpart produces. The documentation already describes this
behavior: the signatures are ffi.getInt8(pointer[, offset]),
ffi.setInt8(pointer, offset, value) and
ffi.toBuffer(pointer, length[, copy]), and the getters are documented to
return a number or a bigint.

ExportBytes() carried the same two tests. They are unreachable through
the public API because exportBytes is not exported and its three callers
all validate len in JavaScript first, but they are the same shape.

Signed-off-by: Soul Lee <alus20x@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 23, 2026
@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (a48e33f) to head (8926b4d).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/ffi/data.cc 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #65500    +/-   ##
========================================
  Coverage   90.12%   90.12%            
========================================
  Files         751      751            
  Lines      252425   252589   +164     
  Branches    47471    47535    +64     
========================================
+ Hits       227504   227655   +151     
- Misses      16216    16239    +23     
+ Partials     8705     8695    -10     
Files with missing lines Coverage Δ
src/ffi/data.cc 77.55% <66.66%> (+1.83%) ⬆️

... and 61 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.

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

Labels

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.

ffi: memory helpers return undefined instead of throwing when a required argument is omitted

2 participants