Repository navigation
Conversation
|
Updated 1:37 AM PT - Oct 1st, 2026
❌ @robobun, your commit a383dfd has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44083That installs a local version of the PR into your bun-44083 --bun |
…ore dlopen
A compiled executable wrote one embedded library alone to
{tmpdir}/.bun-{euid}-{hash}.{ext} before dlopen. The library's own
dependencies resolve relative to that path ($ORIGIN on Linux,
@loader_path on macOS, the DLL directory on Windows), so an addon that
links a shared library next to it failed with ERR_DLOPEN_FAILED even
when the library was embedded with --asset.
The standalone writer now records every embedded shared library
(.node, .so, .so.N, .dylib, .dll) in the module graph: each file's
index, a hash over the set, and for the bundler's hoisted
[name]-[hash].node the index of the --asset copy read from the same
source file. That copy's bytes are stored once.
resolve_embedded_file_to_buf writes the recorded set once into
{tmpdir}/.bun-{euid}-{set hash}/ with the embedded layout and returns
the requested file's path inside it. The hoisted addon resolves to its
--asset copy, next to its dependencies. The warm path is one lstat
relative to the temp dir, with no content hash per call. A scratch
directory is renamed into place, so Workers and concurrent processes
never see a half-written set.
c382d94 to
a63dcc8
Compare
There was a problem hiding this comment.
Beyond the inline finding, I also checked the Windows build against the new owner field on MirrorDir (src/runtime/jsc_hooks.rs:4593): the #[cfg(windows)] arm of is_ours at line 4608 reads it, so the workspace's dead_code = "deny" is not tripped there.
Extended reasoning...
The latest two commits (7d9d924, f7f1e2d) change how the mirror owner is determined and add a Windows read of the field; the Windows dead-code compile-failure claim was ruled out by reading the cfg arm. The remaining posted finding concerns the owner-from-filesystem fallback on squashing mounts, and the hunt exited on the max-bugs bound, so this is not an approval.
f7f1e2d to
1e173bf
Compare
…move it at exit
The canonical directory `.bun-{euid}-{hash}` is this user's only when the
filesystem shows the euid as its owner. Some temp filesystems show another
owner for what the process itself creates (NFS `root_squash`, CIFS `uid=`,
drvfs). There the process did not recognise its own copy either, so each
`dlopen` call wrote the whole set again. Nothing removed an own copy.
- Read the owner that the filesystem shows for the scratch directory from
its handle. The process knows a copy of its own by that owner and by
the euid it had when it made the copy.
- When that owner is not the euid, do not move the scratch directory to
the canonical name: no later run could tell it from another user's.
- Remove own copies in an exit callback. It makes one pass inside the
copy over the members and their directories, and does not retry.
A package that ships every platform's prebuilt binaries embeds them all under --asset. A Linux executable has no use for a .dll or .dylib, so the record leaves them out and the mirror never writes them. A .node stays in on every target.
|
CI at a383dfd: 181 of 182 jobs pass. The one red job is |
Fixes #44063
Problem
--compileexecutable whose.nodeaddon links a library next to it fails withERR_DLOPEN_FAILED(libfoo.so: cannot open shared object file: No such file or directory), even with the library embedded. sharp fails the same way (Library not loaded: @rpath/libvips-cpp...dylib).resolve_embedded_file_to_buf(src/runtime/jsc_hooks.rs) wrote the requested file alone into the temp directory. The dependency is not there, and another user can put one there.Fix
NativeLibrarySet). The runtime writes the set once into a private directory,{tmpdir}/.bun-{euid}-{set hash}/, with the embedded layout...stays inside.test/bundler/compile-asset-bunfs.test.ts,29585.test.ts,30717.test.ts,napi.test.ts.Background
dlopen(2)cannot read/$bunfs/, a compiled executable's virtual filesystem, so a library goes to disk first.$ORIGIN(@loader_pathon macOS) is the directory of the library that names it.Downsides
mkdiratcalls and onefstat.Notes
Own copies (bbea975)
Review on 1e173bf found this: a temp directory can show another owner than the euid for what the process itself creates (NFS
root_squash, CIFSuid=, drvfs). The process then did not recognise its own copy either, so eachdlopencall wrote the set again. Nothing removed an own copy, there or when another user held the name.A first fix (7d9d924) took the shown owner for the canonical name too. It is dropped: another principal with that shown owner could create the directory first.
This commit keeps the rule for the canonical name: this user's only by the euid. It reads the owner that the filesystem shows for the scratch directory from its handle. The process knows its own copy by that owner and by the euid it had. When that owner is not the euid, the scratch directory does not take the canonical name. An exit callback removes the own copies: one pass inside the copy over the members and their directories, with no retry.
Measured on release builds, linux-x64, 3 runs of 3
dlopencalls each.setfsuid(12345)throughbun:ffistands for such a filesystem: the owner of a new file is then not the euid.fstat)text(size)Limits. A process that a signal kills leaves its copy (SIGTERM measured). NFS and SMB keep a library that is still loaded until the process is gone, and Windows does not remove it, so directories of the copy can stay. Not measured: no such mount in the build container.
Fail before: at 1e173bf the reworked test finds the own copy in the temp directory after the process exits, and 3 entries where 1 is expected for the other owner.
Self-review of this commit: 13 concerns raised, 10 addressed. Not addressed: a signal skips the exit callback (the limit above). A relative
BUN_TMPDIRwith a laterchdirmakes the exit pass look in another directory (each call already has that). On a filesystem that shows one owner for every user, a temp sweeper can remove the own copy of a running process and another user can then create its name again (the process checks the name by owner, as before).After the eight levels (1e173bf)
Three things that
maindoes and 46d44da did not. Each is checked on release builds ofmaina4f1429 and of 46d44da, and on a debug build of this head (linux-x64).better_sqlite3.node.process.dlopenrefuses a path that ends in that name, and it looks at the path after extraction. Onmainthat path is a hash, so the check never matched an embedded addon: it loads. The mirror keeps the embedded name, so 46d44da threw'better-sqlite3' is not yet supported in Bun. The check now skips a path that came from the executable. It still applies to every other path.dlopencalls and 5 Workers: 45 copies of the whole set at 46d44da, one for each call, and 3 now, one for each process.mainleaves 45 single files. The process remembers the copy it wrote and checks it like the mirror on each call.mainloads.ERR_DLOPEN_FAILEDand one with 3 copies where 1 is expected.mainand on 46d44da, with aptracesyscall counter (straceis not installed):--asset), the other user's library at seven levels from the temp directory up:mainloads it in 18, 46d44da in 0. A climb of 9 or 10 levels from a hoisted addon loads it at 46d44da.mkdirat0 to 16,openat23 to 26,renameat1 to 2. Warm: one morenewfstatat.main, 4,269,808 at 46d44da.--asset node_modules:ERR_DLOPEN_FAILEDonmain; loads at 46d44da, 17,055,960 bytes in 2 files,mkdirat27.text(size): 80,660,492 onmain, 80,676,108 at 46d44da.matchthatclippy::manual_maprejects).dlopenneeds. It needs the reader that the depth from search paths needs. A version is onrobobun/b76cf9cb/mirror-pad-closure.The eight levels (46d44da)
mainhas this for$ORIGINitself: the released 1.4.3-canary prints{"declared":666,"built":-1,"ffi":667}for the new testa library that another user put in the temp directory is not loaded.maindoes not have it for sharp, whose entries resolve to/there.1777temp directory, this head. Nothing of the other user loads for:$ORIGIN,$ORIGIN/deps,$ORIGIN/../libfrom a hoisted addon,$ORIGIN/../../libwith--asset lib, a five-level entry fromnode_modules/@img/x/lib, an eight-level entry from a hoisted addon, and a path built in code (dlopen("$ORIGIN/../lib/x.so"), anddladdrplus../lib). A nine-level entry from a hoisted addon loads the other user's library: that is the limit in Downsides. At 8bd6f2a that entry resolved to/libwith the default/tmp, so the levels move the first exposed height from one level to nine.{"addon":666,"scoped":706}and{"declared":666,"built":666,"ffi":667}. With this head all 13 tests of the file pass.straceandperfare not installed in the build container, so the syscall counts come from aptracecounter.LD_DEBUG=libs): 4, 4, 4 before and 0, 0, 0 after, for a one-level, a two-level and a five-level climbdlopen: the same syscalls before and after (openat,geteuid, onenewfstatatper member plus one,close)mkdirat2 to 18 with--asset lib, and 1 to 16 for a hoisted addon, which also gets 2 moreopenatand 1 moreclosefor its new parent directorystatandsizeon release builds of 8bd6f2a and of this head)DT_RPATH,DT_RUNPATH,LC_RPATH) and recorded at build time. It is exact for declared paths at any height. A library with no such path stays at the top of the mirror, so a path built in code still loaded the other user's library in the two-user run. It also needs a hand-written ELF and Mach-O reader. That work is on the branchrobobun/b008d3fc/recorded-depth.{tmpdir}/bunx-{uid}-{pkg}/node_modules/..., and sharp's five-level entry from there is the temp directory. bunx: store the package cache under the per-user bun cache directory #31447 moves that cache.bun_standalone_graphjoins the Miri set, so CI runs the unit tests ofnative_libs.The mirror (first five commits)
Fix, in full:
NativeLibrarySet, flagHAS_NATIVE_LIBRARY_SET): each file's index, a hash over the set, and for the bundler's hoistedaddon-[hash].nodethe index of the--assetcopy read from the same source file (both paths go throughrealpath). The--assetcopy is always the one loaded. Its bytes are stored once.{tmpdir}/.bun-{euid}-{set hash}/with the embedded layout and returns the requested file's path inside it.process.dlopen,require("x.node")andbun:ffiall go through it.lstateach, no content hash. A missing member is written back in place through a temp name and a rename. The directory itself is never removed, so a process loading from it keeps what it sees. A new set is written into a scratch directory and renamed into place, so Workers and concurrent processes never see a half-written set. If the set cannot be written in full, the requested file is mirrored on its own, as before.test/bundler/compile-asset-bunfs.test.ts(two new tests, the released bun fails both with the error above),test/regression/issue/29585.test.ts(one copy across 10 dlopens, 5 Workers, restart),30717.test.ts,test/napi/napi.test.ts --compile,bun-build-compile.test.ts.Background, in full:
/$bunfs/is the virtual filesystem of a compiled executable.dlopen(2)reads real paths only, so Dedupe extracted embedded native modules in compiled binaries #29587 wrote an embedded library to a content-hashed, euid-scoped, 0600 file in the temp dir. This keeps those properties at directory level.StandaloneModuleGraphchains optional records after the module table inFlagsbit order. An older bun ignores a bit it does not know, and an executable without the record is mirrored one file at a time, as before.dlopenof every process (19 to 25 ms for an 18.9 MB sharp-sized set in a release bun, JS probe throughBun.embeddedFiles) and keeps the addon embedded twice. Rpath parsing needs ELF, Mach-O and PE readers and still cannot reach sharp's$ORIGIN/../../sharp-libvips-*/libwithout the whole tree.Costs, in full:
du -b), about 18 MiB for sharp. Paid once per set per machine, and again after a temp sweep. A user who embeds many libraries and loads one pays for all of them.dlopencosts onelstatper member of the set plus one for the directory, instead of onelstatof the file: 4 syscalls before (geteuid, openat, fstatat, close) and 3 + N after. The per-call content hash is gone. Counted from the code: strace and perf are not installed in the build container.--asset(the record), -15,538 B with--asset lib(the addon's second copy is stored once).More:
.bun-0-e7c1e2b45a40e6c2/_/_/_/_/_/_/_/_/lib/{addon.node,libfoo.so}. The hoistedaddon-8hekbvxp.nodeis not written: it aliaseslib/addon.node. A symlinked--assetdirectory, a single-file--asset lib/addon.node, and--asset-naming assets/native/[name]-[hash].[ext]all produce the alias and load.open_trusted_temp_dirhas one call to replace and its check covers both paths.LoadLibraryExWwithLOAD_WITH_ALTERED_SEARCH_PATH(already used byprocess.dlopenandbun:ffi) finds a dependent DLL next to the loaded one. Renaming the scratch directory onto an existing one fails withEPERMthere, the same fallback applies. Not run on Windows here (the test needs a C toolchain and skips there);bun run rust:check-allis green...segments (custom--asset-naming [dir]/...) are rewritten to_.._the waybun builddoes, so every member stays inside the mirror.Offsetstrailer ofbun build --compile app.js [--asset lib]built by the released 1.4.3 and by this branch (5 B of the delta is baseline drift between the two builds).lib/addon.node, so that goal is met here.--assettree are skipped today (versioned soname links such aslibfoo.so.1 -> libfoo.so.1.0.0), bare-specifier resolution from/$bunfs/root/node_modules("bun build" does not embed binaries from node_modules correctly #15374),bun:sqlite'ssetCustomSQLiteandcc({library}), which never call this helper. Data files next to an addon are not mirrored.dlopenthe dependency by path throughbun:ffifirst so the loader matches it by soname, thenprocess.dlopenthe addon.uid=) never reports the euid as owner, so the canonical directory is never trusted there and each process keeps a copy of its own. That fails closed on purpose: the mapped uid is shared with other principals.BUN_TMPDIRon a private filesystem is the way out.cargo clippyclean on the touched crates.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/napi/napi.test.ts, test/bundler/compile-asset-bunfs.test.ts