Skip to content

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter
May 27, 2026
Merged

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter

Conversation

@Anshikakalpana
Copy link
Copy Markdown
Contributor

Fixes: #63461

Passing 'void' as a parameter type in getFunction or getFunctions triggers an internal assertion instead of a user-friendly error.

In C, void in a parameter list means a function takes no arguments, expressed in Node.js FFI as an empty array []. There is no valid use case for 'void' as a parameter type.

Fix:
add early validation in getFunction and getFunctions that throws ERR_INVALID_ARG_VALUE when 'void' appears in the parameters array, before wrapWithSharedBuffer is called.

@ShogunPanda
Copy link
Copy Markdown
Contributor

No, this is wrong. Don't validate at JS level but within C++ level please.

@Anshikakalpana Anshikakalpana force-pushed the ffi-void-parameter branch 2 times, most recently from 455f25e to 06410ff Compare May 23, 2026 10:19
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

No, this is wrong. Don't validate at JS level but within C++ level please.

Moved validation into the native FFI signature parsing layer in src/ffi/types.cc as suggested. Also removed the temporary JS-side validation.

Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (2e3daf6) to head (401f456).
⚠️ Report is 23 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #63504    +/-   ##
========================================
  Coverage   90.34%   90.34%            
========================================
  Files         730      730            
  Lines      234359   234468   +109     
  Branches    43923    43911    -12     
========================================
+ Hits       211720   211823   +103     
- Misses      14361    14373    +12     
+ Partials     8278     8272     -6     
Files with missing lines Coverage Δ
src/ffi/types.cc 47.51% <100.00%> (+0.60%) ⬆️

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

@ShogunPanda
Copy link
Copy Markdown
Contributor

CI failures are relevant

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 24, 2026

CI failures are relevant

Hi @ShogunPanda, the Jenkins failures are from the old commit 06410ff (confirmed via COMMIT_SHA_CHECK in the job parameters). GitHub Actions on the current commit 618bed6 are fully green. Could you please trigger a fresh CI run?

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Fixed the Windows failure — DynamicLibrary(null) rejects null on Windows. Now using kernel32.dll on Windows and null on Unix. Sorry for the multiple runs!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@ShogunPanda
Copy link
Copy Markdown
Contributor

@Anshikakalpana unfortunately the failure still seems relevant. Can you please take a look?

@Anshikakalpana Anshikakalpana force-pushed the ffi-void-parameter branch 2 times, most recently from a28da0d to 5683e58 Compare May 25, 2026 07:42
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@ShogunPanda
Copy link
Copy Markdown
Contributor

Still failing :(

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Update: found the issue — assert.throws with a regex matches against the error message, not the code. Fixed both assertions to use { code: 'ERR_INVALID_ARG_VALUE' }. Latest push should be green.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@ShogunPanda
Copy link
Copy Markdown
Contributor

@Anshikakalpana Now is the linter 😭

Fixes: nodejs#63461
Signed-off-by: Anshikakalpana <anshikajain196872@gmail.com>
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Sorry! Trailing space on the blank line — fixed now.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@ShogunPanda ShogunPanda added ffi Issues and PRs related to experimental Foreign Function Interface support. commit-queue Add this label to land a pull request using GitHub Actions. labels May 27, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 27, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/63504
✔  Done loading data for nodejs/node/pull/63504
----------------------------------- PR info ------------------------------------
Title      ffi: validate 'void' as parameter type in getFunction and getFunctions (#63504)
Author     Anshika Jain <anshikajain196872@gmail.com> (@Anshikakalpana)
Branch     Anshikakalpana:ffi-void-parameter -> nodejs:main
Labels     needs-ci, ffi
Commits    1
 - ffi: validate 'void' as parameter type in getFunction and getFunctions
Committers 1
 - anshikakalpana <anshikajain196872@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/63504
Fixes: https://github.com/nodejs/node/issues/63461
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/63504
Fixes: https://github.com/nodejs/node/issues/63461
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 23 May 2026 09:00:33 GMT
   ✔  Approvals: 1
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/63504#pullrequestreview-4358414426
   ✘  This PR needs to wait 75 more hours to land (or 0 minutes if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-05-26T20:10:13Z: https://ci.nodejs.org/job/node-test-pull-request/73757/
- Querying data for job/node-test-pull-request/73757/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/26495351583

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 27, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 27, 2026
@nodejs-github-bot nodejs-github-bot merged commit 31c40f0 into nodejs:main May 27, 2026
89 checks passed
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Landed in 31c40f0

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. ffi Issues and PRs related to experimental Foreign Function Interface support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi: 'void' as parameter type triggers internal assertion

4 participants