server: harden parseBinaryParams against malformed length-encoded values

[expxiaoli]

What problem does this PR solve?

Issue Number: close #67486

Problem Summary:

This PR supersedes #67542. I do not have write access to the original contributor branch, so I am carrying the same fix forward on a branch I can maintain.

Malformed length-encoded parameter payloads in COM_STMT_EXECUTE may panic in parseBinaryParams. Large or truncated values should be rejected as malformed packets instead of reaching unsafe slicing logic.

What changed and how does it work?

This PR keeps the original hardening change and continues from it:

1. Check that the length-encoded header has enough bytes before decoding it.
2. Check the decoded payload length against the remaining buffer using uint64-safe bounds before converting to int and slicing.
3. Cover truncated and oversized payload cases with regression tests so malformed packets return an error instead of panicking.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test
• I checked and no code files have been changed.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Fix a panic in `COM_STMT_EXECUTE` parameter parsing by rejecting malformed length-encoded parameter payloads before slicing them.

Summary by CodeRabbit

• Bug Fixes

  • Parsing now returns explicit errors for truncated or malformed length-encoded data, preventing out-of-bounds panics and ensuring malformed packets surface as parse errors.
  • Handshake parsing now reliably detects and fails on truncated auth and connection-attribute payloads.

• Refactor

  • Centralized binary-parameter slicing and bounds checks for consistent error handling.

• Tests

  • Added/extended tests to cover malformed length-encoded parameters and truncated handshake payloads.

[pantheon-ai]

@expxiaoli I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

_{ℹ️ Learn more details on Pantheon AI.}

[tiprow]

Hi @expxiaoli. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fc8b3b0b-52a0-44e1-86dd-fd23a058c038

📥 Commits

Reviewing files that changed from the base of the PR and between 7f99803 and 80dcc68.

📒 Files selected for processing (1)

• pkg/server/conn_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/server/conn_test.go

---

📝 Walkthrough

Walkthrough

Adds error reporting for truncated length-encoded integers, centralizes safe extraction of length-encoded parameter payloads, refactors binary parameter parsing to use the helper, updates handshake parsing to check length-encoding errors, and adds tests for malformed/truncated headers.

Changes

Length-encoded parameter parsing and malformed packet handling

Layer / File(s)	Summary
ParseLengthEncodedInt and tests pkg/server/internal/util/util.go, pkg/server/internal/util/util_test.go	ParseLengthEncodedInt now returns (num,isNull,n,err) and returns io.EOF for truncated inputs; ParseLengthEncodedBytes propagates errors; tests updated to assert errors (io.EOF) and conditional sizes.
Binary parameter helper and parser refactor pkg/server/conn_stmt_params.go, pkg/server/conn_stmt_params_test.go	Adds takeBinaryParamValue to validate/slice length-encoded parameter payloads; parseBinaryParams now uses util2.ParseLengthEncodedInt + takeBinaryParamValue for blob/string-like types; adds TestParseExecArgsMalformedLengthEncodedParam.
Handshake length-encoded validation and tests pkg/server/internal/parse/parse.go, pkg/server/conn_test.go	HandshakeResponseBody now checks the err from ParseLengthEncodedInt for auth-data and connection-attributes lengths and returns mysql.ErrMalformPacket on parse errors; tests added for truncated headers.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibble bytes beneath the moonlight,
Length headers short or wildly contrite.
I guard each slice with a careful paw,
No more panics, no more awe,
Now packets behave through the night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: hardening parseBinaryParams against malformed length-encoded values, which directly addresses the panic issue in the linked PR objective.
Description check	✅ Passed	The PR description includes the required issue number, clear problem summary addressing the panic, detailed explanation of changes, and a release note, meeting the template requirements comprehensively.
Linked Issues check	✅ Passed	The PR fully addresses issue #67486 by implementing bounds checking for length-encoded headers [#67486], validating decoded payload lengths with uint64-safe checks [#67486], and adding regression tests for truncated/oversized payloads [#67486].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to hardening length-encoded parameter parsing: refactoring parseBinaryParams, adding helper functions, updating parse utilities, and adding focused regression tests—no extraneous modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

pkg/server/conn_stmt_params_test.go (1)

285-317: ⚡ Quick win

Add a plain short-payload regression case.

This covers the truncated-header path and the huge-length overflow path, but it doesn't directly exercise the Line 51 remaining-bytes guard in pkg/server/conn_stmt_params.go. A case with a valid header and too few payload bytes would lock in the common truncation path this PR is hardening.

🧪 Minimal addition

 	{
 		name:        "truncated length-encoded uint64 header",
 		paramValues: []byte{0xfe},
 	},
+	{
+		name:        "declared length exceeds remaining payload",
+		paramValues: []byte{0x03, 'a', 'b'},
+	},
 	{
 		name: "overflowing length-encoded uint64",
 		// length = 1<<63, which previously could slip past `pos + int(length)` checks and panic on slicing.
 		paramValues: []byte{0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80},
 	},

🤖 Prompt for 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.

In `@pkg/server/conn_stmt_params_test.go` around lines 285 - 317, Add a regression
case to TestParseExecArgsMalformedLengthEncodedParam that exercises the "valid
header but too few payload bytes" path: append a test entry whose paramValues
contains a well-formed length-encoded header (e.g., 0xFC followed by a 2-byte
little-endian length > actual payload) but then provides fewer payload bytes
than that length so decodeAndParse triggers the remaining-bytes guard in
conn_stmt_params.go; ensure the test still asserts mysql.ErrMalformPacket via
the same decodeAndParse call.

🤖 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.

Nitpick comments:
In `@pkg/server/conn_stmt_params_test.go`:
- Around line 285-317: Add a regression case to
TestParseExecArgsMalformedLengthEncodedParam that exercises the "valid header
but too few payload bytes" path: append a test entry whose paramValues contains
a well-formed length-encoded header (e.g., 0xFC followed by a 2-byte
little-endian length > actual payload) but then provides fewer payload bytes
than that length so decodeAndParse triggers the remaining-bytes guard in
conn_stmt_params.go; ensure the test still asserts mysql.ErrMalformPacket via
the same decodeAndParse call.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e1c50dd9-7738-47a1-9c20-48cf40848ad0

📥 Commits

Reviewing files that changed from the base of the PR and between c1faae7 and 7476b58.

📒 Files selected for processing (2)

• pkg/server/conn_stmt_params.go
• pkg/server/conn_stmt_params_test.go

[codecov]

Codecov Report

❌ Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.0901%. Comparing base (032f5ac) to head (e56167d).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #68917        +/-   ##
================================================
- Coverage   76.3186%   75.0901%   -1.2285%     
================================================
  Files          2041       2027        -14     
  Lines        562849     569350      +6501     
================================================
- Hits         429559     427526      -2033     
- Misses       132377     141711      +9334     
+ Partials        913        113       -800     

Flag	Coverage Δ
integration	41.4195% <0.0000%> (+1.6808%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	60.4610% <ø> (ø)
parser	∅ <ø> (∅)
br	49.5656% <ø> (-13.2339%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wuhuizuo]

/retest

[tiprow]

@wuhuizuo: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[YangKeao]

LGTM

[lance6716]

/lgtm

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-06-04 07:35:25.241146808 +0000 UTC m=+427026.311464198: ☑️ agreed by YangKeao.
• 2026-06-04 09:10:25.708355458 +0000 UTC m=+432726.778672858: ☑️ agreed by lance6716.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, wjhuang2016, YangKeao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [YangKeao,lance6716,wjhuang2016]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[expxiaoli]

/retest-required

[tiprow]

@expxiaoli: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest-required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[expxiaoli]

/retest-required

[tiprow]

@expxiaoli: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest-required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[expxiaoli]

/retest

[tiprow]

@expxiaoli: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[YangKeao]

/retest

[YangKeao]

/ok-to-test