Conversation
Encoding::encode_with_max_size's Base64url arm allocated an intermediate
Vec<u8> via simdutf_encode_url_safe_alloc, then copied into the JS string.
The sibling Base64 and Hex arms already encode directly into the
String::create_uninitialized_latin1 destination; do the same for Base64url
using bun_base64::url_safe_encode_len + encode_url_safe. One fewer heap
alloc+free per digest('base64url') / CSRF.generate / randomUUID('base64url').
Delete simdutf_encode_url_safe_alloc; this was its only caller.
|
Warning Review limit reached
More reviews will be available in 7 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 7:42 AM PT - Jun 15th, 2026
❌ @robobun, your commit eb0d8f6 has 6 failures in
🧪 To try this PR locally: bunx bun-pr 32320That installs a local version of the PR into your bun-32320 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward refactor making the Base64url arm match the existing Base64/Hex pattern.
Extended reasoning...
Overview
This PR rewrites the Self::Base64url arm of Encoding::encode_with_max_size in src/runtime/node/types.rs to encode directly into a String::create_uninitialized_latin1 buffer (via bun_base64::url_safe_encode_len + bun_base64::encode_url_safe) instead of allocating an intermediate Vec<u8> and copying. The new code is a near-verbatim copy of the adjacent Self::Base64 arm. It also deletes the now-unused simdutf_encode_url_safe_alloc helper from src/base64/lib.rs and drops the no-longer-needed ZigStringJsc trait import.
Security risks
None identified. The change is allocation-shape only; encoding logic still goes through the same simdutf::base64::encode(.., true) call. url_safe_encode_len computes the exact no-padding length (0/2/3 trailing chars per leftover), which matches what simdutf writes, so the destination buffer is fully initialized — no uninitialized-byte exposure. The is_dead() OOM guard mirrors the sibling arms.
Level of scrutiny
Low. ~10 lines of net change, mechanical pattern-matching against existing code in the same function, no new unsafe, no behavioral change. Affected callers (hash digests, CSRF token generation, UUID encoding) are bounded-size inputs well within the encoding's correctness envelope.
Other factors
I confirmed simdutf_encode_url_safe_alloc has no remaining callers in the tree. The PR description lists four passing test suites covering base64url encoding, CryptoHasher, and CSRF, plus a manual SHA-256 digest sanity check. The bug-hunting system found nothing. No prior review comments to address.
There was a problem hiding this comment.
LGTM — straightforward alloc-elision that mirrors the sibling Base64 arm; one nit on the new test below.
Extended reasoning...
Overview
This PR changes the Base64url arm of Encoding::encode_with_max_size in src/runtime/node/types.rs to encode directly into the String::create_uninitialized_latin1 destination buffer instead of allocating an intermediate Vec<u8> and copying. It deletes the now-unused simdutf_encode_url_safe_alloc helper from src/base64/lib.rs (grep confirms no remaining references), drops a now-unused ZigStringJsc import, and adds a parameterised digest-length test to base64-url-safe-encode.test.ts.
Security risks
None. This is a pure encoding-buffer plumbing change with no input parsing, auth, or trust-boundary implications. The only memory-safety question is whether url_safe_encode_len(input) exactly matches what encode_url_safe writes — both bottom out in the same no-padding ceil(n*4/3) formula (one via simdutf, one via the const fn in lib.rs), and the debug_assert_eq!(n, encoded_len) guards it. Inputs are bounded digest sizes (≤ EVP_MAX_MD_SIZE), so there's no overflow concern.
Level of scrutiny
Low. The new arm is a near-verbatim copy of the Base64 arm directly above it (same create_uninitialized_latin1 → is_dead() early-return → encode → debug_assert_eq! → transfer_to_js shape), just swapping encode_len/encode for url_safe_encode_len/encode_url_safe. Both swapped-in functions already exist and are exercised elsewhere. The deleted helper was a trivial wrapper with this as its sole caller.
Other factors
The inline nit notes that the newly added test.each block uses createHash (C++ JSHash path) rather than Bun.CryptoHasher, so it doesn't actually exercise the Rust arm it claims to cover. However, the pre-existing Bun.CryptoHasher digest('base64url') test on line 80 of the same file and the 397-test bun-cryptohasher.test.ts suite (which iterates base64url over every algorithm via Bun.CryptoHasher) do cover the changed code, so this is purely a comment-accuracy nit that doesn't gate the merge. No CODEOWNERS apply to these paths and there are no outstanding human review comments.
…ypto node:crypto's createHash().digest() routes through C++ JSHash / StringBytes::encode, not the Rust Encoding::encode_with_max_size arm. Switch the per-algorithm sizing test to Bun.CryptoHasher so it actually covers the in-place base64url encode.
There was a problem hiding this comment.
LGTM — the Base64url arm now mirrors the Base64/Hex pattern in the same match, and the test fix addresses my earlier comment.
Extended reasoning...
Overview
This PR rewrites the Self::Base64url arm of Encoding::encode_with_max_size in src/runtime/node/types.rs to encode directly into a String::create_uninitialized_latin1 buffer instead of allocating an intermediate Vec<u8> and copying. The new arm is line-for-line structurally identical to the Self::Base64 arm immediately above it (compute length → allocate WTF string → check is_dead() → encode in place → debug_assert_eq! on written length → transfer_to_js). It also deletes the now-orphaned simdutf_encode_url_safe_alloc helper from src/base64/lib.rs (grep confirms no remaining callers), drops the now-unused ZigStringJsc import and a stale "we heap-allocate" comment, and adds per-algorithm sizing tests in test/js/bun/util/base64-url-safe-encode.test.ts.
Security risks
None. The underlying encoding call is unchanged (simdutf::base64::encode(.., true) on the same input bytes); only the destination buffer's provenance changes. Buffer sizing uses the existing url_safe_encode_len (= ⌈n·4/3⌉, the exact unpadded base64url length), guarded by debug_assert_eq!(n, encoded_len) and now exercised across digest sizes covering all three mod-3 phases. No auth, crypto-primitive, or permission code is touched.
Level of scrutiny
Low. This is a mechanical refactor that makes one match arm consistent with its two siblings using primitives that already exist in the same crate. Output is byte-identical by construction. The deleted helper had exactly one caller. None of the touched files are in CODEOWNERS.
Other factors
I previously flagged that the new test used node:crypto's createHash (C++ JSHash path) rather than the Rust path this PR modifies. The author addressed that in eb0d8f6 — the test now uses Bun.CryptoHasher and the comment accurately documents which path it covers; the inline thread is resolved. No new bugs were found on the updated revision. The CI failures reported by robobun are on the earlier commit and are unrelated infrastructure noise (clang -no-pie linker warnings, ci.ts build failures), not Rust compilation or test failures from this change.
|
Build 62501 finished: 282 passed, 4 failed. Failures are all unrelated to this diff:
This diff touches only The diff is ready. As noted in the PR body, the change is alloc-count only with byte-identical output (same |
|
Stale PR review: closing. This PR has had no human activity since 2026-06-15 and conflicts with main. Its goal is already on main. #40374 (commit 3e8b2e6, merged 2026-08-25) removed the per-encoding arms from Reopen if this evidence is wrong. |
1 participant
What
Encoding::encode_with_max_size'sBase64urlarm allocated an intermediateVec<u8>viasimdutf_encode_url_safe_alloc, then copied that into the JS string viaZigString::init(&buf).to_js(), then dropped the Vec. One extra heap alloc+free per call relative to the original Zig, which encoded into a comptime-sized stack buffer.The sibling
Base64andHexarms in the same match already encode directly into theString::create_uninitialized_latin1destination buffer. This change makesBase64urldo the same using the existingbun_base64::url_safe_encode_len+bun_base64::encode_url_safe.Also deletes
simdutf_encode_url_safe_alloc; this was its only caller.Affected paths
encode_with_max_sizeis called from:Bun.CryptoHasher#digest('base64url')and theBun.SHA*family (CryptoHasher.rs)Bun.CSRF.generate(secret, { encoding: 'base64url' })(csrf_jsc.rs)Bun.randomUUIDv7('base64url')(Crypto.rs)(
node:crypto'screateHash(...).digest('base64url')takes the C++JSHash/StringBytes::encodepath, not this one.)Verification
Output is byte-identical; the change is alloc-count only. Correctness covered by existing tests plus new per-algorithm sizing checks through
Bun.CryptoHasher:Note on testing
This change is alloc-count only; output is byte-identical before and after (both implementations call the same
simdutf::base64::encode(.., true)on the same input, only the destination buffer's provenance differs). There is no JS-observable difference to assert on, so the added tests are correctness guards for the in-place sizing (url_safe_encode_lenmust exactly match what simdutf writes for every digest length) rather than a fail-before regression test. The allocation removal is verifiable by inspection:types.rsno longer callssimdutf_encode_url_safe_alloc, and that helper (whosevec![0u8; len]was the extra allocation) is deleted.