Bun.password: report an argon2 memory cost that cannot be allocated as OutOfMemory instead of aborting - #39021
Bun.password: report an argon2 memory cost that cannot be allocated as OutOfMemory instead of aborting#39021robobun wants to merge 1 commit into
Conversation
…s OutOfMemory instead of aborting rust-argon2 allocates the block matrix infallibly, so a memoryCost the system cannot satisfy (hash options, or the m= field of a hash given to verify) aborted the process. Reserve the same amount fallibly first and report it as OutOfMemory, which Bun.password surfaces as the PASSWORD_OUT_OF_MEMORY error it produced before the Rust port.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fixed, waiting on CI. Split out of #38941.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The probe-then-allocate approach is sound and strictly additive (can only turn aborts into errors, never the reverse), but since it's a deliberate interim design choice in Bun.password — chosen over patching rust-argon2 or switching crates — a maintainer should confirm they're happy landing it this way.
What was reviewed
check_memory_is_allocatable:u32 → usizewidening +checked_mul(1024)beforetry_reserve_exact;?onbun_alloc::AllocErrorreachescrate::Error::Allocvia#[from], andError::name()returns"OutOfMemory"→PASSWORD_OUT_OF_MEMORYinpassword_error_instance.- Verify path:
m=recorded inside the existing prescan; the ulimit test'sm=4194304is exactlyMAX_VERIFY_MEMORY_COST(1 << 22), so it passes theWeakParameterscap and reaches the probe as intended. - Probe uses the global allocator, same as rust-argon2's
vec[Block; n], so ASAN'smax_allocation_size_mband mimalloc underulimit -vsee the same failure the real allocation would.
Extended reasoning...
Overview
The PR adds a fallible allocation probe (check_memory_is_allocatable) in src/runtime/crypto/pwhash.rs that reserves memoryCost * 1024 bytes via Vec::try_reserve_exact before rust-argon2's infallible vec[Block::zero(); n] runs. It is called from str_hash (using config.mem_cost) and from str_verify (using the m= value already parsed by the existing cost-cap prescan). On failure it returns Error::Alloc(AllocError), which Error::name() reports as "OutOfMemory" and password_error_instance formats as PASSWORD_OUT_OF_MEMORY — the same error the pre-port Zig implementation produced. Two new subprocess tests cover all four entry points (hash/hashSync/verify/verifySync) under ASAN's max_allocation_size_mb cap and under a Linux ulimit -v address-space limit.
Security risks
None introduced. This does not touch the argon2/bcrypt primitives, salts, or comparison logic; it only front-runs an allocation that would otherwise abort the process. If anything it hardens verify slightly against a hostile PHC string whose m= is sized to abort the process — that now becomes a catchable error. The probe cannot cause a previously-successful hash to fail: if the probe reservation succeeds it is immediately dropped, and the subsequent real allocation sees the same allocator state modulo a TOCTOU window the PR description explicitly acknowledges.
Level of scrutiny
Moderate-to-high. The Rust change is ~20 lines and mechanically simple, but it lives in Bun.password and encodes a design choice: a best-effort probe rather than patching rust-argon2 (blocked on #33153) or switching to RustCrypto's argon2 (which would change PHC codec/verify semantics). The PR description makes the case for this tradeoff thoroughly, and it also flags interactions with three other open PRs (#33865 memoryCost ceiling, #37015 node:crypto argon2, #32314 same prescan loop). That's exactly the kind of context where a maintainer should sign off on the direction, even though the implementation itself is straightforward and correct.
Other factors
I verified the error-plumbing chain end to end: bun_alloc::AllocError → crate::Error::Alloc (via #[from] at src/runtime/error.rs:419) → name() == "OutOfMemory" (line 782) → password_error_instance builds PASSWORD_OUT_OF_MEMORY. The verify test's m=4194304 sits exactly at MAX_VERIFY_MEMORY_COST so it clears the existing > limit check and reaches the new probe. The checked_mul guards 32-bit overflow. The tests follow harness conventions (bunEnv spread, concurrent pipe drain, exit-code asserted last, skipIf with stated reasons) and each has a plausible CI lane where it runs (ASAN debug for the first, Linux release for the second). No bugs found by the multi-agent hunt or by this pass; deferring purely so a human confirms the probe approach is the interim fix they want.
Problem
Bun.password.hash/hashSyncacceptmemoryCostup to 4294967295 KiB (4 TiB), andverify/verifySynctakem=from the encoded hash (the existing prescan caps it at 4 GiB).pwhash::argon2::str_hash/str_verify(src/runtime/crypto/pwhash.rs) hand the cost to rust-argon2, whoseMemory::newallocates the block matrix withvecfor both hashing and verifying). When that allocation fails the process aborts:memory allocation of N bytes failed,panic(main thread): abort() called, and a "Bun has crashed" report, for a value the caller passed in (or, for verify, for a hash string somebody else produced).sh -c 'ulimit -v 4194304; exec bun -e "Bun.password.hashSync(\"pw\", { algorithm: \"argon2id\", memoryCost: 4194304 })"'aborts withmemory allocation of 4294967296 bytes failed; a 512 MiB cost under a 1 GB limit aborts the same way, and so does the 4 TiB maximum on a machine with Linux's default overcommit setting (the request exceeds RAM plus swap, sommaprefuses it).std.crypto.pwhash.argon2took an allocator and returnederror.OutOfMemory, whichPasswordObject.zigreported like every other hash failure:Password hashing failed with error "OutOfMemory"withcode: "PASSWORD_OUT_OF_MEMORY".Fix
check_memory_is_allocatable(m)reservesm * 1024bytes withtry_reserve_exact, drops the probe, and returnsError::Allocon failure.str_hashcalls it right beforehash_encoded;str_verifyrecordsmin the prescan it already runs over the cost fields and calls it right beforeverify_encoded(the decoder only accepts strings with exactly onem=, so this is the cost it would allocate for; anything else fails to decode before allocating).Error::Allocnames itselfOutOfMemory, so the unchangedpassword_error_instanceinPasswordObject.rsproduces exactly the pre-port error, on both the sync path and the work-pool path:Password hashing failed with error "OutOfMemory"/Password verification failed with error "OutOfMemory",code: "PASSWORD_OUT_OF_MEMORY".m=of a hash string an attacker controls), and any address-space or commit limit. It deliberately changes nothing when the allocation succeeds and the machine later runs out of memory while argon2 touches the blocks (overcommit_memory=1, cgroup limits); that is the OOM killer's domain, as before.argon2, which changes the PHC codec and the verify semantics) are much larger changes than this needs. The probe is best effort by construction (another thread can take the memory between the probe and the crate's allocation, and the crate rounds the cost down to a multiple of 4 blocks), which is acceptable: it cannot make anything that works today fail, and it converts every deterministic failure into the error. It costs one untouched reservation and release per call, microseconds next to the hash itself. The function ispub(crate)so thenode:cryptoargon2 binding proposed in node:crypto: implement argon2 and argon2Sync #37015, which drives the same crate, can use the same check.memoryCostceiling forhash. Bun.password: reject argon2 cost options that verify would refuse #33865 proposes capping it at verify's 4 GiB; that is compatible with this and does not replace it, since the costs that fail to allocate in practice (64 MiB default, 4 GiB maximum) are well inside any plausible ceiling. crypto: accept argon2 PHC params in any order in Bun.password.verify #32314 edits the same prescan loop; whichever lands second has a three-line conflict.test/js/bun/util/password.test.ts: the same child script runshashSync,hash,verifySyncandverifyagainst an unallocatable cost and checks an allocatable one still works afterwards, in two variants. Under ASAN (test.skipIf(!isASAN)) withallocator_may_return_null=1:max_allocation_size_mb=32and the default 64 MiB cost: fails on main withmemory allocation of 67108864 bytes failed, passes here. On Linux release builds (test.skipIf(isASAN || !isLinux); ASAN builds cannot start under an address-space limit) withulimit -v 4194304and a 4 GiB cost, which can never fit while bun itself starts in about 0.35 GB: fails against the current release build withmemory allocation of 4294967296 bytes failed(output below); its passing side runs on CI's Linux release lanes, since the local debug build is ASAN. The rest of the file: 72 pass, 9 debug-only skips.TextEncoderStreamhalf of the same port regression), which shares no code with it.Background
mis in KiB: the algorithm fills a matrix ofmone-kibibyte blocks, allocated up front in one piece, and reads it back while hashing, so it is the dominant allocation of a hash or verify call.verifygets it from the PHC string it is given ($argon2id$v=19$m=65536,t=2,p=1$salt$hash), so it is input, not configuration.Vec::try_reserve_exactreturnsErrwhen the allocator returns null, where the crate'svec!callshandle_alloc_errorand aborts. Reserving without writing touches no pages, so the probe costs the allocator a reservation and a release and nothing else.Error::Alloc(AllocError)is the runtime crate's out-of-memory variant;Error::name()reports it asOutOfMemory, andBun.passwordbuilds both the message and thePASSWORD_*code from that name, which is also how the Zig implementation formattederror.OutOfMemory.ASAN_OPTIONS=allocator_may_return_null=1:max_allocation_size_mb=Nmakes any single allocation above N MiB return null;ulimit -v(RLIMIT_AS) makesmmapfail once the process's address space would exceed the limit, which is how mimalloc ends up returning null in the release variant.Release build, without the fix, address-space variant of the test
With the fix, all four entry points report
{"name":"Error","code":"PASSWORD_OUT_OF_MEMORY","message":"Password hashing failed with error \"OutOfMemory\""}(
verificationfor the verify pair) and a hash withmemoryCost: 8still round-trips afterwards.