Repository navigation
server: limit accumulated COM_STMT_SEND_LONG_DATA size (#69694) - #70231
ti-chi-bot[bot] merged 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@YangKeao This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions 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 ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughPrepared statements now limit accumulated ChangesLong-data validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 2
🤖 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 `@pkg/server/conn_stmt_test.go`:
- Around line 26-30: Clean up the import block in conn_stmt_test.go by removing
the literal merge-conflict markers surrounding the resolve and servererr
imports. Preserve both required imports and ensure the resulting import
declaration is valid Go.
In `@pkg/server/driver_tidb.go`:
- Around line 111-135: The prepared statement currently checks packet size per
parameter instead of against one statement-wide byte budget. In
pkg/server/driver_tidb.go lines 111-135, update TiDBStatement’s long-data
handling and CheckLongDataSize to track cumulative bound bytes, reject additions
that exceed MaxAllowedPacket, and reset the accumulated count in Reset. In
pkg/server/conn_stmt_test.go lines 401-409, adjust the test to keep the combined
payload exactly at the limit, then append one byte and assert the overflow
error.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b0c90a2-eb37-418d-86be-918c61b52c06
📒 Files selected for processing (4)
pkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/driver.gopkg/server/driver_tidb.go
| if uint64(len(ts.boundParams[paramID]))+uint64(len(data)) > ts.ctx.GetSessionVars().MaxAllowedPacket { | ||
| // MySQL reports the packet-too-large error on the following EXECUTE, not on SEND_LONG_DATA. | ||
| // Stop appending more bytes once the limit is exceeded so the statement cannot grow unboundedly. | ||
| ts.boundParamsTooLarge = true | ||
| return nil | ||
| } | ||
| ts.boundParams[paramID] = append(ts.boundParams[paramID], data...) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // CheckLongDataSize implements PreparedStatement CheckLongDataSize method. | ||
| func (ts *TiDBStatement) CheckLongDataSize() error { | ||
| if ts.boundParamsTooLarge { | ||
| return servererr.ErrNetPacketTooLarge | ||
| } | ||
| maxAllowedPacket := ts.ctx.GetSessionVars().MaxAllowedPacket | ||
| for _, boundParam := range ts.boundParams { | ||
| if uint64(len(boundParam)) > maxAllowedPacket { | ||
| return servererr.ErrNetPacketTooLarge | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Use one statement-wide byte budget in both implementation and test.
The implementation only enforces per-parameter lengths, and the test encodes that same incorrect behavior; together they allow and approve totals above MaxAllowedPacket.
pkg/server/driver_tidb.go#L111-L135: maintain and validate a prepared-statement-wide accumulated byte count, resetting it duringReset.pkg/server/conn_stmt_test.go#L401-L409: keep the combined payload at the limit, then add one byte and assert the overflow behavior.
📍 Affects 2 files
pkg/server/driver_tidb.go#L111-L135(this comment)pkg/server/conn_stmt_test.go#L401-L409
🤖 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/driver_tidb.go` around lines 111 - 135, The prepared statement
currently checks packet size per parameter instead of against one statement-wide
byte budget. In pkg/server/driver_tidb.go lines 111-135, update TiDBStatement’s
long-data handling and CheckLongDataSize to track cumulative bound bytes, reject
additions that exceed MaxAllowedPacket, and reset the accumulated count in
Reset. In pkg/server/conn_stmt_test.go lines 401-409, adjust the test to keep
the combined payload exactly at the limit, then append one byte and assert the
overflow error.
ti-chi-bot
Bot
added
cherry-pick-approved
Keep the servererr import required by the new test, and drop the unused resolve import that only exists on master. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70231 +/- ##
================================================
Coverage ? 55.1176%
================================================
Files ? 1849
Lines ? 666387
Branches ? 0
================================================
Hits ? 367297
Misses ? 271704
Partials ? 27386
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
@expxiaoli: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: expxiaoli, wjhuang2016, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
6 participants
This is an automated cherry-pick of #69694
What problem does this PR solve?
Issue Number: close #69693
Problem Summary:
COM_STMT_SEND_LONG_DATAappends each payload chunk into the prepared statement's bound parameter buffers. Before this PR, TiDB did not check the cumulative size of those buffers, so an authenticated client could keep sending long-data chunks for a prepared statement and grow connection memory without executing the statement.What changed and how does it work?
This PR tracks the total bytes accumulated by
COM_STMT_SEND_LONG_DATAfor each prepared statement. Before appending a new chunk, TiDB checks whether the statement-level accumulated long-data size would exceed the sessionmax_allowed_packet. If it would exceed the limit, TiDB returns the existingErrNetPacketTooLargeerror and keeps the already buffered data unchanged.COM_STMT_RESETclears both the buffered parameters and the byte counter.Check List
Tests
Tested locally:
git diff --checkgo test ./pkg/server -run '^(TestStmtSendLongDataMaxAllowedPacket|TestCursorFetchSendLongData|TestCursorFetchSendLongDataReset)$' -count=1go test --tags=intest ./pkg/server -run '^(TestStmtSendLongDataMaxAllowedPacket|TestCursorFetchSendLongData|TestCursorFetchSendLongDataReset)$' -count=1Side effects
Documentation
Release note
Summary by CodeRabbit