Skip to content

src: load addon images on Windows and delete them again - #10

Merged
mcollina merged 1 commit into
mcollina:vfs-ffi-materializefrom
pipobscure:win-addonimage-sharing-fix
Sep 11, 2026
Merged

src: load addon images on Windows and delete them again#10
mcollina merged 1 commit into
mcollina:vfs-ffi-materializefrom
pipobscure:win-addonimage-sharing-fix

Conversation

@pipobscure

Copy link
Copy Markdown

Targeting this branch rather than main, so ffi.dlopen() from a VFS works on Windows before it lands. The fix is in AddonImage::Materialize()/AfterOpen(), which this branch moves but does not otherwise change, so it applies equally to main if you would rather it went there first.

Loading from bytes has never worked on Windows

Materialize() keeps its temporary file open with GENERIC_WRITE across the load. The loader opens a DLL for read and execute while sharing read alone, and sharing is checked in both directions, so an open handle holding write access is refused whatever this side shares. Every load fails with ERROR_SHARING_VIOLATION:

Error: The process cannot access the file because it is being used by another process.
    \\.\nul\vfs\0\binding.node
    at process.dlopen (node:internal/vfs/setup:1015:14)

On main that is test-dlopen-binary, test-permission-dlopen-binary and test-vfs-addon, all failing on Windows today. On this branch it is also every ffi.dlopen() of a library inside a mounted VFS, since it goes through the same path.

The fix is to write the image and close it again before loading, so nothing holds the file when the loader opens it.

Fixing the load exposes that the cleanup never worked either

Windows will not unlink a file backing a mapped image section. Measured on Windows 11, with the DLL loaded:

attempt result
delete-on-close handle closed file survives
DeleteFileW() ERROR_ACCESS_DENIED
FileDispositionInfoEx, POSIX semantics ERROR_ACCESS_DENIED
FreeLibrary(), then close the handle deleted

So the retained delete-on-close handle could never have removed anything. It only looked correct because the load failed first and the file went on the failure path, where nothing had mapped it. Fixing the load alone leaks one image per addon into %TEMP%, which is why this is one commit and not two.

An image can only go once its module is unloaded, and Node keeps addons loaded for the life of the process, so each image is kept with the module it was loaded as and both are released at exit. Only materialized images are unloaded; an addon loaded from a real path is untouched. The hook is registered during static initialisation, because atexit() runs handlers last-registered-first and registering before main() puts it behind every handler registered while running.

At exit the delete is attempted before the unload. That doubles as the test for whether the image is still mapped, that being the only thing that can stop it: an FFI library the caller already close()d has been unloaded by uv_dlclose() and its image just goes, where unloading it again through the stale module handle would be wrong. test-ffi-vfs.js calls lib.close(), so this path is exercised.

With no handle to retain, the delete-on-close file and the read-only reopen both go away: Materialize() writes the image, closes it, and records its path.

Tests

test-dlopen-binary-image-cleanup.js loads an addon from bytes repeatedly in a child process with TMPDIR/TMP/TEMP pointed at a private directory, then asserts after the child exits that no image is left behind. It covers each platform's contract: on Linux the image is a memfd, so it also counts /proc/self/fd across the loads to catch a descriptor leak, which the on-disk assertion cannot see; on other POSIX and on Windows the directory must be empty afterwards.

Verification

Windows 11 26200, VS 2022, clang-cl 19.1.5, x64 release:

  • addons, js-native-api, node-api, ffi, sea, plus the four dlopen/VFS tests: 245/245, no failures
  • ffi/test-ffi-vfs passes, so this branch's feature works on Windows
  • test-dlopen-binary, test-permission-dlopen-binary, test-vfs-addon pass, having failed before
  • four images present during a run, zero after the process exits
  • cpplint, eslint and lint-md clean

Worth a look in review

  • FreeLibrary() at exit runs the addon's DllMain(DLL_PROCESS_DETACH). Registering the hook before main() puts it behind everything registered at runtime and every test passes, but that is evidence rather than proof, and an addon that misbehaves at detach would do so here.
  • An FFI library closed early keeps its image until process exit: delayed, not leaked. Removing it at close() would mean plumbing from DynamicLibrary back into AddonImage, which seemed out of scope.
  • Holding a read handle across the load would have kept another process from swapping the file between write and load. That is dropped here: %TEMP% is per-user, unlike the /tmp the POSIX path hardens against with mkdtemp(0700) and O_EXCL|O_NOFOLLOW.

One note on the description of this PR: "Windows retains the delete-on-close handle for the process lifetime, exactly as for addons" is no longer accurate, and the addon behaviour it referred to did not work.

🤖 Generated with Claude Code

Loading a shared object from bytes never worked on Windows. Materialize()
kept its temporary file open with GENERIC_WRITE across the load, and the
loader opens a DLL for read and execute while sharing read alone. Sharing
is checked in both directions, so an open handle holding write access is
refused whatever this side shares: every load failed with
ERROR_SHARING_VIOLATION, "The process cannot access the file because it is
being used by another process". That is every VFS-resident addon, and with
it test-dlopen-binary, test-permission-dlopen-binary and test-vfs-addon.

Write the image and close it again before loading, so nothing holds the
file when the loader opens it.

That exposes the other half. The file cannot be removed while it is loaded:
Windows refuses to unlink a file backing a mapped image section, by
delete-on-close, by DeleteFile() and by a POSIX-semantics disposition
alike, all with ERROR_ACCESS_DENIED. The delete-on-close handle the code
retained could therefore never have removed anything; it only appeared to
work because the load failed first and the file was deleted on that path.
Fixing the load alone leaks an image per addon into the temporary directory.

An image can only go once its module is unloaded, and Node keeps addons
loaded for the life of the process, so keep each image with the module it
was loaded as and release both at exit. Only materialized images are
unloaded -- an addon loaded from a real path is untouched. The hook is
registered during static initialisation because atexit() runs handlers
last-registered-first, which puts it behind every handler registered while
running.

At exit the delete is attempted before the unload, which doubles as the
test for whether the image is still mapped, that being the only thing that
can stop it. An FFI library the caller already close()d has been unloaded
by uv_dlclose() and its image just goes; unloading it again through the
stale module handle would be wrong.

With no handle to retain, the delete-on-close file and the read-only reopen
go away: Materialize() writes the image, closes it and records its path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@mcollina mcollina left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

lgtm

@mcollina
mcollina merged commit f0084c5 into mcollina:vfs-ffi-materialize Sep 11, 2026
19 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants