sql(mysql): reject out-of-range binary TIME fields instead of wrapping - #34706
sql(mysql): reject out-of-range binary TIME fields instead of wrapping#34706robobun wants to merge 2 commits into
Conversation
Time::from_binary reads the 4-byte days field as a raw u32 and the TIME arm of decode_binary_value computed total_hours = hours + days*24 in u32 with no range check. MySQL TIME is bounded to +/-838:59:59 (days <= 34), so a hostile or buggy server/proxy sending days=178956971 made days*24 wrap past 2^32 to 8 and Bun returned the string "08:05:06" with no error. Reject days>34 / hours>23 / minutes>59 / seconds>59 with InvalidBinaryValue, matching the range validation the DATETIME path already does.
|
Status Reproduced with a mock server sending a binary TIME cell with Fix: range-check the TIME fields in
CI #75769: the new test passes on all lanes. Remaining failures are unrelated to this diff: |
|
Updated 7:52 AM PT - Jul 19th, 2026
❌ @robobun, your commit 4526a95 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34706That installs a local version of the PR into your bun-34706 --bun |
|
Warning Review limit reached
Next review available in: 12 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 (2)
WalkthroughThe MySQL binary-protocol TIME decoder now rejects out-of-range fields. Tests construct raw TIME rows through a prepared-statement server and verify invalid values fail while the documented maximum decodes successfully. ChangesMySQL binary TIME validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sql_jsc/mysql/protocol/DecodeBinaryValue.rs`:
- Around line 137-138: Extend the TIME validation in DecodeBinaryValue.rs around
the existing per-field guard to reject the cross-field boundary where days=34
and hours exceed 22, or equivalently where the computed total hours exceed 838.
Add the corresponding days=34, hours=23 case to the test.each matrix in
test/js/sql/sql-mysql-time-binary-range.test.ts, expecting invalid binary value
handling.
🪄 Autofix (Beta)
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: f272a3c5-c65d-48ca-ab00-9f76c9cc722f
📒 Files selected for processing (2)
src/sql_jsc/mysql/protocol/DecodeBinaryValue.rstest/js/sql/sql-mysql-time-binary-range.test.ts
The per-field guard allowed days=34 hours=23 (839 hours), one past the documented +/-838:59:59 cap. days<=34 already bounds days*24 to 816 so the u32 sum is overflow-free; add a total_hours > 838 check after it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the total_hours > 838 guard closes the cross-field boundary I flagged earlier, and the (34,23) case is now in the reject matrix.
What was reviewed:
- Verified the per-field bound (
days <= 34) makesdays*24overflow-safe in u32 before the sum, and the follow-uptotal_hours > 838check seals the 839h gap. - Confirmed
InvalidBinaryValuemaps toERR_MYSQL_INVALID_BINARY_VALUE(any_mysql_error_jsc.rs:102) as the test asserts, andTimefield types (days: u32,hours/minutes/seconds: u8) match the guard arithmetic. - Test follows the existing
wire-frames.tsfault-injection pattern used by ~20 sibling files; the838:59:59positive case pins the documented maximum so the guard can't over-tighten.
Extended reasoning...
Overview
Two files: a 9-line addition to the MYSQL_TYPE_TIME arm of decode_binary_value in src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs that range-checks the wire fields (days > 34, hours > 23, minutes > 59, seconds > 59) and then rejects total_hours > 838, plus a new fault-injection test test/js/sql/sql-mysql-time-binary-range.test.ts driving a mock MySQL server via the shared wire-frames.ts helpers.
Security risks
The change is strictly defensive — it tightens validation of untrusted server bytes and turns a silent u32 wrap into a typed error. A compliant MySQL server never emits TIME outside ±838:59:59, so no legitimate input is newly rejected. No new parsing, no allocation, no user-controlled sizing; the error path uses the existing crate::Error::InvalidBinaryValue variant already used by sibling arms in this function and mapped to ERR_MYSQL_INVALID_BINARY_VALUE.
Level of scrutiny
Low-to-medium. It's a localized, additive bounds check in a single decode arm with no control-flow or type changes elsewhere. The arithmetic is trivially verifiable: with days <= 34, days*24 <= 816 (fits u32), plus hours <= 23 gives at most 839, which the second guard rejects. The Time struct (days: u32, hours/minutes/seconds: u8) matches the code's assumptions.
Other factors
- All prior review threads (CodeRabbit's cross-field gap, my own stale nit on the same point) are resolved by commit 4526a95 and confirmed present in the current diff.
- Test coverage hits the exact-limit-succeeds / one-past-fails contract REVIEW.md asks for: 7 reject cases (including the u32-wrap repro and the 839h boundary) and one accept case at 838:59:59, with
sawExecuteasserted so the test can't pass by short-circuiting before the row is decoded. - The test file reuses the established mock-server harness (
listeningServer,mysqlHandshakeV10,mysqlStmtPrepareOk,mysqlReadPackets) already used by many sibling SQL fault-injection tests, and the header comment correctly scopes it as a do-not-copy pattern. - The bug-hunting system found nothing this run.
1 participant
What does this PR do?
The MySQL binary-protocol TIME decoder read the 4-byte
daysfield as a rawu32and computedtotal_hours = hours + days * 24inu32with no range check. MySQL TIME is bounded to-838:59:59 .. 838:59:59(days <= 34), so a hostile or buggy server/proxy sendingdays = 178956971madedays * 24wrap past2^32to8and Bun returned the string"08:05:06"with no error. The adjacent DATETIME arm already validates month/day/hour/minute/second and surfacesInvalid Date; the TIME arm now rejectsdays > 34/hours > 23/minutes > 59/seconds > 59withERR_MYSQL_INVALID_BINARY_VALUEthe same way.Repro
Other shapes of the same bug:
days = 0xFFFFFFFFdecoded as"4294967272:05:06"days = 178956970, hours = 255decoded as"239:05:06"How did you verify your code works?
New fault-injection test
test/js/sql/sql-mysql-time-binary-range.test.tsdrives a mock server that emits binary TIME cells with each field out of range and asserts the query rejects withERR_MYSQL_INVALID_BINARY_VALUE. A boundary case (838:59:59, i.e.days = 34, hours = 22) confirms the documented maximum still decodes.The existing container round-trip in
test/js/sql/sql-mysql.test.ts("time"test,-838:59:59/838:59:59) is within the new bound.Note
The assert build profile sets
CARGO_PROFILE_RELEASE_DEBUG_ASSERTIONS=true(scripts/build/rust.ts) but notCARGO_PROFILE_RELEASE_OVERFLOW_CHECKS, so integer wraps in this whole class stay silent under ASAN/assert builds too. Enabling overflow-checks there would let fuzzing catch these; left out of this PR since it is a build-profile change with broader blast radius.