Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions src/bun_core/string/immutable/unicode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,11 @@ pub fn to_utf8_list_with_type_bun<const SKIP_TRAILING_REPLACEMENT: bool>(

let count: usize = replacement.utf8_width() as usize;
#[cfg(not(target_family = "wasm"))]
{
let extra = ((utf16_remaining.len() as u64 & ((1u64 << 52) - 1)) as f64 * 1.2) as usize;
list.reserve_exact((i + count + list.len() + extra).saturating_sub(list.len()));
}
let extra = ((utf16_remaining.len() as u64 & ((1u64 << 52) - 1)) as f64 * 1.2) as usize;
#[cfg(target_family = "wasm")]
{
list.reserve_exact(
(i + count + list.len() + utf16_remaining.len() + 4).saturating_sub(list.len()),
);
}
let extra = utf16_remaining.len() + 4;
list.try_reserve_exact(i + count + extra)
.map_err(|_| AllocError)?;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
append_u16_as_u8(list, to_copy);

if SKIP_TRAILING_REPLACEMENT {
Expand All @@ -174,8 +169,8 @@ pub fn to_utf8_list_with_type_bun<const SKIP_TRAILING_REPLACEMENT: bool>(
}

if !utf16_remaining.is_empty() {
let need = utf16_remaining.len() + list.len();
list.reserve_exact(need.saturating_sub(list.len()));
list.try_reserve_exact(utf16_remaining.len())
.map_err(|_| AllocError)?;
append_u16_as_u8(list, utf16_remaining);
}

Expand Down
49 changes: 28 additions & 21 deletions src/runtime/webcore/TextEncoderStreamEncoder.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use core::cell::Cell;
use core::ptr::NonNull;

use bun_collections::VecExt as _;
use bun_alloc::AllocError;

Check warning on line 4 in src/runtime/webcore/TextEncoderStreamEncoder.rs

View check run for this annotation

Claude / Claude Code Review

PR title/description still claim the argon2 fix that was split out

The PR title and roughly half the description still document the `Bun.password` argon2 fix (`check_memory_is_allocatable`, `str_hash`/`str_verify`, `Error::Alloc`, `PASSWORD_OUT_OF_MEMORY`, the `ulimit` repro, the `password.test.ts` variants), but commit 3250eb48 split that half out — the diff now touches only the three TextEncoderStream files. The title should drop "and Bun.password argon2" and the argon2 sections of the description should be removed so the PR does not overstate what it ships (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The PR title and roughly half the description still document the Bun.password argon2 fix (check_memory_is_allocatable, str_hash/str_verify, Error::Alloc, PASSWORD_OUT_OF_MEMORY, the ulimit repro, the password.test.ts variants), but commit 3250eb4 split that half out — the diff now touches only the three TextEncoderStream files. The title should drop "and Bun.password argon2" and the argon2 sections of the description should be removed so the PR does not overstate what it ships (CLAUDE.md #11).

Extended reasoning...

What the finding is

Commit 3250eb4 ("Split out the argon2 half; run the native-sink cases behind a carried lead surrogate") removed the src/runtime/crypto/pwhash.rs and test/js/bun/util/password.test.ts changes from this PR. The diff now contains exactly three files:

  • src/bun_core/string/immutable/unicode.rs
  • src/runtime/webcore/TextEncoderStreamEncoder.rs
  • test/js/web/encoding/text-encoder-stream.test.ts

However, the PR metadata was not trimmed to match:

  • Title: still reads "Throw out of memory from TextEncoderStream and Bun.password argon2 instead of aborting".
  • Description → Problem: the second bullet (Bun.password argon2: memoryCost, str_hash/str_verify, rust-argon2 Memory::new, the ulimit -v 4194304 repro) describes code that is no longer in the diff.
  • Description → Fix: the check_memory_is_allocatable bullet, the "Why a probe rather than a change to the crate" bullet, the "Not changed, deliberately: the memoryCost ceiling" bullet, the password.test.ts verification bullet, and the second <details> block ("Bun.password without the fix") all document the split-out half.
  • Description → Related open PRs: crypto: accept argon2 PHC params in any order in Bun.password.verify #32314 (verify prescan) and node:crypto: implement argon2 and argon2Sync #37015 (node:crypto argon2) are argon2-only.

Why the timeline confirms this rather than refutes it

The timeline shows comment-cop and robobun replies on src/runtime/crypto/pwhash.rs ("Cut to two lines in f41bd0d", "Keeping these two lines: … rust-argon2 allocating the matrix infallibly in Memory::new"), which confirms pwhash.rs was in scope at commit f41bd0d and removed at 3250eb4. CodeRabbit's "Files selected for processing" for the range 293b832..3250eb4 lists only the three TextEncoderStream files. So the split is deliberate and the description is simply stale.

Step-by-step proof

  1. git log --oneline on this branch: 293b832 (both halves), f41bd0d/6b0a6cf7/4602d8f7 (comment/test cleanups on both halves), then 3250eb4 ("Split out the argon2 half").
  2. <changed-files count="3"> in the PR metadata lists only unicode.rs, TextEncoderStreamEncoder.rs, and text-encoder-stream.test.ts. Neither pwhash.rs nor password.test.ts appears.
  3. <title> still contains "and Bun.password argon2".
  4. Search the description for "argon2", "pwhash", "password.test.ts", "check_memory_is_allocatable", "PASSWORD_OUT_OF_MEMORY", "ulimit -v": all present, none of it corresponds to a diff hunk.

Impact

CLAUDE.md #11: "NEVER overstate what you got done or what actually works in commits, PRs". A reader skimming the title/description (or a future git log/blame that lands on this PR) would conclude the Bun.password argon2 abort is fixed here, when it is not — that fix now lives in whatever PR the split-out half becomes. The description's "Verified" section also claims test/js/bun/util/password.test.ts (72 pass, 9 debug skips) was run for this PR, which is no longer meaningful for the current diff.

This is metadata only — merging as-is causes no runtime failure — so it is a nit, not a merge blocker.

How to fix

  • Retitle to "Throw out of memory from TextEncoderStream instead of aborting" (or similar).
  • Delete the argon2 bullets from Problem, Fix, Verified, and Related open PRs, and drop the second <details> block.
  • Optionally note that the argon2 half was split into its own PR and link it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done: the title and description were rewritten for the TextEncoderStream half right after 3250eb4 was pushed (this review ran against the pre-edit metadata); the scope bullet now links the argon2 half, #39021.

use bun_core::strings;
use bun_jsc::{JSGlobalObject, JSUint8Array, JSValue};
use bun_ptr::RawSlice;
Expand Down Expand Up @@ -30,18 +30,20 @@
return JSUint8Array::create_empty(global);
}
let mut buffer = Vec::new();
self.encode_latin1_into(input, &mut buffer);
if self.encode_latin1_into(input, &mut buffer).is_err() {
return global.throw_out_of_memory_value();
}
JSUint8Array::from_bytes(global, buffer.into())
}

fn encode_latin1_into(&self, input: &[u8], buffer: &mut Vec<u8>) {
fn encode_latin1_into(&self, input: &[u8], buffer: &mut Vec<u8>) -> Result<(), AllocError> {
bun_output::scoped_log!(
TextEncoderStreamEncoder,
"encodeLatin1: \"{}\"",
bstr::BStr::new(input)
);
if input.is_empty() {
return;
return Ok(());
}

let prepend_replacement_len: usize = if self.pending_lead_surrogate.take().is_some() {
Expand All @@ -57,7 +59,9 @@
// 278.00 ms 13.0% 278.00 ms simdutf::arm64::implementation::utf8_length_from_latin1(char const*, unsigned long) const
//
//
buffer.reserve(input.len() + prepend_replacement_len);
buffer
.try_reserve(input.len() + prepend_replacement_len)
.map_err(|_| AllocError)?;
if prepend_replacement_len > 0 {
buffer.extend_from_slice(&[0xef, 0xbf, 0xbd]);
}
Expand All @@ -75,14 +79,17 @@
remain = &remain[result.read as usize..];

if result.written == 0 && result.read == 0 {
buffer.reserve(2);
buffer.try_reserve(2).map_err(|_| AllocError)?;
} else if buffer.len() == buffer.capacity() && !remain.is_empty() {
buffer.ensure_total_capacity(buffer.len() + remain.len() + 1);
buffer
.try_reserve(remain.len() + 1)
.map_err(|_| AllocError)?;
}
}
debug_assert!(
buffer.len() == (simdutf::length::utf8::from::latin1(input) + prepend_replacement_len)
);
Ok(())
}

fn encode_utf16(&self, global: &JSGlobalObject, input: &[u16]) -> JSValue {
Expand All @@ -99,7 +106,7 @@
JSUint8Array::from_bytes(global, buf.into())
}

fn encode_utf16_into(&self, input: &[u16], buf: &mut Vec<u8>) -> Result<(), ()> {
fn encode_utf16_into(&self, input: &[u16], buf: &mut Vec<u8>) -> Result<(), AllocError> {
bun_output::scoped_log!(
TextEncoderStreamEncoder,
"encodeUTF16: \"{}\"",
Expand Down Expand Up @@ -144,7 +151,9 @@

remain = &remain[1..];
if remain.is_empty() {
buf.extend_from_slice(&sequence[0..converted.utf8_width() as usize]);
let width = converted.utf8_width() as usize;
buf.try_reserve(width).map_err(|_| AllocError)?;
buf.extend_from_slice(&sequence[0..width]);
return Ok(());
}

Expand All @@ -158,13 +167,14 @@

let length = simdutf::length::utf8::from::utf16::le(remain);

buf.reserve(
buf.try_reserve(
length
+ match prepend {
Some(pre) => pre.len as usize,
None => 0,
},
);
)
.map_err(|_| AllocError)?;

if let Some(pre) = &prepend {
buf.extend_from_slice(&pre.bytes[0..pre.len as usize]);
Expand All @@ -188,8 +198,7 @@

if result.status != simdutf::Status::SUCCESS {
// Slow path: there was invalid UTF-16, so we need to convert it without simdutf.
let lead_surrogate =
strings::to_utf8_list_with_type_bun::<true>(buf, remain).map_err(|_| ())?;
let lead_surrogate = strings::to_utf8_list_with_type_bun::<true>(buf, remain)?;
if let Some(pending_lead) = lead_surrogate {
self.pending_lead_surrogate.set(Some(pending_lead));
}
Expand Down Expand Up @@ -289,15 +298,13 @@
// (theoretical) re-entrant encode-into-sink call cannot BorrowMut-panic.
let mut buf = this.scratch.take();
buf.clear();
if str.is_16bit() {
if this
.encode_utf16_into(str.utf16_slice_aligned(), &mut buf)
.is_err()
{
return global.throw_out_of_memory_value();
}
let encoded = if str.is_16bit() {
this.encode_utf16_into(str.utf16_slice_aligned(), &mut buf)
} else {
this.encode_latin1_into(str.slice(), &mut buf);
this.encode_latin1_into(str.slice(), &mut buf)
};
if encoded.is_err() {
return global.throw_out_of_memory_value();
}
if buf.is_empty() {
this.scratch.replace(buf);
Expand Down
163 changes: 161 additions & 2 deletions test/js/web/encoding/text-encoder-stream.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "bun:test";
import { readableStreamFromArray } from "harness";
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, readableStreamFromArray } from "harness";

// META: global=window,worker
// META: script=resources/readable-stream-from-array.js
Expand Down Expand Up @@ -267,3 +267,162 @@ test("TextEncoderStream -> TextDecoderStream -> TextEncoderStream -> native HTTP
expect(out.byteLength).toBe(expected.byteLength);
expect(Buffer.compare(out, Buffer.from(expected))).toBe(0);
});

// The encoder sizes a chunk's output buffer from the chunk itself (up to three
// bytes per UTF-16 unit), so those reservations are where a TextEncoderStream
// runs out of memory. A failed one has to error the stream like any other
// transform failure, not abort the process (#33014 fixed the decoding direction
// the same way). ASAN's per-allocation cap makes the failure deterministic: the
// input strings are allocated by JSC and are not subject to it, while every
// encoder reservation above CAP_MB fails. Each input is shaped so that a
// different reservation in the encoder is the one that fails. (Bun's own startup
// needs allocations of up to about 1.5 MiB, so the cap cannot go much lower; the
// Latin-1 growth cases encode three quarters of it through the debug build's
// per-char loop, so it should not be much higher either.)
describe.skipIf(!isASAN)("a failed output buffer allocation errors the stream instead of aborting", () => {
const CAP_MB = 4;
const env = {
...bunEnv,
// detect_leaks=0: natives owned only by a JSC cell are invisible to
// LeakSanitizer's reachability scan and would be reported at exit.
ASAN_OPTIONS: [
bunEnv.ASAN_OPTIONS,
"allocator_may_return_null=1",
`max_allocation_size_mb=${CAP_MB}`,
"detect_leaks=0",
]
.filter(Boolean)
.join(":"),
};
const prelude = /* js */ `
const MiB = 1024 * 1024;
const inputs = {
// Latin-1: the up-front reservation (one byte per input byte) is above the cap.
latin1: () => Buffer.alloc(${2 * CAP_MB} * MiB, "a").toString("latin1"),
// Latin-1: the up-front reservation (3/4 of the cap) fits, but every byte
// encodes to two, so the buffer fills up half way through and the reservation
// growing it (to at least 1.5x the input) fails.
latin1Grow: () => Buffer.alloc(${0.75 * CAP_MB} * MiB, 0xe9).toString("latin1"),
// Latin-1: an ASCII byte followed by an odd number of two-byte chars leaves a
// single spare byte, so a pass encodes nothing and the branch reserving room
// for one more char is the one that fails.
latin1Stuck: () => {
const bytes = Buffer.alloc(${0.75 * CAP_MB} * MiB + 2, 0xe9);
bytes[0] = 0x61;
return bytes.toString("latin1");
},
// UTF-16 fast path: simdutf predicts three bytes per unit, 1.5x the cap.
utf16: () => Buffer.alloc(${CAP_MB} * MiB, "\\u65e5", "utf16le").toString("utf16le"),
// UTF-16 slow path, N ASCII units behind a lone surrogate (the surrogate
// makes the concatenation a 16-bit string): simdutf's prediction (N + 2
// bytes) is reserved fine, then the surrogate hands the chunk to the
// replacement encoder, whose first reservation (1.2 bytes per remaining
// unit, 1.2N + 3) does not fit.
utf16Invalid: () => "\\ud800" + Buffer.alloc(${CAP_MB - 0.25} * MiB, "a").toString("latin1"),
};
const describeError = e => ({ name: e.name, message: e.message });
const SMALL = "ok\\u00e9";
const LEADING = ${JSON.stringify(leading)};
`;
const outOfMemory = { name: "RangeError", message: "Out of memory" };
const smallEncoded = [0x6f, 0x6b, 0xc3, 0xa9];

async function runChild(script: string) {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", prelude + script],
env,
stdout: "pipe",
// ASAN prints a "failed to allocate" warning for every refused allocation;
// drain it, don't assert on it.
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
const result = JSON.parse(stdout.trim() || JSON.stringify({ stdout, stderr, exitCode }));
return { result, exitCode };
}

test.concurrent("output enqueued to JS: the write rejects and the readable errors", async () => {
const { result, exitCode } = await runChild(/* js */ `
const results = {};
for (const [name, makeInput] of Object.entries(inputs)) {
const stream = new TextEncoderStream();
const reader = stream.readable.getReader();
const writer = stream.writable.getWriter();
const read = reader.read();
results[name] = {
write: await writer.write(makeInput()).then(() => "resolved", describeError),
read: await read.then(chunk => "resolved with " + chunk.value?.length + " bytes", describeError),
};
}
// Encoding still works in this process afterwards.
const stream = new TextEncoderStream();
const writer = stream.writable.getWriter();
writer.write(SMALL);
writer.close();
results.afterwards = Array.from(await new Response(stream.readable).bytes());
console.log(JSON.stringify(results));
`);
expect(result).toEqual({
latin1: { write: outOfMemory, read: outOfMemory },
latin1Grow: { write: outOfMemory, read: outOfMemory },
latin1Stuck: { write: outOfMemory, read: outOfMemory },
utf16: { write: outOfMemory, read: outOfMemory },
utf16Invalid: { write: outOfMemory, read: outOfMemory },
afterwards: smallEncoded,
});
expect(exitCode).toBe(0);
});

// With a native sink attached (Bun.serve's response sink here) the encoder
// writes into its own buffer and hands that to the sink instead of enqueueing
// Uint8Arrays: a separate entry point into the same encoders. Each chunk is
// preceded by a dangling lead surrogate, so the output has to be assembled in
// the encoder's buffer (the replacement goes in front of the chunk's bytes);
// that is the case the sink path owns even if plain chunks are ever handed to
// the sink directly (#36877), and it also covers the prepend variant of every
// reservation. The errored transform cancels the stream piped into it, so the
// source's cancel reason is where the error shows up.
test.concurrent("output written to a native sink: the transform errors and the server keeps serving", async () => {
const { result, exitCode } = await runChild(/* js */ `
const cancelReasons = {};
const server = Bun.serve({
port: 0,
fetch(req) {
const name = new URL(req.url).pathname.slice(1);
const { promise, resolve } = Promise.withResolvers();
cancelReasons[name] = promise;
const source = new ReadableStream({
start(controller) {
controller.enqueue(LEADING);
if (name in inputs) {
controller.enqueue(inputs[name]());
} else {
controller.enqueue(SMALL);
controller.close();
}
},
cancel: reason => resolve(describeError(reason)),
});
return new Response(source.pipeThrough(new TextEncoderStream()));
},
});
const results = {};
for (const name of Object.keys(inputs)) {
await fetch(new URL(name, server.url)).then(response => response.arrayBuffer(), () => {});
results[name] = await cancelReasons[name];
}
results.afterwards = Array.from(await (await fetch(new URL("small", server.url))).bytes());
server.stop(true);
console.log(JSON.stringify(results));
`);
expect(result).toEqual({
latin1: outOfMemory,
latin1Grow: outOfMemory,
latin1Stuck: outOfMemory,
utf16: outOfMemory,
utf16Invalid: outOfMemory,
afterwards: replacementEncoded.concat(smallEncoded),
});
expect(exitCode).toBe(0);
});
});
Loading