Repository navigation
Bun.password: report an argon2 memory cost that cannot be allocated as OutOfMemory instead of aborting #39021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,19 @@ pub(crate) mod argon2 { | |
| #[derive(Copy, Clone, Default)] | ||
| pub(crate) struct VerifyOptions; | ||
|
|
||
| /// rust-argon2 allocates its `m` KiB block matrix infallibly (`Memory::new`), so | ||
| /// reserve that much fallibly first and report `OutOfMemory` instead of aborting. | ||
| pub(crate) fn check_memory_is_allocatable(mem_cost_kib: u32) -> Result<(), Error> { | ||
| let bytes = (mem_cost_kib as usize) | ||
| .checked_mul(1024) | ||
| .ok_or(bun_alloc::AllocError)?; | ||
| let mut probe: Vec<u8> = Vec::new(); | ||
| probe | ||
| .try_reserve_exact(bytes) | ||
| .map_err(|_| bun_alloc::AllocError)?; | ||
|
Comment on lines
+105
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: pre-existing: on Linux with the default overcommit setting, a memoryCost larger than RAM plus swap still gets the process OOM-killed instead of the promised "OutOfMemory" error. The probe at src/runtime/crypto/pwhash.rs:105-108 goes through the mimalloc global allocator, and upstream mimalloc mmaps with MAP_NORESERVE whenever /proc/sys/vm/overcommit_memory is 0 or 1, which makes the kernel skip the heuristic check, so the reservation succeeds and rust-argon2 then fills the matrix until the OOM killer fires. Fix: make the probe fail for any cost that cannot be backed on Linux, e.g. probe with a direct mmap without MAP_NORESERVE, on both the hash and verify paths. Why this was flaggedA user on a stock Linux box (vm.overcommit_memory=0, e.g. 16 GiB RAM, no swap) calls Bun.password.hashSync("pw", { algorithm: "argon2id", memoryCost: 67108864 }) meaning 64 MiB but passing bytes, which argon2 reads as 64 GiB. src/runtime/crypto/PasswordObject.rs:137 accepts any integer up to 4294967295, so str_hash reaches check_memory_is_allocatable at src/runtime/crypto/pwhash.rs:168. The probe's try_reserve_exact at pwhash.rs:107 calls bun_alloc::Mimalloc::alloc (src/bun_alloc/lib.rs:536), i.e. mi_malloc; mimalloc's unix prim adds MAP_NORESERVE to every mmap when it detects overcommit_memory 0 or 1, and the kernel only applies the mode-0 heuristic to accountable mappings, so the 64 GiB reservation succeeds and the probe returns Ok. vendor::hash_encoded at pwhash.rs:169 then allocates the same 64 GiB and writes every block, and the process is SIGKILLed by the OOM killer with no JS error and no crash report. This is what the base branch does too, so the change fixes only the RLIMIT_AS and Windows-commit cases. Verification: pre-existing (the base already OOM-kills by the same route). The trigger is a non-ASAN Linux build with vm.overcommit_memory=0 and a memoryCost larger than RAM+swap. src/runtime/crypto/pwhash.rs:105-108 probes with try_reserve_exact through bun_alloc::Mimalloc, and upstream mimalloc's unix_mmap adds MAP_NORESERVE, so the reservation succeeds and the process is OOM-killed while the blocks are written. |
||
| Ok(()) | ||
| } | ||
|
Comment on lines
+101
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: pre-existing: callers of node:crypto's crypto.argon2 / argon2Sync still abort the process with "memory allocation of N bytes failed" when the block matrix cannot be allocated, the exact failure this PR fixes for Bun.password. The sibling site src/runtime/node/node_crypto_binding.rs:1459 drives the same crate's hash_raw infallibly, guarded only by synthetic_allocation_limit (default u32::MAX bytes), so any memory below 4 GiB that mmap refuses still crashes. Fix: call check_memory_is_allocatable(self.memory) before hash_raw at node_crypto_binding.rs:1459 and route failure into the existing Why this was flaggedA user calls crypto.argon2Sync("argon2id", { message, nonce, memory: 4194303, ... }) from node:crypto (src/js/node/crypto.ts:368/389) in a process whose address space or commit is limited, e.g. under ulimit -v 4194304 as in the PR's own repro. node_crypto_binding.rs:1421-1423 only pre-fails when memory*1024 exceeds synthetic_allocation_limit(), which defaults to u32::MAX (src/jsc/VirtualMachine.rs:44-45), so 4 GiB passes the check. node_crypto_binding.rs:1459 then calls rust_argon2::hash_raw, whose Memory::new does vec![Block::zero(); n] infallibly; the mmap fails and Rust's handle_alloc_error aborts the process with "memory allocation of 4294967296 bytes failed". On the base branch this aborts identically, so it is pre-existing; but the PR adds a pub(crate) probe explicitly meant for this binding while stating the binding does not exist yet, leaving the same crate driven by two callers with different failure behavior. Verification: src/runtime/node/node_crypto_binding.rs:1421-1423 is the only pre-check, and |
||
|
|
||
| fn map_err(e: &vendor::Error) -> Error { | ||
| use vendor::Error as E; | ||
| match e { | ||
|
|
@@ -152,6 +165,7 @@ pub(crate) mod argon2 { | |
| version: vendor::Version::Version13, | ||
| }; | ||
|
|
||
| check_memory_is_allocatable(config.mem_cost)?; | ||
| let encoded = vendor::hash_encoded(password, &salt, &config).map_err(|e| map_err(&e))?; | ||
| let bytes = encoded.as_bytes(); | ||
|
|
||
|
|
@@ -211,6 +225,7 @@ pub(crate) mod argon2 { | |
| } | ||
| }; | ||
|
|
||
| let mut memory_cost: Option<u32> = None; | ||
| if let Some(after_dollar) = normalised.as_bytes().strip_prefix(b"$") { | ||
| if let Some(sep) = strings::index_of_char_usize(after_dollar, b'$') { | ||
| let mut rest = &after_dollar[sep + 1..]; | ||
|
|
@@ -244,9 +259,16 @@ pub(crate) mod argon2 { | |
| if value > limit { | ||
| return Err(crate::Error::WeakParameters); | ||
| } | ||
| if key == b"m" { | ||
| memory_cost = Some(value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Strings without exactly one `m=` fail to decode below, before allocating. | ||
| if let Some(memory_cost) = memory_cost { | ||
| check_memory_is_allocatable(memory_cost)?; | ||
|
Comment on lines
+268
to
+270
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Callers verifying a malformed argon2 string on a memory-constrained host now get "OutOfMemory" where the base returned "InvalidEncoding", and every verify now attempts a reservation before the string is validated. At src/runtime/crypto/pwhash.rs:270 the probe runs on the prescanned Why this was flaggedInput: any verify string with a parseable Verification: The prescan records |
||
| } | ||
|
|
||
| match vendor::verify_encoded(&normalised, password) { | ||
| Ok(true) => Ok(()), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code