diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index edc0325bef15..dcbf86a6ffe7 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -1446,8 +1446,6 @@ pub mod fs { kind_from_mode(stat_.st_mode as bun_sys::Mode) == FileKind::SymLink; let mut file_kind = kind_from_mode(stat_.st_mode as bun_sys::Mode); - let mut symlink: &[u8] = b""; - if is_symlink { let file: Fd = if let Some(valid) = existing_fd.unwrap_valid() { valid @@ -1468,23 +1466,28 @@ pub mod fs { }; FileSystem::set_max_fd(file.native()); - // The close-or-store cleanup runs on - // BOTH success and error paths — use scopeguard so close-or-store happens even if - // fstat()/get_fd_path() return early with `?`. - let need_to_close_files = self.need_to_close_files(); - let cache_ptr: *mut EntryCache = &raw mut cache; - let _guard = scopeguard::guard(file, move |file| { - if (!store_fd || need_to_close_files) && !existing_fd.is_valid() { + let we_opened_it = !existing_fd.is_valid(); + let close_even_on_success = !store_fd || self.need_to_close_files(); + let fd_published = core::cell::Cell::new(false); + let _guard = scopeguard::guard((), |()| { + if we_opened_it && (close_even_on_success || !fd_published.get()) { let _ = bun_sys::close(file); - } else if bun_core::feature_flags::STORE_FILE_DESCRIPTORS { - // SAFETY: `cache_ptr` points into a stack local that outlives this guard. - unsafe { (*cache_ptr).fd = file }; } }); - let file_stat = bun_sys::fstat(*_guard)?; - symlink = bun_sys::get_fd_path(*_guard, &mut outpath)?; + let file_stat = bun_sys::fstat(file)?; + let symlink = bun_sys::get_fd_path(file, &mut outpath)?; file_kind = kind_from_mode(file_stat.st_mode as bun_sys::Mode); + if !symlink.is_empty() { + cache.symlink = + Interned::from_static(FilenameStore::instance().append_slice(symlink)?); + } + if bun_core::feature_flags::STORE_FILE_DESCRIPTORS + && !(we_opened_it && close_even_on_success) + { + cache.fd = file; + } + fd_published.set(true); } debug_assert!(file_kind != FileKind::SymLink); @@ -1494,10 +1497,6 @@ pub mod fs { } else { EntryKind::File }; - if !symlink.is_empty() { - cache.symlink = - Interned::from_static(FilenameStore::instance().append_slice(symlink)?); - } Ok(cache) } diff --git a/test/js/bun/resolve/resolver-kind-symlink-fd-leak.test.ts b/test/js/bun/resolve/resolver-kind-symlink-fd-leak.test.ts new file mode 100644 index 000000000000..0866c3af0e05 --- /dev/null +++ b/test/js/bun/resolve/resolver-kind-symlink-fd-leak.test.ts @@ -0,0 +1,138 @@ +// RealFS::kind() opens a symlink's target, then fstat()s and reads the fd's +// realpath. When the resolver was asked to cache the fd (store_fd, set by +// --hot / --watch), the cleanup guard wrote the fd into a stack-local +// EntryCache instead of closing it. On the success path that EntryCache is +// returned and the fd reaches the resolver's Entry cache; on the error path +// the EntryCache is dropped and the fd leaks. +// +// On Linux the realpath step is readlink("/proc/self/fd/N"), called through +// libc, so an LD_PRELOAD shim can make it fail with EIO for fds whose target +// contains a marker string. The inner process runs under --hot (store_fd=true), +// resolves one symlink per target, then counts open fds that still point at a +// marker target. +import { afterAll, beforeAll, expect, test } from "bun:test"; +import { bunEnv, bunExe, isLinux, isMusl, tempDir } from "harness"; +import { symlinkSync } from "node:fs"; +import { join } from "node:path"; + +const cc = Bun.which("cc") || Bun.which("gcc") || Bun.which("clang"); +const LINK_COUNT = 16; +const MARKER = "__KIND_FAIL__"; + +const SHIM_C = /* c */ ` +#define _GNU_SOURCE +#include +#include +#include +#include +#include + +static ssize_t (*real_readlink)(const char *, char *, size_t); +static const char *off_path; + +__attribute__((constructor)) static void init(void) { + real_readlink = (ssize_t (*)(const char *, char *, size_t))dlsym(RTLD_NEXT, "readlink"); + off_path = getenv("SHIM_OFF_PATH"); +} + +ssize_t readlink(const char *path, char *buf, size_t bufsiz) { + if (!real_readlink) + real_readlink = (ssize_t (*)(const char *, char *, size_t))dlsym(RTLD_NEXT, "readlink"); + ssize_t n = real_readlink(path, buf, bufsiz); + if (n <= 0) return n; + if (off_path && access(off_path, F_OK) == 0) return n; + if (strncmp(path, "/proc/self/fd/", 14) != 0) return n; + size_t m = (size_t)n < bufsiz ? (size_t)n : bufsiz - 1; + char saved = buf[m]; + buf[m] = 0; + int hit = strstr(buf, "${MARKER}") != NULL; + buf[m] = saved; + if (hit) { errno = EIO; return -1; } + return n; +} +`; + +const INNER = /* ts */ ` +import { readdirSync, readlinkSync, writeFileSync } from "node:fs"; +const N = ${LINK_COUNT}; +let failed = 0; +for (let i = 0; i < N; i++) { + try { + require.resolve("./link" + i + ".js"); + } catch { + failed++; + } +} +// Disarm the shim so the leak count can readlink the fds it is counting. +writeFileSync(process.env.SHIM_OFF_PATH!, ""); +let leaked = 0; +for (const name of readdirSync("/proc/self/fd")) { + try { + if (readlinkSync("/proc/self/fd/" + name).includes("${MARKER}")) leaked++; + } catch {} +} +console.log("FAILED=" + failed + " LEAKED=" + leaked); +process.exit(0); +`; + +let dir: ReturnType; +let shimPath: string; +let offPath: string; + +beforeAll(async () => { + if (!isLinux || isMusl || !cc) return; + const files: Record = { + "shim.c": SHIM_C, + "inner.ts": INNER, + "package.json": "{}", + }; + for (let i = 0; i < LINK_COUNT; i++) files[`targets${MARKER}/t${i}.js`] = `export {};\n`; + dir = tempDir("resolver-kind-fail-leak", files); + for (let i = 0; i < LINK_COUNT; i++) { + symlinkSync(join(String(dir), `targets${MARKER}`, `t${i}.js`), join(String(dir), `link${i}.js`)); + } + shimPath = join(String(dir), "shim.so"); + offPath = join(String(dir), "SHIM_OFF"); + await using ccProc = Bun.spawn({ + cmd: [cc, "-shared", "-fPIC", "-o", shimPath, join(String(dir), "shim.c"), "-ldl"], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [ccOut, ccErr, ccExit] = await Promise.all([ccProc.stdout.text(), ccProc.stderr.text(), ccProc.exited]); + if (ccExit !== 0) throw new Error(`shim compile failed: ${ccErr || ccOut}`); +}); + +afterAll(() => { + dir?.[Symbol.dispose](); +}); + +// LD_PRELOAD interposition requires a dynamic libc; the musl build links libc +// statically, so the shim cannot intercept readlink() there. +test.skipIf(!isLinux || isMusl || !cc)( + "resolver closes the symlink-target fd when get_fd_path fails after a successful open (store_fd)", + async () => { + const existing = bunEnv.LD_PRELOAD; + await using proc = Bun.spawn({ + cmd: [bunExe(), "--hot", "inner.ts"], + cwd: String(dir), + env: { + ...bunEnv, + LD_PRELOAD: existing ? `${shimPath}:${existing}` : shimPath, + SHIM_OFF_PATH: offPath, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // FAILED=0: Entry::kind swallows the error and falls back to File, so the + // resolves still succeed. If bun stops routing get_fd_path through libc + // readlink() the shim goes inert and this test measures nothing; LEAKED=0 + // is the only load-bearing assertion. + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ + stdout: `FAILED=0 LEAKED=0`, + stderr: "", + exitCode: 0, + }); + }, +);