[Linux] Prefer getrandom(2) over /dev/urandom; keep /proc/self/statm open - #394
[Linux] Prefer getrandom(2) over /dev/urandom; keep /proc/self/statm open#394dylan-conway wants to merge 1 commit into
Conversation
…open Two small changes so a JSC embedder can start inside a filesystem sandbox that hides /dev and /proc without crashing, and with fewer syscalls on a GC path. RandomDevice / bmalloc CryptoRandom: On Linux, probe getrandom(GRND_NONBLOCK) once. If the pool is already initialized, use getrandom for all subsequent reads and never open /dev/urandom (no path access, no fd held for the process lifetime). If the probe returns anything else -- EAGAIN (pool not yet ready, where /dev/urandom would have returned without blocking), ENOSYS (kernel < 3.17) or EPERM (seccomp) -- fall back to the existing /dev/urandom code unchanged, so behaviour is identical wherever getrandom is not a strict improvement. The urandom fd is now opened O_CLOEXEC. currentProcessMemoryStatus(): This is reached from Heap::proportionalHeapSize via memoryFootprint() after collections. Open /proc/self/statm once and pread() it instead of fopen/fgets/fclose per call, matching what LinuxMemory in AvailableMemory.cpp already does for memoryStatus(). If the open fails the function keeps returning zeros as before, but now costs nothing on later calls. As with LinuxMemory, the cached fd is bound to the opening pid; JSCOnly never runs JSC in a fork()-without-exec child.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesLinux entropy source selection
Process memory status collection
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies the cryptographic randomness sources in both WTF and bmalloc (and the author notes it wasn't built locally), a human look would still be worthwhile.
What was reviewed:
- Verified
m_fddefaults to-1in RandomDevice.h so the early-return path is sound; destructor now guardsclose(). - Checked the getrandom probe/fallback: EAGAIN/ENOSYS/EPERM all fall through to the unchanged
/dev/urandompath, and the read loop's existing EINTR/EAGAIN handling covers the blockinggetrandomcall. - Confirmed the
statmopen-once +preadpattern matches the existingLinuxMemoryprecedent inAvailableMemory.cpp; buffer is null-terminated with a byte reserved.
Extended reasoning...
Overview
Three Linux-only changes: (1) WTF::RandomDevice and (2) bmalloc::ARC4RandomNumberGenerator::stir now probe getrandom(2) with GRND_NONBLOCK at init and use it for all subsequent reads if the kernel entropy pool is initialized, falling back to the existing /dev/urandom path otherwise. (3) currentProcessMemoryStatus() switches from per-call fopen/fgets/fclose to a once-opened fd + pread, mirroring LinuxMemory::footprint() in AvailableMemory.cpp. All open() calls gain O_CLOEXEC.
Security risks
The first two changes touch the seeding path for cryptographic randomness — RandomDevice backs WTF::cryptographicallyRandomValues() and ARC4RandomNumberGenerator seeds bmalloc's CSPRNG. getrandom(2) without GRND_RANDOM draws from the same pool as /dev/urandom, so entropy quality is unchanged. The probe only commits to getrandom when it successfully returns 1 byte (pool initialized), and the pool cannot become uninitialized afterward, so subsequent blocking getrandom calls won't hang. The fallback path is byte-identical to the old code plus O_CLOEXEC. I don't see a way this weakens randomness, but any change to a CSPRNG seed source deserves a second set of eyes.
Level of scrutiny
High — this is security-critical infrastructure (crypto RNG seeding) even though the diff is small and mechanically straightforward. The author also notes "Not built locally", so CI is the first compile check.
Other factors
The statm change is a low-risk performance/robustness improvement with direct upstream precedent in the same tree. The fork-without-exec caveat is explicitly called out and matches what upstream already accepts for memoryStatus(). The #if guarding is careful (SYS_getrandom presence checked, GRND_NONBLOCK fallback-defined), and non-Linux platforms are unaffected by preprocessor construction.
Preview Builds
|
1 participant
Two small Linux-only changes so JSC can start inside a filesystem sandbox that hides
/devand/procwithout crashing, and does less work on a GC path. No behaviour change on macOS/Windows/FreeBSD.WTF::RandomDeviceandbmalloc::ARC4RandomNumberGenerator::stirToday both open
/dev/urandomat startup andCRASH()/RELEASE_BASSERTif that fails, then keep the fd for the life of the process.Now, on Linux, each probes
getrandom(…, GRND_NONBLOCK)once:getrandomfor every subsequent read. No path is touched and no fd is held.EAGAIN(pool not yet initialized, where a/dev/urandomread would have returned immediately),ENOSYS(kernel < 3.17),EPERM(seccomp) → fall through to the existing/dev/urandomcode, unchanged.So the only observable difference is on systems where
getrandomalready works and would return the same bytes from the same pool: one fewer open file and no dependency on/dev. The fallbackopen()gainsO_CLOEXEC.WTF::currentProcessMemoryStatus()Reached from
Heap::proportionalHeapSize→memoryFootprint()(becauseUSE(BUN_JSC_ADDITIONS)turns onUSE_MEMORY_FOOTPRINT_API), i.e. after collections. It didfopen("/proc/self/statm")+fgets+fcloseon every call. It now opens the file once (O_RDONLY|O_CLOEXEC) andpread()s at offset 0, which is exactly whatLinuxMemoryinAvailableMemory.cppalready does formemoryStatus(). If the open fails it latches and keeps returning zeros as before, at zero cost.Same caveat as
LinuxMemory: the cached fd is bound to the pid that opened it, so afork()-without-execchild would read the parent's numbers. JSCOnly never runs JSC in such a child (every fork is followed by exec), and upstream already accepted this trade-off formemoryStatus()on the same path.Not built locally.