[pipobscure]

On Linux an addon or FFI library inside a VFS is loaded from an anonymous memfd through /proc/self/fd/N, and the memfd is closed right after the load. The next image could then get the same fd number, hence the same path, and the dynamic loader, which knows a loaded object by the path it was opened with, returned the object already loaded under that path instead of loading the new file. So the second of any two addons or libraries loaded from a VFS resolved to the first: an addon after an FFI library failed with "Module did not self-register", an FFI library after an addon had none of its own symbols, and two addons silently became the same one.

Before loading, ask the loader whether the image's path is taken, using dlopen() with RTLD_NOLOAD, and move the fd to a higher number with F_DUPFD_CLOEXEC while it is. The fd is held from the check through the load, so nothing else can take the path in between, and this needs neither a counter nor a lock. The fd is still closed right after the load, because the load's mapping keeps the memory alive from then on, so no file descriptor outlives a load, whether the library is closed, a worker exits, or the load fails.

An object that has really been unloaded gives its number back, so only images loaded at the same time compete for numbers. An object that dlclose() leaves loaded (for example one with an STB_GNU_UNIQUE symbol or a thread_local destructor) keeps its number, as do addons on the main thread, which are never unloaded. If so many images are loaded that no number is left, the image goes through the existing temp-file fallback, which a noexec or read-only temporary directory rules out. Each load checks every number taken below the one it ends up with, so the cost grows with the number of images loaded.

The first two commits are earlier approaches that the last one replaces: keeping each loaded memfd open until exit leaked one fd per load, and a process-wide counter used up one fd number per load, even for images closed again.

The fallback and Windows are otherwise unchanged. This was tested on glibc, not on musl.

[nodejs-github-bot]

Review requested:

• @nodejs/ffi

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.68%. Comparing base (98dbf60) to head (21cb9f5).
⚠️ Report is 63 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_binding.cc	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #67068      +/-   ##
==========================================
+ Coverage   90.65%   90.68%   +0.03%     
==========================================
  Files         797      797              
  Lines      278268   278802     +534     
  Branches    53517    53625     +108     
==========================================
+ Hits       252261   252831     +570     
+ Misses      16386    16323      -63     
- Partials     9621     9648      +27     

Files with missing lines	Coverage Δ
src/node_binding.h	80.00% <ø> (ø)
src/node_binding.cc	76.71% <93.33%> (+5.92%)	⬆️

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

[pipobscure]

@mcollina @jasnell @trivikr can one of you give this a request-ci and a review please

[addaleax]

Do we have to leak here? Can we just close after uv_dlclose()?

There is an expectation that Node.js instances clean up file descriptors opened by them unless explicitly told otherwise.

[pipobscure]

Do we have to leak here

You might have a point. I'm reworking it with a slightly better idea that makes sure we don't leak. uv_dlclode() doesn't solve the problem, since that would just lead to reused /proc/self/fs/<x> numbers and therefore fail on loading later libraries.

Hold for changes comint

[pipobscure]

You're right, we didn't have to leak, and I've pushed a fix. The memfd now closes right after dlopen(), so no fd outlives a load, whether it's ffi.close(), worker exit or a failed load.
I didn't close after uv_dlclose() because dlclose() can leave an object loaded, so the next image would reuse the fd number and get the stale object. Main-thread addons are also never unloaded, so their fds would have no point at which to close. Instead, each image takes an fd number no earlier image has had, which keeps its /proc/self/fd/N path unique. If the numbers run out, it falls back to the temp-file path.
Windows still keeps its image files until process exit, since it can't delete them while they're mapped.

[addaleax]

I don't think this is a good permanent solution tbh, but I don't think it hurts either right now

[addaleax]

This will reliably start to break once the fd number reaches the EMFILE limit (ulimit -n) – it doesn't matter how many file descriptors are actually open, the resource limit limits the maximum fd value.

[pipobscure]

True. Do you have any idea how to solve that though? I'd actually like to solve this in a solid permanent way, so if you have any ideas I'd very much appreciate it!

[addaleax]

Yeah no, if something had come to mind, I'd have mentioned it. The real bug here is the dynamic loader's behavior, imo, but I also realize it's obviously a wide-impact and slow-moving change if one were to tackle it there.

I've done a bit of testing though, and it seems there's a ridiculous solution that might just work – while glibc dlopen caches by both path and inode (and at least the latter seems to be spec-mandated), you can get it to work for distinct files by using denormalized path representations – i.e. if the file has been replaced in between, dlopen("/foo"), dlopen("//foo"), dlopen("/./foo"), dlopen("//./foo") etc. will actually give you different objects.

So, and I know this sounds messed up at first, but we could probably just keep a counter, and when loading a temp object, use its binary translation to translate 1s to / and (non-leading) 0s to /. and then go off and load /proc/self/fd/42, //proc/self/fd/42, //./proc/self/fd/42, ///proc/self/fd/42, //././proc/self/fd/42, etc. With the standard Linux path length restriction of 4096, that should give us at least 2^2039 combinations.

[pipobscure]

🤣 The funny thing is that's pretty much what I had tried first and discarded as way too hacky, they'll never go for that.😂

What do you think of what I tried now, to just check on load. It seems a good compromise to me, but then 🤷

[addaleax]

Yeah, it's way too hacky to be considered good solution, but I don't think we'd need to worry about glibc's behavior changing too quickly.

But yes, the current approach probably works a little better 🙂

[nodejs-github-bot]

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

[pipobscure]

Given the valid criticisms that @addaleax raised, and the fact that she has more than a point I'd like to solve this in a cleaner way. If anyone has any ideas I'd appreciate hearing them.

I'm also thinking that maybe simpler is better. So what if we didn't do any of the platform dependent divergence and instead just did what we do on Windows
(doesn't work because of noexec on most tmpfs)

Thoughts very much appreciated!

[pipobscure]

@addaleax

You were right about the counter: it used up a number on every load, even for libraries that had been closed, so a loop would reliably reach ulimit -n. I've pushed a replacement in 21cb9f5.

A number only has to differ from those of objects that are still loaded, and the loader can tell us which those are. So before each load we call dlopen(path, RTLD_NOLOAD) on /proc/self/fd/N, and if the path is taken we move the fd up with F_DUPFD_CLOEXEC and check again. There's no counter or lock, because holding the fd from the check through to the load stops anything else taking that path in between. The fd still closes right after dlopen(), so none outlives a load.

A library that has really unloaded gives its number back, so the limit now depends on how many images are loaded at once, not how many have ever been loaded. With ulimit -n 128 and no usable temp dir (so memfd only), 512 FFI open and close cycles pass. In a small C test at a limit of 64, 100,000 load and unload cycles never went above fd 3.

It can still run out, but only with roughly ulimit -n images loaded at the same time, including libraries dlclose() can't unload and main-thread addons. At that point it falls back to a temp file, which won't work if temp is noexec or read-only. Each load also checks every taken number on its way up, so it gets slower as more images are loaded: about 0.7 ms with 500 and 10 ms with 2,000.

I looked for a way to avoid the path match altogether, but glibc has no dlopen flag for it and no fdlopen. dlmopen namespaces gave out after 13 in my test and each loads its own libc. Temp files everywhere aren't an option either, since Docker's tmpfs mounts are noexec by default and --read-only leaves /tmp unwritable.

On musl, which doesn't match by path, the same logic ran correctly in a C test (5,000 cycles with no check hits), but I haven't run a musl build of Node.

Your approval was for the counter version, so please take another look at this one, particularly if you know of a case where RTLD_NOLOAD wouldn't report a loaded object.

[nodejs-github-bot]

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