sql: drop redundant clamps and a zero-fill of a zero-initialized buffer

[robobun]

What

Removes four dead guards in src/sql/ that are porting artifacts from the Zig rewrite (#30412). In Zig the original code initialized buffers with undefined and had different slice-length contracts; in Rust these checks are provably unreachable.

No behaviour change.

Why each one is dead

src/sql/mysql/protocol/PreparedStatement.rs write_null_bitmap: [0u8; MYSQL_MAX_PARAMS] is zero-initialized by the language, so the subsequent .fill(0) on a sub-slice writes zeros over zeros. The sibling NewWriter::write_null_bitmap (src/sql/mysql/protocol/NewWriter.rs:104) already omits the fill.

src/sql/mysql/protocol/HandshakeV10.rs: auth_plugin_data_len is floored to 21 before the subtraction, so auth_plugin_data_len - 8 >= 13 and .max(13) is a no-op. Collapsed the if clamp and the later .max(13) into a single .max(21), which also lets the binding lose mut.

src/sql/postgres/CommandTag.rs: strings::index_of_char wraps highway::index_of_char, which returns None when the byte is absent and otherwise an index strictly inside the slice (the implementation debug_assert!s haystack[result] == needle). So idx + 1 <= len and .min(len) on idx + 1 is a no-op. Three call sites.

src/sql/postgres/protocol/NewReader.rs int<Int>: both concrete postgres ReaderContext::read implementations, the shared StackReader::read (src/sql/shared/StackReader.rs:94) and the live-socket Reader::read (src/sql_jsc/postgres/PostgresSQLConnection.rs:1743), either return Err(ShortRead) or a slice of exactly count bytes. The slice.len() < Int::SIZE re-check is unreachable under every extant impl, and from_be_slice already slices [..SIZE] internally so an undersized input from a future implementation would still fail loudly rather than silently. Added a doc comment on the trait method stating the contract.

Verification

cargo check -p bun_sql and cargo clippy -p bun_sql are clean. The existing fault-injection protocol tests that exercise HandshakeV10 decoding and NewReaderWrap::int still pass under bun bd:

test/js/sql/sql-mysql-auth-short-nonce.test.ts        2 pass
test/js/sql/postgres-invalid-message-length.test.ts  10 pass

This is a pure refactor with no observable difference on any input, so there is no test that can fail on the released binary and pass on this branch; the justification is the analysis above plus the existing coverage.

[robobun]

^{Updated 1:44 AM PT - Jul 20th, 2026}

@robobun, your commit bd89027 is building: #76178

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f1599147-b204-4607-8561-96cc9bb996c0

📥 Commits

Reviewing files that changed from the base of the PR and between 0278471 and bd89027.

📒 Files selected for processing (4)

• src/sql/mysql/protocol/HandshakeV10.rs
• src/sql/mysql/protocol/PreparedStatement.rs
• src/sql/postgres/CommandTag.rs
• src/sql/postgres/protocol/NewReader.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — each removed guard is provably dead.

What was reviewed:

• HandshakeV10: .max(21) before - 8 guarantees >= 13, so the dropped .max(13) was a no-op; u8 arithmetic can't underflow here.
• CommandTag: verified strings::index_of_char → highway::index_of_char returns an in-bounds index, so idx + 1 <= len and the .min(len) clamps were unreachable.
• NewReader::int: confirmed the only ReaderContext::read impl is StackReader::read (src/sql/shared/StackReader.rs:94), which returns exactly count bytes or ShortRead; from_be_slice still slices [..SIZE] so a future short read would panic, not silently truncate.
• PreparedStatement: [0u8; N] is zero-initialized by definition; .fill(0) was a no-op.

Extended reasoning...

Overview

Removes four dead defensive guards in src/sql/ that are Zig-to-Rust porting artifacts: a .fill(0) on an already-zeroed stack array, a redundant .max(13) clamp after a .max(21) floor, three .min(len) clamps on indices that are already in-bounds, and a length re-check on a slice whose only producer guarantees exact length. Net -8/+4 lines across four files.

Verification of each claim

I traced each removed guard to source:

• strings::index_of_char (src/bun_core/string/immutable.rs:1682) delegates to highway::index_of_char, and the sibling index_of_char_pos debug_assert!s the returned index is strictly in-bounds. So idx + 1 <= len holds and &tag[idx+1..] is always valid (possibly empty).
• StackReader::read (src/sql/shared/StackReader.rs:94) either errors with SHORT_READ or returns Data::Temporary over &buffer[offset..offset+count] — exactly count bytes. Grep confirms it's the sole concrete ReaderContext impl for the postgres reader (plus the blanket &mut C forwarder). The from_be_slice macro body still does &bytes[..Self::SIZE] before copy_from_slice, so a hypothetical undersized slice would panic on the index rather than read garbage.
• u8::max(21) - 8 ranges [13, 247], so both the old if < 21 branch and the .max(13) were equivalent to the new single .max(21).
• [0u8; N] is zero-initialized by the language; the .fill(0) was a Zig undefined-buffer artifact.

Security risks

None. This is protocol parsing over server-supplied bytes, but every removed check was provably unreachable given the invariants of the callee it guards. No validation of untrusted input is weakened — the HandshakeV10 floor and the StackReader short-read error remain in place.

Level of scrutiny

Low-to-medium. Pure dead-code removal with no observable behavior change on any input; each claim is mechanically verifiable against the callee source. Existing fault-injection tests still exercise these paths.

Other factors

No prior reviewer comments. The PR description is precise and each claim held up on inspection. The "no test can distinguish before/after" justification is accurate for a pure no-op removal.

[robobun]

This PR removes code that is provably unreachable on every input, so there is no test that can distinguish the patched binary from the released one: any assertion that passes here also passes on main. The analysis for each of the four sites is in the PR body, and the existing fault-injection tests (test/js/sql/sql-mysql-auth-short-nonce.test.ts, test/js/sql/postgres-invalid-message-length.test.ts) exercise the touched decode paths and still pass.

This is the same class of src-only src/sql/ cleanup as #32128 and #32141. Leaving it open for maintainer review on the strength of the analysis; happy to close if that is preferred.

[claude]

LGTM — each removed guard is provably dead and the reasoning in the description checks out.

What was reviewed:

• highway::index_of_char (src/highway/lib.rs:126) returns None at len and otherwise an in-bounds index, so idx + 1 <= len always holds — the three .min(len) clamps in CommandTag were no-ops.
• Both concrete ReaderContext::read impls (StackReader.rs:94, PostgresSQLConnection.rs:1743) return exactly count bytes or ShortRead; from_be_slice still slices [..SIZE] internally so a future non-conforming impl panics rather than misreads. The new trait doc comment records the contract.
• HandshakeV10: .max(21) on a u8 guarantees the subsequent - 8 >= 13, so the dropped .max(13) was unreachable and no underflow is possible.
• [0u8; N] is zero-initialized by construction; the sibling NewWriter::write_null_bitmap already omits the fill.

Extended reasoning...

Overview

Four small deletions of provably-dead defensive code in src/sql/, all identified as Zig→Rust porting artifacts: a .fill(0) on an already-zero-initialized stack array in the MySQL COM_STMT_EXECUTE null-bitmap writer; a redundant .max(13) clamp in MySQL HandshakeV10 decoding (collapsed with the preceding if < 21 into a single .max(21)); three .min(len) clamps on index_of_char results in the Postgres CommandTag parser; and a length re-check in the Postgres NewReaderWrap::int<Int> helper. A doc comment is added to ReaderContext::read stating the exact-count-or-ShortRead contract that int<Int> now relies on directly.

Security risks

None. All removed checks operate on server-provided wire bytes, but each is unreachable given the invariants established immediately before it. The one case where the guard was structural rather than arithmetic (int<Int>'s length re-check) still fails loudly if a future ReaderContext impl violates the newly-documented contract: from_be_slice slices &bytes[..SIZE] before copy_from_slice, so an undersized slice panics on the bounds check rather than reading garbage. No buffer arithmetic on adversarial input was loosened.

Level of scrutiny

Low-to-moderate. This is protocol-parsing code on the client side of MySQL/Postgres connections, so I traced each claim to source rather than taking the description at face value. I confirmed highway::index_of_char returns None when the needle is absent and otherwise a strictly in-bounds index (src/highway/lib.rs:126-140, with a debug_assert! on haystack[result]); confirmed both live ReaderContext::read implementations return exactly count bytes on Ok (src/sql/shared/StackReader.rs:94-103 and src/sql_jsc/postgres/PostgresSQLConnection.rs:1743-1754); confirmed from_be_slice internally re-slices to [..SIZE]; and confirmed the sibling NewWriter::write_null_bitmap (src/sql/mysql/protocol/NewWriter.rs:104) already omits the redundant fill. The HandshakeV10 arithmetic is closed-form on u8 and cannot underflow.

Other factors

The PR follows the review guideline to delete defensive code only when the condition is shown to be unreachable, and it explains the Zig-era origin of each guard. The description notes existing fault-injection tests (sql-mysql-auth-short-nonce.test.ts, postgres-invalid-message-length.test.ts) still pass, and correctly observes that a pure no-behavior-change refactor cannot produce a test that fails on the released binary. The added trait doc comment is the right way to record the contract now that a caller depends on it without a local re-check. No prior reviews on the PR; the bug-hunting system found nothing.