Repository navigation
server: limit accumulated COM_STMT_SEND_LONG_DATA size - #69694
Conversation
Signed-off-by: bb7133 <bb7133@gmail.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughChangesLong-data packet limits
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TiDBStatement
participant handleStmtExecute
Client->>TiDBStatement: Send long-data chunks
TiDBStatement->>TiDBStatement: Track cumulative size
Client->>handleStmtExecute: Execute prepared statement
handleStmtExecute->>TiDBStatement: CheckLongDataSize()
TiDBStatement-->>handleStmtExecute: ErrNetPacketTooLarge
handleStmtExecute-->>Client: Return packet-too-large error
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 |
ti-chi-bot
Bot
added
size/M
bb7133
removed
the
do-not-merge/work-in-progress
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69694 +/- ##
================================================
- Coverage 76.3227% 73.7835% -2.5393%
================================================
Files 2041 2058 +17
Lines 560399 591773 +31374
================================================
+ Hits 427712 436631 +8919
- Misses 131786 153540 +21754
- Partials 901 1602 +701
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiancaiamao, 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:
|
86aeefb to
ecf29a5
Compare
![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 `@pkg/server/conn_stmt_test.go`:
- Around line 597-614: Update the long-data test around dispatchSendLongData so
the initial chunks cumulatively total 1024 bytes, then assert the next append
returns servererr.ErrNetPacketTooLarge. Verify the rejected append leaves both
stmt.BoundParams() buffers unchanged, and remove the deferred execution-time
error expectation while preserving the successful CheckLongDataSize assertion.
🪄 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: 45838da0-61ac-4a14-a1b8-9e4d08e59135
📒 Files selected for processing (4)
pkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/driver.gopkg/server/driver_tidb.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/server/conn_stmt.go
- pkg/server/driver.go
- pkg/server/driver_tidb.go
| require.NoError(t, dispatchSendLongData(c, stmt.ID(), 0, bytes.Repeat([]byte{'a'}, 1024))) | ||
| require.NoError(t, dispatchSendLongData(c, stmt.ID(), 1, bytes.Repeat([]byte{'b'}, 1024))) | ||
| require.NoError(t, stmt.CheckLongDataSize()) | ||
|
|
||
| err = dispatchSendLongData(c, stmt.ID(), 0, []byte{'c'}) | ||
| require.NoError(t, err) | ||
| require.Len(t, stmt.BoundParams()[0], 1024) | ||
| require.Len(t, stmt.BoundParams()[1], 1024) | ||
|
|
||
| err = c.Dispatch(context.Background(), append( | ||
| binary.LittleEndian.AppendUint32([]byte{mysql.ComStmtExecute}, uint32(stmt.ID())), | ||
| 0x0, 0x1, 0x0, 0x0, 0x0, | ||
| 0x0, 0x0, | ||
| )) | ||
| require.ErrorIs(t, err, servererr.ErrNetPacketTooLarge) | ||
| require.Nil(t, stmt.BoundParams()[0]) | ||
| require.Nil(t, stmt.BoundParams()[1]) | ||
| require.NoError(t, stmt.CheckLongDataSize()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the cumulative limit at send time.
With a 1024-byte limit, the second 1024-byte append already exceeds the statement’s cumulative limit. The extra byte must also be rejected by dispatchSendLongData, while existing buffered data remains unchanged; this test currently expects both sends to succeed and defers the error to execution.
Use chunks whose combined size equals 1024, assert servererr.ErrNetPacketTooLarge on the next append, and verify the existing buffers are preserved.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 606-606: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(stmt.ID())
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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_test.go` around lines 597 - 614, Update the long-data
test around dispatchSendLongData so the initial chunks cumulatively total 1024
bytes, then assert the next append returns servererr.ErrNetPacketTooLarge.
Verify the rejected append leaves both stmt.BoundParams() buffers unchanged, and
remove the deferred execution-time error expectation while preserving the
successful CheckLongDataSize assertion.
|
/retest |
|
/cherry-pick release-8.5 |
|
@YangKeao: new pull request created to branch 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 ti-community-infra/tichi repository. |
|
/cherry-pick release-7.5 |
|
@djshow832: new pull request created to branch 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 ti-community-infra/tichi repository. |
…it (pingcap#69694) Keep CheckLongDataSize while using release-7.5 parse.ExecArgs, and drop the master-only planner/core/resolve import. Signed-off-by: djshow832 <zhangming@pingcap.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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
Bug Fixes
COM_STMT_SEND_LONG_DATApayload sizes.MaxAllowedPacket, instead of proceeding with parameter parsing.Tests
MaxAllowedPacketboundary and oversized long-data submissions, including post-error buffer clearing and continued validation success.