Repository navigation
[Linux] Read OS randomness with getrandom(2) and open /dev/urandom only when it fails - #749
Conversation
|
Preview build of 1fd7048: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the short-return handling: in RandomDevice.cpp the getrandom result feeds the existing accumulate loop (fine for requests over 256 bytes), and in CryptoRandom.cpp a short getrandom return falls through to the /dev/urandom loop, which overwrites the whole 128-byte buffer, so no partially filled buffer is consumed; static_assert(sizeof(randomness) <= 256) matches length = sizeof(randomness).
Extended reasoning...
The change swaps the Linux entropy source in WTF::RandomDevice and bmalloc's ARC4 generator from an always-open /dev/urandom descriptor to a raw SYS_getrandom syscall with /dev/urandom as fallback, adding O_CLOEXEC to the fallback opens. It touches the cryptographic randomness path used by Math.random, UUIDs and Gigacage, and Source/WTF and Source/bmalloc are CODEOWNERS-listed; two confirmed findings (post-startup seccomp denial now crashes, and unguarded SYS_getrandom on old kernel headers) are posted inline, so approval is not appropriate.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
Source/WTF/wtf/RandomDevice.cpp— Bun processes that restrict getrandom after startup now abort on their next random draw, where the base branch kept running. The probe at RandomDevice.cpp:101 succeeds once and drops the descriptor; every later call goes through syscall(SYS_getrandom, ..., 0) at RandomDevice.cpp:137 with no fallback. If that returns EPERM or ENOSYS (a seccomp filter installed via bun:ffi, a napi sandbox addon, or a hardening library after the VM is up) the check at RandomDevice.cpp:145 calls crashUnableToReadFromURandom(). Fix: on a non-EAGAIN/EINTR error from getrandom, lazily open /dev/urandom and continue the loop instead of crashing; apply the same to CryptoRandom.cpp:132.Why this was flagged
Trigger: any code that installs a seccomp/BPF filter denying getrandom after the static RandomDevice in OSRandomSource.cpp:36 was constructed. Bun exposes prctl/seccomp to plain JS through bun:ffi, and npm sandboxing addons do the same, so this is reachable without patching Bun. On base, RandomDevice held an open /dev/urandom fd and read() from it was unaffected by a getrandom filter, so Math.random reseeds, crypto.randomUUID and randomBytes kept working. After this change m_fd stays -1 (RandomDevice.h:54), the loop at RandomDevice.cpp:137 issues syscall(SYS_getrandom, ..., 0), gets -1 with errno EPERM (or ENOSYS for SCMP_ACT_ERRNO(ENOSYS)), and RandomDevice.cpp:145-146 crashes the process. The author lists this as accepted, but the consequence is a hard CRASH() in a long-running server for a filter that base tolerated, and the remedy is cheap: on an unexpected errno fall back to opening /dev/urandom (O_CLOEXEC) and reading it, only crashing if that open also fails.
Verification: In Source/WTF/wtf/RandomDevice.cpp the constructor probes once at line 101, leaving
m_fd == -1. Every subsequent draw then takes the line 136-137 branch; on -1 the check at lines 144-146 tolerates only EAGAIN/EINTR, so EPERM/ENOSYS from a later filter callscrashUnableToReadFromURandom(). On the base branch reads went throughread(m_fd, ...), which a getrandom filter does not affect, so the process kept running.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughLinux builds now use ChangesRandom source integration
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change reads Linux randomness through getrandom and opens /dev/urandom only when the syscall fails. That should stop startup aborts when /dev cannot be opened or file descriptors are exhausted. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed explanation, rationale, implementation summary, behavior changes, and testing results. However, it does not include the required Bugzilla issue link, Reviewed by NOBODY (OOPS!), or the template-formatted changed-file and function list. Resolution Add the associated Bugzilla URL, include the required review line, and provide the changed-file and function list in the repository template format. Ensure the pull request is linked to the Bugzilla issue and has the required labels and assignment.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
… when it fails at construction
|
On the finding about a filter that starts denying |
There was a problem hiding this comment.
I reviewed the head commit and didn't find any bugs; the #if defined(SYS_getrandom) guard added in f926720 covers the compile concern from the earlier round. Since this changes the process's OS entropy source in both WTF and bmalloc, a human look would still be worthwhile.
What was reviewed:
- The WTF getrandom loop terminates on a 0 or non-EINTR failure and carries the partial count into the unchanged read() loop, so the fallback fills only the remainder.
- The lazy
openURandom()has a single caller (ARC4RandomNumberGenerator::stir()viaOSRandomSource.cpp), which runs underm_lock, so the unsynchronizedm_fd < 0check is not racy in the tree as it stands. - In bmalloc,
sizeof(randomness)andlengthare the same 128-byte array, so the<= 256whole-or-fail assumption holds, and the/dev/urandomcall_oncepath is byte-for-byte the previous code plusO_CLOEXEC. SYS_getrandomis provided by glibc, musl and bionic<sys/syscall.h>;syscallcomes from the already-included<unistd.h>.
Extended reasoning...
The diff touches Source/WTF/wtf/RandomDevice.cpp/.h and Source/bmalloc/bmalloc/CryptoRandom.cpp, replacing the eager /dev/urandom open on Linux with getrandom(2) via raw syscall and a lazy /dev/urandom fallback. This is the OS entropy source feeding WTF's and bmalloc's ARC4 generators (Math.random, crypto seeds, Gigacage randomization), which is security-sensitive crypto-adjacent code that the approval guidelines say not to auto-approve. Both directories are listed under jsc-reviewers in .github/CODEOWNERS. The earlier inline finding about unguarded SYS_getrandom was addressed by a later commit, the only RandomDevice caller is lock-serialized, and no new defects were found, so a defer with the reviewed areas is the right outcome rather than approve.
…44281) ### What does this PR do? Fixes a segfault at address `0x10` in `JSC::JSModuleLoader::loadModule` ← `moduleLoadTopSettled` (Sentry BUN-4NFS, BUN-41VH), on every platform. The fix is in oven-sh/WebKit#748 (merged), which also explains why it belongs there. This PR bumps WebKit to it and adds the tests. Two things ride along: - **The WebKit bump goes to `fb1167ebf2cb`, the current tip.** Besides the fix (`0e8e9c238e91`) that brings in one more commit, oven-sh/WebKit#749: on Linux, OS randomness is read with `getrandom(2)`, and `/dev/urandom` is opened only when that fails. - **A type error on main.** `src/js/bun/sql.ts(892,26): error TS2339: Property 'command' does not exist on type '{}'` came in with #35119 and fails "Lint JavaScript" for every PR that includes it. `unsafeQueryFromTransaction()` now says what its query resolves to (`SQLResultArray`). It is a type argument only, so the code that runs is the same. **Cause** The module loader has two tables of what is loaded: the registry, and a shortcut that lets a repeated `import()` skip resolving. Removing a module clears both. But an `import()` of that module that is still in flight adds it to the shortcut afterwards, when its dependencies have loaded. The next `import()` finds the old module there, pairs it with a new registry entry that has no load promise yet, and dereferences null. ```ts const inFlight = import("./a"); // a.ts is registered, its dependencies are still loading mock.module("./a", () => ({ a: "mocked-a" })); await inFlight; await import("./a"); // segfault ``` Everything that removes a module while its `import()` is in flight gets there: | route | 1.4.2 | canary `11c41c645` | with the fix | |---|---|---|---| | `delete require.cache[path]` | segfault | ok (#40123) | ok | | `mock.module()` | segfault | segfault | ok | | `build.module()` in a plugin | segfault | segfault | ok | | a `bun --hot` reload | segfault | segfault | ok | | a plugin's `onResolve` redirecting a → b → c → d, `delete require.cache[d]`, `import(a)` again | segfault | segfault | ok | The last one needs nothing in flight: the shortcut is keyed by the name that was asked for, and removing a module cleared it only by the name it resolved to. It was found in review, and nothing in the crash reports points at it. Of the 481 reports, 273 are from `bun test`, 203 from `bun run` or `bun <file>`, and 240 had an HTTP server running. **Fix** (oven-sh/WebKit#748) The place that adds a module to the shortcut first checks that the registry still holds it, and skips the write if not. And removing a module clears it from the shortcut under whatever name it was asked for. It is eleven lines in Bun's part of the loader. For a program that removes no module nothing changes. ### How did you verify your code works? Four new tests, one for each route that still crashes. All four fail on 1.4.2 and on canary `11c41c645`, and pass against oven-sh/WebKit#748 on debug and release builds (Linux x64). Without the WebKit bump, CI had all three segfault at `0x10` on every platform: macOS, Linux glibc and musl, and Windows, x64 and arm64. The hot reload test passed 10 runs of 10. | test | file | |---|---| | mock.module() of a module whose import() is still loading its dependencies | `mock-module.test.ts` | | build.module() of a module whose import() is still loading its dependencies | `plugins.test.ts` | | should import a module again after a hot reload while its import() was still loading its dependencies | `hot.test.ts` | | import() after delete require.cache of a module that onResolve redirected a resolved path to | `plugins.test.ts` | The WebKit PR has the evidence that nothing changes for a program that removes no module, the comparison of behavior with main, WebKit's own module tests and the cost.
1 participant
On Linux, JSC reads OS randomness with
getrandom(2)and opens/dev/urandomonly when the syscall fails. Replaces #394.Why
WTF::RandomDeviceopens/dev/urandomin its constructor andCRASH()es if the open fails. It is constructed during VM creation, so Bun aborts before running any JS when:/dev/urandomis not there (a filesystem sandbox, a minimal container root), orulimit -n 5aborts Bun 1.4.2).bmalloc::ARC4RandomNumberGenerator::stir()has the same open behind aRELEASE_BASSERT.Why here and not in Bun
Both pieces are upstream code. Upstream's own Linux sandbox mounts a
/dev(BubblewrapLauncher.cpppasses--dev /dev), so the crash is not reachable there. Bun is a CLI that gets started inside whatever sandbox, container or rlimit the user has.RandomDeviceis private to WTF and there is no hook for an embedder to supply entropy, so nothing on the Bun side can avoid the open.What changes
Both places try
getrandom(GRND_NONBLOCK)on every read, and whenever it fails they run the code that is on main today: open/dev/urandomonce, crash if that fails, read from it. That coversEAGAIN(pool not initialized, where/dev/urandomdoes not block),ENOSYS, andEPERMfrom a seccomp filter, including one installed after startup.WTF::RandomDevice: on Linux the constructor no longer opens anything.cryptographicallyRandomValues()fills the buffer withgetrandom, resuming after short counts. If a call fails,/dev/urandomis opened if it is not open yet, and the existing read loop, which is unchanged, fills the rest. The open needs no synchronization of its own: the device's one caller,ARC4RandomNumberGenerator::stir()inCryptographicallyRandomNumber.cpp, isWTF_REQUIRES_LOCK(m_lock), so reads are already serialized.bmalloc::ARC4RandomNumberGenerator::stir()needs 128 bytes, andgetrandomfills a request of up to 256 bytes whole or fails, so it needs no loop and no new state.Both fallback opens gain
O_CLOEXEC.syscall(SYS_getrandom, …)rather than the libc wrapper, which needs glibc 2.25. Thegetrandomblocks compile only where the headers defineSYS_getrandom(kernel headers 3.17 and later); elsewhere the files build to the/dev/urandomcode alone.The bmalloc generator's only caller is Gigacage, which is compiled out under
USE_MIMALLOC. So that half is live in the debug and ASAN builds and is not linked into a release Bun at all. It is included so those builds behave the same as release under a sandbox.Behaviour changes
getrandomworking: same kernel pool, one fewer open descriptor (two fewer in debug/ASAN builds), so later descriptor numbers shift down.getrandomfailing, at startup or later: main's/dev/urandomcode, with the open at the first failure instead of at construction, plusO_CLOEXEC.O_CLOEXEConly. macOS and Windows: none.getrandomis not a new dependency for the process: Bun 1.4.2 already makes 4getrandomcalls before JSC opens/dev/urandom.What #394 had that this does not
Options::useGetrandom, withRandomDevice::setUseGetrandomandbmalloc::setCryptoRandomUsesGetrandom. With it set to false the releasejscfrom [Linux] Prefer getrandom(2) over /dev/urandom; keep /proc/self/statm open #394 still makes 11getrandomcalls from other components, and a failinggetrandomalready falls back on its own, so there is no case it rescues. The setter also constructs bmalloc's generator on every platform, including release builds that otherwise never link it./proc/self/statmdescriptor incurrentProcessMemoryStatus().LinuxMemoryinAvailableMemory.cppalready holds that file open from startup and every collection already reads it, so this was a second descriptor for the same RSS field.Heap::proportionalHeapSizeonly reaches it when RAM is underheapGrowthFunctionThresholdInMB(16 GB). Measured: 1.3 µs forfopen+fgets+fcloseagainst 0.43 µs forpread, where the cheapest full collection measured was 180 µs. It also turned one failed open into no footprint reading for the life of the process. It is unrelated to sandbox startup: that function never crashed.Hiding
/procstill aborts, inStackBounds. That is #556.Testing
Linux x64. Bun at
bf42a525d59built against this branch, as a release build (mimalloc) and a debug build (ASAN, libpas)./devhidden withbwrap --bind / / --tmpfs /dev.getrandomfailures forced with a seccomp filter that returns the errno./dev/urandomopens on a normal start/devhidden/devhidden, 8 Workersulimit -n 5ulimit -n 4getrandom→ENOSYS/EPERM/EAGAIN/dev/urandomO_CLOEXECO_CLOEXECgetrandom→EPERMfrom a filter installed after startup, then a forced reseed/dev/urandomat the reseedgetrandom→EPERMand/devhiddenMath.random()differs between runs, on either sourceThe late filter is installed from JS with
bun:ffi(prctl(PR_SET_SECCOMP)), followed by 450,000new ShadowRealm()to draw the 1.6 MB that makes WTF's generator reseed. The first commit of this PR, which chose the source once in the constructor, aborts in that test. All results here are from the head commit,1fd7048dd007.A harness linked against the built
RandomDevice.cpp.o, 3 runs, all passing:getrandomare uniform and open nothing; an empty buffer returns.SIGALRMevery 50 µs withoutSA_RESTART: all 40 first calls returned a short count and were resumed, every fill complete,/dev/urandomnot opened.getrandomnever returnedEINTRhere, so that branch is not exercised./dev/urandomopened exactly once,O_CLOEXEC. With the lock removed, which no caller does, 10 runs opened between 1 and 10 descriptors and every fill was still uniform.The
SYS_getrandomguard, checked with a shadowsys/syscall.hthat includes the real one and undefines the macro: both files compile with-Werrorwith and without it, in the release and debug configurations, and the commit before the guard fails there withuse of undeclared identifier 'SYS_getrandom'. Built without the macro, the objects have no reference tosyscall, and the same harness passes 3 runs with/dev/urandomopened once at the first read. The Bun builds tested here are built with it.Bun test files run on the release build:
spawn,spawn-stdin-pipe-fd-leak,spawn-pipe-start-error,fs-open-cloexec,fs,worker,web-globals,randomUUIDv7,crypto-random,shell/leak,bun-jsc,socket,blob-write. Six of them also on the debug build. All pass exceptfs.test.ts"mkdtempSync() empty name", which creates a directory directly under/and getsEACCESas a non-root user. On an earlier commit's debug build oneworker.test.tstest in "terminate() races and lifecycle edges" failed once on a loaded machine and the file passed on rerun; over 10 interleaved runs of that group that build passed 10, and a debug build without the change passed 9 (a 5 s timeout). It passes on the head commit.Not run locally: musl, arm64, Android, macOS, Windows, FreeBSD. The Linux includes are the ones #394 already compiled in CI.
No test in this repo can cover it. The Bun PR that picks this up can assert that no
/proc/self/fd/*link is/dev/urandom(1 on a release build today, 2 on debug, 0 with this).