Report UTF-16 to UTF-8 encode counts through size_t so exactly 2^32 written bytes doesn't wrap to 0 - #37252
Report UTF-16 to UTF-8 encode counts through size_t so exactly 2^32 written bytes doesn't wrap to 0#37252robobun wants to merge 6 commits into
Conversation
A UTF-16 source can encode to exactly 2^32 UTF-8 bytes, and JSC allows
2^32-byte ArrayBuffers, so EncodeIntoResult's u32 counts wrapped to 0
even though every byte was written: Buffer.write returned 0 and
TextEncoder.encodeInto returned {written: 0} for a full 4 GiB
destination.
EncodeIntoResult.read/written are now usize end to end, the
TextEncoder__encodeInto8/16 FFI returns the struct by value instead of
packing two u32 halves into a u64, and V8String's WriteUtf8V2 drops the
chunking workaround it carried for those packed 32-bit counts.
WalkthroughChangesThe encoding result contract now uses native-size Native encode count propagation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I grepped for other TextEncoder__encodeInto8/16 callers — all three C++ sites (JSTextEncoder.cpp, V8String.cpp ×2) are updated to the new struct-return ABI, and no other .read as usize/.written as usize casts remain. Given the FFI return-type change and the V8String chunked-encode removal, a human look is still worthwhile.
Extended reasoning...
Verified via grep that the FFI signature change is complete on both sides (Rust producer + all C++ consumers updated; #[repr(C)] on the Rust struct matches the TextEncoderEncodeIntoResult typedef in headers-handwritten.h), and that the one flagged leftover cast in fmt.rs:587 is the only remaining as usize on the widened fields. Deferring rather than approving because the change spans 16 files including an FFI ABI change (u64 packed → struct-by-value) and deletes a non-trivial chunked-encode workaround in V8String::WriteUtf8V2.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 7:19 AM PT - Aug 9th, 2026
❌ @robobun, your commit 501e796 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37252That installs a local version of the PR into your bun-37252 --bun |
There was a problem hiding this comment.
I reviewed this PR and found no bugs (the earlier redundant-cast nit was fixed in 9255fac). Because it changes an FFI ABI — TextEncoder__encodeInto8/16 now return a repr(C) struct by value instead of a packed u64 — across Rust and three C++ call sites, and removes the chunked-encode workaround in V8String::WriteUtf8V2, a human look would still be worthwhile. The comment-cop lint on the new doc comments in lib.rs/headers-handwritten.h is also still outstanding.
Checked: all TextEncoder__encodeInto call sites are covered by the diff; both <true> (STOP) callers of copy_latin1_into_utf8_stop_on_non_ascii were updated to the new usize::MAX sentinel; jsNumber(size_t) yields exact 4294967296.0 for the encodeInto result.
Extended reasoning...
Overview
Widens EncodeIntoResult.{read,written} from u32 to usize so a UTF-16→UTF-8 encode that writes exactly 2^32 bytes reports the correct count instead of wrapping to 0. 16 files: the core struct definition (bun_core/lib.rs), all producers in bun_core/string/immutable/unicode.rs, ~10 consumer files that mechanically drop as usize casts, an FFI ABI change (TextEncoder__encodeInto8/16 return the struct by value instead of packing two u32s into a u64), the C++ mirror struct in headers-handwritten.h, updates to the two C++ callers (JSTextEncoder.cpp, V8String.cpp — the latter also drops a chunked-encode workaround that existed specifically because of the 32-bit wrap), and a new memory-gated test.
Security risks
None identified. This is an integer-width fix in encoding count reporting; no auth, crypto, or untrusted-input parsing changes. The buffer contents were already correct — only the reported count was wrong.
Level of scrutiny
Medium-high. Most of the diff is mechanical (as usize cast removal, identity behavior below 2^32), but the FFI ABI change — returning a 16-byte {size_t, size_t} struct by value across the Rust↔C++ boundary on all five platform/arch combos — is the kind of change where a layout or calling-convention mismatch would silently corrupt results. The PR reports rust:check-all passing on all 10 targets and the buffer/encoding/v8 test suites passing, which is good evidence, but this is core hot-path encoding infrastructure that a maintainer should sign off on.
Other factors
- My prior review's one finding (leftover
as usizecast at fmt.rs:587) was fixed in 9255fac and the thread is resolved. - Verified all
TextEncoder__encodeIntoreferences are in the diff (5 files); no stale callers with the old u64 signature. - Verified the
usize::MAXsentinel change: only twoSTOP=truecallers exist (both inRopeStringEncoder), both updated; no lingeringu32::MAXchecks onEncodeIntoResultfields. - The comment-cop bot flagged the new 3–4 line doc comments on
EncodeIntoResultandTextEncoderEncodeIntoResult. These document the FFI mirror relationship (which REVIEW.md says to comment when ownership/layout is non-local across FFI), so they read as false positives to me — but they're outstanding and the author should either shorten them or dismiss the lint. - The
V8String::WriteUtf8V2chunked-encode deletion is a net simplification, but it removes surrogate-pair-boundary handling that the single-call path must now get right implicitly; the v8.test.ts pass gives some confidence.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier redundant-cast nit was fixed in 9255fac). Because it changes the extern "C" return ABI of TextEncoder__encodeInto8/16 (packed u64 → struct-by-value across Rust/C++) and removes the chunked-encode workaround in V8String::WriteUtf8V2, a human look would still be worthwhile.
What was reviewed:
- FFI layout:
#[repr(C)] EncodeIntoResult { usize, usize }matchesTextEncoderEncodeIntoResult { size_t, size_t }; all three C++ call sites updated, no stale declarations remain. usize::MAXsentinel: bothcopy_latin1_into_utf8_stop_on_non_ascii::<true>callers (RopeStringEncoder append8/write8) updated in lockstep with the producer.jsNumber(size_t)at 2^32 yields an exact double;WriteUtf8(int-returning) narrowing is bounded byINT_MAX.
Extended reasoning...
Overview
The PR widens EncodeIntoResult.{read,written} from u32 to usize, marks it #[repr(C)], and switches the TextEncoder__encodeInto8/16 FFI from a packed-u64 return to returning the struct by value (mirrored as TextEncoderEncodeIntoResult in headers-handwritten.h). Sixteen files are touched: two producers in bun_core, four FFI-boundary files (Rust exporter + C++ header + two C++ callers), ~10 mechanical as usize cast removals across consumers (fmt, websocket_client, PackageInstall, paths, node_process, TextDecoder/Encoder/EncoderStream, encoding, WindowsWatcher), plus a memory-gated regression test in buffer.test.js. V8String::WriteUtf8V2 also drops its chunked-encode loop, which existed only to work around the packed-u32 wrap.
Security risks
None identified. No user-controlled data flows into new trust boundaries; the change widens an integer type and adjusts an internal FFI signature. The one place that previously relied on a value fitting in 32 bits (WriteUtf8's static_cast<uint32_t>) is bounded by unsigned_length <= INT_MAX, so no new truncation is introduced.
Level of scrutiny
Moderate-to-high. The mechanical cast removals are low-risk (Rust would fail to compile on a type mismatch), but two aspects warrant a maintainer's eye: (1) the extern "C" return-type change from u64 to a 16-byte POD struct — correct on all Bun targets (SysV rax:rdx, Win64 hidden-pointer, AAPCS x0:x1) since Rust's extern "C" follows the platform C ABI, but it's an ABI-boundary change in a core encoding path and rust:check-all only type-checks, it doesn't link or run; (2) deleting the 20-line chunked-encode workaround in WriteUtf8V2 is a real behavior-preserving refactor whose correctness depends on the FFI change being right.
Other factors
- The
u32::MAX→usize::MAXsentinel change is consistent: the sole<STOP=true>producer inbun_core/lib.rsand both RopeStringEncoder consumers were updated together; no other<true>callers exist. - All prior review threads (my redundant-cast nit, the comment-cop verbosity flags) are resolved; the doc comments were trimmed in 4ed8c69.
- The new regression test is gated on
totalmem() >= 16 GiB(peak ~10.5 GiB RSS), so it will skip on most CI runners — the fix's coverage in CI depends on at least one lane having that much memory. - The PR touches
bun_corestring encoding, which is on the hot path for essentially every string→bytes operation in the runtime; that breadth is why I'm deferring rather than approving.
Pre-fix the wrapped count failed encode16's completeness check and the 2^32-byte result fell into the allocating fallback, whose ArrayBuffer length cast panics, so encode() aborted the process rather than miscounting.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes an FFI return signature (packed u64 → struct-by-value) across the Rust/C++ boundary and deletes the chunked-encode workaround in V8String::WriteUtf8V2, a human look would still be worthwhile.
What was reviewed:
EncodeIntoResultwidening: all producers/consumers updated; theu32::MAX → usize::MAXsentinel incopy_latin1_into_utf8_stop_on_non_ascii::<true>has exactly two callers, both updated.TextEncoder__encodeInto8/16callers: grepped repo-wide — only the three declared sites (JSTextEncoder.cpp, V8String.cpp ×2) exist; no stale declarations or DOMJIT paths.- ABI: Rust
#[repr(C)] { usize, usize }matches C++{ size_t, size_t }on all supported 64-bit targets;jsNumber(size_t)picks the correct overload for 2^32.
Extended reasoning...
Overview
Widens EncodeIntoResult.{read,written} from u32 to usize (now #[repr(C)]) so a UTF-16→UTF-8 encode that writes exactly 2^32 bytes reports the correct count instead of wrapping to 0. Twelve of the sixteen files are mechanical as usize cast removals (fmt.rs, unicode.rs, websocket_client.rs, PackageInstall.rs, string_paths.rs, node_process.rs, TextDecoder.rs, TextEncoderStreamEncoder.rs, encoding.rs, WindowsWatcher.rs). The substantive changes are: the struct definition in bun_core/lib.rs; TextEncoder__encodeInto8/16 now returning the struct by value instead of a packed u64 (TextEncoder.rs, headers-handwritten.h, JSTextEncoder.cpp, V8String.cpp); the sentinel u32::MAX → usize::MAX in the STOP-on-non-ASCII path; and V8String::WriteUtf8V2 dropping its chunked-encode loop that existed only to work around the 32-bit-count wrap. A memory-gated regression test is added to buffer.test.js.
Security risks
None identified. No untrusted-input parsing changed, no new allocations sized from external data, no auth/crypto/permission code touched. The change widens integer types, which strictly reduces truncation risk.
Level of scrutiny
Medium-high. The mechanical cast removals are trivial, but the FFI return-type change from u64 (packed) to a 16-byte struct-by-value crosses the Rust/C++ ABI boundary in a hot path used by Buffer.write, TextEncoder.encodeInto, and the V8 String::WriteUtf8 shim. Struct-by-value return is well-defined on SysV x86-64 / Win64 / AArch64 for a two-size_t POD, and the Rust #[repr(C)] layout matches the C++ declaration, but this is the kind of change where a second pair of eyes on the ABI contract is cheap insurance. The WriteUtf8V2 chunked-loop deletion also removes surrogate-pair-boundary handling that is now unnecessary (a single call handles the whole string), which a maintainer should confirm they're comfortable with.
Other factors
- The one nit from the earlier automated pass (leftover
as usizeat fmt.rs:587) was fixed in 9255fac, and the comment-cop doc-comment length flags were addressed in 4ed8c69; all threads are resolved. - Verified no other callers of
TextEncoder__encodeInto8/16exist (grepped repo-wide) and no other consumers of theusize::MAXsentinel exist beyond the two updated in TextEncoder.rs. - The author reports
rust:check-all(10 targets), the full buffer/encoding/v8 test suites, and a targeted regression test all pass; the test is properly gated onos.totalmem() >= 16 GiB, drains stdout/stderr concurrently, and asserts a combined object per REVIEW.md conventions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/v8/V8String.cpp`:
- Around line 242-249: Update the UTF-16 path in the V8 string encoding flow
around TextEncoder__encodeInto16 to honor kReplaceInvalidUtf8: retain
replacement-character encoding when set, but emit WTF-8 bytes for unpaired
surrogates when unset. Add coverage for both flag states, including an unpaired
U+D800 case.
In `@test/js/node/buffer.test.js`:
- Around line 4699-4704: Replace the multi-line regression explanation above the
test with a single comment containing the relevant issue URL, preserving the
regression test unchanged.
- Around line 4764-4766: Remove the explicit ten-minute timeout argument from
the resource-heavy test near the spawn/4.3 GB encode comments. Let the test use
the default test timeout, and leave any special scheduling or external timeout
handling to the repository’s resource-test harness.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc438b2e-2191-4ad9-b416-df42a46865d3
📒 Files selected for processing (16)
src/bun_core/fmt.rssrc/bun_core/lib.rssrc/bun_core/string/immutable/unicode.rssrc/http_jsc/websocket_client.rssrc/install/PackageInstall.rssrc/jsc/bindings/headers-handwritten.hsrc/jsc/bindings/v8/V8String.cppsrc/jsc/bindings/webcore/JSTextEncoder.cppsrc/paths/string_paths.rssrc/runtime/node/node_process.rssrc/runtime/webcore/TextDecoder.rssrc/runtime/webcore/TextEncoder.rssrc/runtime/webcore/TextEncoderStreamEncoder.rssrc/runtime/webcore/encoding.rssrc/watcher/WindowsWatcher.rstest/js/node/buffer.test.js
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes an FFI ABI boundary (TextEncoder__encodeInto8/16 now return a #[repr(C)] struct by value instead of a packed u64) across Rust↔C++ in a hot encoding path, a human look would still be worthwhile.
What was reviewed:
EncodeIntoResultwidening tousizeand all producer/consumer sites — theusize::MAXsentinel change incopy_latin1_into_utf8_stop_on_non_ascii::<true>has exactly two callers (both updated inRopeStringEncoder).- Struct-by-value return ABI: Rust
#[repr(C)] { usize, usize }matches C++{ size_t, size_t }; all three C++ call sites updated, no staleextern "C" size_tdeclarations remain. V8String.cppchunk-loop removal — same encoder call, surrogate handling unchanged (chunking is gone so the split-pair guard is no longer needed).jsNumber(size_t)forencodeIntoresult — 2^32 is exact as a double.
Extended reasoning...
Overview
This PR widens EncodeIntoResult.{read, written} from u32 to usize and makes the struct #[repr(C)] so TextEncoder__encodeInto8/16 can return it by value across the Rust↔C++ FFI instead of packing two u32s into a u64. The motivation is that a UTF-16→UTF-8 encode can produce exactly 2^32 bytes (JSC's max ArrayBuffer size), which wraps a u32 to 0 — causing Buffer.write/encodeInto to report 0 bytes written and TextEncoder.encode to abort. Sixteen files are touched: the core type change (lib.rs, unicode.rs), the FFI wrappers (TextEncoder.rs, headers-handwritten.h), three C++ callers (JSTextEncoder.cpp, V8String.cpp), nine mechanical as usize cast removals, and a new memory-gated regression test.
Security risks
None identified. This is a count-width fix in encoding output reporting; no input validation, auth, or trust boundary is touched. The widening is strictly more permissive on the return side and cannot introduce truncation.
Level of scrutiny
Medium-high. The mechanical cast removals are trivially safe (Rust would reject a wrong-typed slice index). The substantive risk is the FFI ABI change: extern "C" functions now return a 16-byte POD struct by value instead of a u64. This is a well-established pattern in the codebase (e.g. BunString, SystemError are passed/returned by value), the struct is #[repr(C)] with matching field types/order on both sides, and rust:check-all passed all 10 target/platform combos. But it is a hot path (TextEncoder is used everywhere), and struct-return ABI differs across SysV/Win64/AArch64, so a maintainer glance at the ABI shape is warranted.
Other factors
- The
V8String.cppWriteUtf8V2refactor removes a ~20-line chunking loop that existed solely to work around the u32 wrap. The replacement is a single call with identical semantics; the surrogate-pair-split guard is correctly dropped since there's no chunk boundary anymore. CodeRabbit raised and withdrew akReplaceInvalidUtf8concern here (pre-existing behavior, unchanged). - The
usize::MAXsentinel change forcopy_latin1_into_utf8_stop_on_non_ascii::<true>was checked: only two callers exist (RopeStringEncoder::append8/write8), both updated. - My prior nit (leftover
as usizecast at fmt.rs:587) was fixed in 9255fac; comment-cop's verbosity flags were addressed in 4ed8c69. - CodeRabbit's two remaining resolved comments (issue-URL link, per-test timeout) are style nits; the 10-minute timeout is justified by the comment (two sequential ~4 GiB encode passes under debug+ASAN) and follows the pattern of the neighboring
MAX_LENGTHtest in the same file. - Test coverage is thorough: fails-without/passes-with evidence in the description, plus full
buffer.test.js/encoding/v8.test.tsruns.
|
CI status: the diff itself is green. The remaining failures in build 90922 are unrelated to this change: test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts is a pre-existing failure also present on main (unchecked exception in JSC__JSModuleLoader__loadAndEvaluateModule on the x64-asan lane), and the other seven entries are marked flaky (each passed on retry or when run alone). The new 2^32 count test passes on the lanes with enough memory to run it, and the targeted fail-before/pass-after evidence is in the PR description. Ready for review. |
1 participant
Reproduction
Needs ~10 GiB RAM. A UTF-16 string whose UTF-8 encoding is exactly 2^32 bytes, written into a
MAX_LENGTH(2^32) buffer:For
write/encodeInto, every byte is actually written (the buffer contents are correct); only the reported count is wrong, so callers that trust it (advance offsets, slice results, loop on written) misbehave.encode()is worse: the wrapped count failsencode16's completeness check, so the already-correct fast-path result is discarded and the 2^32-byte fallback hits theu32::try_from(len).expect("int cast")inArrayBuffer::from_bytes, aborting the process.Cause
EncodeIntoResultinbun_coredeclaredread: u32, written: u32, and every producer (copy_utf16_into_utf8*,copy_latin1_into_utf8*,copy_latin1_into_utf16,copy_cp1252_into_utf16) cast itsusizecount withas u32. The C ABI wrappersBun__encoding__writeUTF16/writeLatin1take and returnsize_t, so the count silently truncated on the round-trip through the helper. Exactly 2^32 is reachable only for UTF-16 sources (Latin-1 caps at2*(2^31-1) = 2^32-2), and JSC permits ArrayBuffers of exactly 2^32 bytes.TextEncoder__encodeInto8/16additionally packedread/writteninto two u32 halves of a u64 across the FFI, soTextEncoder.encodeInto(and the V8String::WriteUtf8V2shim) could not receive 2^32 even with the helper fixed.Fix
EncodeIntoResult.read/writtenareusize(nowrepr(C)), all producers return exact counts, all consumers updated (no behavior change below 2^32).TextEncoder__encodeInto8/16return the struct by value (declared asTextEncoderEncodeIntoResultinheaders-handwritten.h) instead of the packed u64;encodeInto's result object is built from the full counts (jsNumberyields 4294967296.0 as a double, which is exact).V8String.cppWriteUtf8V2drops the chunked-encode workaround it carried specifically because the packed 32-bit counts could wrap; a single call now returns exactsize_tcounts.TextEncoder.encode()stops aborting at exactly 2^32 output bytes: with exact counts the completeness check passes and the fast path returns the already-filled array, so the panicking fallback is no longer reached for any input that fits in an ArrayBuffer.This is the second layer of the bug family from #34274 (which carried
size_tthroughBuffer.toString/writeargument handling; the returned count still wrapped). The open #37239 works around this wrap in the stream consumer concatenation and can drop that once this lands.Related but deliberately not included here: the pre-existing detached-destination case for
encodeInto(null vector from a detached view) is fixed by the open #36531, which touches the same wrappers; andArrayBuffer::from_bytes's u32 length ceiling remains for its other callers (streams/Blob), tracked separately.Verification
New test in
test/js/node/buffer.test.jsnext to the existingMAX_LENGTHtest, gated onos.totalmem() >= 16 GiB(peak RSS ~10.5 GiB per spawn, two sequential spawns), coveringBuffer.write,TextEncoder.encodeInto,Buffer.byteLength, buffer contents at the 2^32 boundary, andTextEncoder.encode(which aborted pre-fix).write_ret: 0, encode_written: 0(with correct buffer contents), and theencode()spawn aborts with the int-cast panic4294967296,encode()returns a 4 GiB Uint8Arraybun run rust:check-all: all 10 target/platform combos passtest/js/node/buffer.test.js(618 tests),test/js/web/encoding/(197 tests), andtest/v8/v8.test.ts(74 tests, exercising theWriteUtf8V2path) pass with the debug build[review] gate passed · iteration 1 · 16 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file