plugin: preserve raw bytes when onLoad/build.module returns an ArrayBufferView - #35729
plugin: preserve raw bytes when onLoad/build.module returns an ArrayBufferView#35729robobun wants to merge 1 commit into
Conversation
…ufferView
When a runtime plugin's onLoad or build.module callback returns
{ contents: <Buffer|Uint8Array>, ... }, the C++ side wrapped the raw
bytes in a ZigString with no encoding tag. An untagged ZigString is
interpreted as Latin-1 by ZigString::to_slice(), which transcodes any
byte >= 0x80 into its multi-byte UTF-8 encoding before the transpiler
sees it. For UTF-8 source that means double-encoding (an 'é' in the
buffer reaches the parser as 'é'); for binary payloads every high
byte shifts the rest of the stream.
Tag the ArrayBufferView-backed ZigString as UTF-8 so the bytes pass
through to the loader verbatim. Adds a taggedUTF8Ptr() helper next to
the existing taggedUTF16Ptr().
String contents are unaffected: those already carry a correct Latin-1
or UTF-16 tag from toZigString(JSString*), and the transcode there is
intentional.
|
Warning Review limit reached
Next review available in: 2 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 (3)
Comment |
|
Updated 9:28 AM PT - Jul 25th, 2026
❌ @robobun, your commit 4523f8d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35729That installs a local version of the PR into your bun-35729 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line encoding-tag fix with matching helper and tests.
What was reviewed:
- Verified bit 61 in
taggedUTF8PtrmatchesZS_UTF8_BITinbun_alloc/lib.rsandisTaggedUTF8Ptrin helpers.h. - Traced
ZigString::to_slice()(src/bun_core/string/mod.rs:1694): with the UTF-8 bit set it borrows bytes verbatim; without it, Latin-1→UTF-8 transcodes — confirms the root cause. - Checked for sibling
ZigString { view->vector(), byteLength() }sites — this is the only one. - Empty/zero-length buffers:
to_slice()returns early onlen == 0before touching the tagged pointer.
Extended reasoning...
Overview
Single functional line in src/jsc/bindings/ModuleLoader.cpp: when a runtime plugin's onLoad/build.module returns contents as an ArrayBufferView, the ZigString wrapping those bytes is now tagged with the UTF-8 bit (bit 61) instead of being left untagged. A 4-line taggedUTF8Ptr() helper is added in helpers.h alongside the existing taggedUTF16Ptr(). Four new test cases in the existing plugins.test.ts cover sync/async × build.module/onLoad.
Correctness
I verified the bit constant end-to-end: taggedUTF8Ptr sets 1 << 61; the pre-existing isTaggedUTF8Ptr in the same header reads 1 << 61; and on the Rust side ZS_UTF8_BIT = 1usize << 61 in src/bun_alloc/lib.rs. ZigString::to_slice() (src/bun_core/string/mod.rs:1694) branches on is_utf8(): when set, it returns ZigStringSlice::Static borrowing the raw bytes; when unset (the pre-PR state), it calls to_utf8_from_latin1() which expands every byte ≥ 0x80 to two bytes — exactly the reported corruption. The fix is precisely at the layer that owns the invariant (encoding tag on the ZigString), and the string branch above it is intentionally left alone since JSString-sourced ZigStrings are genuinely Latin-1/UTF-16.
Security risks
None. This narrows an over-eager transcode; no new parsing, no new trust boundaries, no size arithmetic changes. The tagged pointer with a null vector is safe because to_slice() returns EMPTY when len == 0 before dereferencing.
Level of scrutiny
Low-to-medium. It touches C++/Rust FFI string tagging in the module loader, but the change is a single well-understood bit flag whose semantics are already established on both sides of the FFI boundary and already used elsewhere. I grepped for other ZigString { ...view->vector()... } construction sites to check the whole bug class — this is the only one.
Other factors
Tests are placed in the existing plugin test file per repo convention, use it.each for the 4-case matrix, assert exact values (both the string and its UTF-8 bytes), and per the PR description all four fail on main with "é". No outstanding reviewer comments.
|
CI on build 80921: the new Diff is ready for review. |
1 participant
What
Runtime
Bun.plugincallbacks (build.module(...)andonLoad(...)) that returncontentsas aBuffer/Uint8Arrayhad any byte>= 0x80expanded to its two-byte UTF-8 encoding before the loader saw it.For binary contents (e.g. a
.wasmbuffer supplied through a plugin, as in #35587) every high byte shifted the rest of the stream, which surfaces as a parse error at the wrong offset.Why
handleOnLoadResultNotPromisebuilt theZigStringfor the ArrayBufferView case with no encoding tag:result.value.sourceText.string = ZigString { reinterpret_cast<const unsigned char*>(view->vector()), view->byteLength() };An untagged
ZigStringis Latin-1 toZigString::to_slice()intranspile_virtual_module, which re-encodes any non-ASCII byte as UTF-8. A typed array of source bytes is already UTF-8 (or opaque binary), so that transcode is data corruption.The string branch right above it goes through
toZigString(JSString*, ...), which correctly tags Latin-1/UTF-16, so the transcode there is intentional and is left alone.Fix
Tag the ArrayBufferView-backed pointer with the UTF-8 bit so
to_slice()borrows the bytes verbatim. Adds ataggedUTF8Ptr()helper alongside the existingtaggedUTF16Ptr()inhelpers.h.Tests
test/js/bun/plugin/plugins.test.tsgains four cases coveringbuild.moduleandonLoad, each sync and async, asserting that a UTF-8éin aBufferround-trips unchanged. All four fail on main with"é".[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
32 concerns were raised and did not survive verification.