Skip to content

ffi: keep DynamicLibrary alive while callbacks are registered - #66842

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-dynlib-gc
Open

trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-dynlib-gc

Conversation

@trivikr

@trivikr trivikr commented Oct 19, 2026

Copy link
Copy Markdown
Member

Fixes: #66841

registerCallback() returns a plain bigint address, which doesn't keep the owning DynamicLibrary reachable. If user code dropped the library without calling close(), GC ran ~DynamicLibrary() and freed every ffi_closure. Native code still holding a callback pointer then segfaulted, even though no documented rule was broken.

This PR makes the library a strong reference while it has registered callbacks. It becomes weak again once the last callback is unregistered or close() is called. Callbacks after unrefCallback() still count, since their trampolines remain live native memory.


Assisted-by: claude:opus-5.5

Callback pointers returned by registerCallback() are plain bigint
addresses, so they do not keep the owning DynamicLibrary reachable.
If user code dropped the library without calling close(), the next GC
ran ~DynamicLibrary(), which freed every ffi_closure. Native code still
holding a callback pointer then jumped into freed trampoline memory and
crashed the process, even though close() was never called.

Make the library a strong reference while it has registered callbacks,
and make it weak again when the last callback is unregistered or the
library is closed. Callbacks released with unrefCallback() still count,
because their trampolines remain live native memory.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5.5
@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 Oct 19, 2026
@trivikr trivikr added the ffi Issues and PRs related to experimental Foreign Function Interface support. label Oct 19, 2026
@codecov

codecov Bot commented Oct 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.50%. Comparing base (1a885ca) to head (7e8c874).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66842      +/-   ##
==========================================
- Coverage   90.51%   90.50%   -0.01%     
==========================================
  Files         797      797              
  Lines      278069   278078       +9     
  Branches    53416    53418       +2     
==========================================
- Hits       251690   251671      -19     
- Misses      16760    16800      +40     
+ Partials     9619     9607      -12     
Files with missing lines Coverage Δ
src/node_ffi.cc 72.90% <100.00%> (+0.27%) ⬆️
src/node_ffi.h 77.77% <ø> (ø)

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

@trivikr trivikr added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 19, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 19, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi: GC of DynamicLibrary frees registered callbacks still held by native code

3 participants