Conversation
Signed-off-by: dongmen <414110582@qq.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
📝 WalkthroughWalkthroughThis pull request fixes a bug in DDLEvent.decodeV1 where MultipleTableInfos were incorrectly parsed when count > 0. The fix adds comprehensive validation, corrects tail layout parsing with proper boundary management, and validates data integrity throughout the decoding process. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
the
size/L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the decodeV1 method in pkg/common/event/ddl_event.go to improve the robustness of DDLEvent data decoding. This includes adding numerous bounds checks and size validations when parsing various data fields like MultipleTableInfos, TableInfo, and DispatcherID from a byte slice, aiming to prevent potential out-of-bounds panics and ensure data integrity. A new test case, TestDDLEventDecodeV1WithMultipleTableInfos, was added in pkg/common/event/ddl_event_test.go to validate the correct decoding of events containing multiple table information entries. The review comments highlight a high-severity Denial of Service vulnerability where the common.UnmarshalJSONToTableInfo function is called without ensuring its input tableInfoDataSize is at least 8 bytes, which could lead to a runtime panic. This issue is present in both the multiple table info decoding loop and the single table info decoding, requiring additional checks to prevent these panics.
Signed-off-by: dongmen <414110582@qq.com>
Signed-off-by: dongmen <414110582@qq.com>
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
| } | ||
| end -= 8 | ||
|
|
||
| t.MultipleTableInfos = t.MultipleTableInfos[:0] |
There was a problem hiding this comment.
| t.MultipleTableInfos = t.MultipleTableInfos[:0] |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898 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 |
[LGTM Timeline notifier]Timeline:
|
![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
🧹 Nitpick comments (1)
pkg/common/event/ddl_event_test.go (1)
165-170: Add a reuse case that shrinks the decoded payload.Re-unmarshalling the same bytes only proves we do not append duplicates. It does not cover the new
t.TableInfo = nilbranch or shrinkingMultipleTableInfosfrom N to 0/1, which are the other stale-state cases this change is trying to prevent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/ddl_event_test.go` around lines 165 - 170, Add a reuse test that verifies unmarshalling can shrink fields: after the existing reuse assertions, reassign reverseEvent to a state with non-nil MultipleTableInfos (or call Unmarshal once with >1 entries), then Unmarshal bytes that encode fewer entries (including a message where t.TableInfo == nil) and assert that reverseEvent.MultipleTableInfos length shrinks to the expected smaller size (0 or 1) and that any previously present TableInfo was cleared; target the reverseEvent.Unmarshal call and the MultipleTableInfos and t.TableInfo fields to ensure the nil/resize branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/event/ddl_event.go`:
- Around line 347-360: The decode/reset path clears TableInfo and
MultipleTableInfos but fails to clear t.PostTxnFlushed, allowing callbacks from
a previous DDLEvent to survive; add a reset for t.PostTxnFlushed (e.g. set
t.PostTxnFlushed = nil or t.PostTxnFlushed = t.PostTxnFlushed[:0] as
appropriate) alongside the existing clears (near where t.MultipleTableInfos is
reset) so reused DDLEvent instances don't retain prior callbacks.
- Around line 415-417: The json.Unmarshal error returned from
json.Unmarshal(data[:restDataEnd], t) should be wrapped with a stack trace using
errors.Trace(err) and the package github.com/pingcap/errors must be imported;
update the error handling so that instead of returning the raw err you return
errors.Trace(err) (referencing json.Unmarshal, the t variable and
data[:restDataEnd]) and add the import for github.com/pingcap/errors to match
the pattern used in peer files like mounter.go.
---
Nitpick comments:
In `@pkg/common/event/ddl_event_test.go`:
- Around line 165-170: Add a reuse test that verifies unmarshalling can shrink
fields: after the existing reuse assertions, reassign reverseEvent to a state
with non-nil MultipleTableInfos (or call Unmarshal once with >1 entries), then
Unmarshal bytes that encode fewer entries (including a message where t.TableInfo
== nil) and assert that reverseEvent.MultipleTableInfos length shrinks to the
expected smaller size (0 or 1) and that any previously present TableInfo was
cleared; target the reverseEvent.Unmarshal call and the MultipleTableInfos and
t.TableInfo fields to ensure the nil/resize branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cee430a-9a1f-41e1-880b-2d095fb80e5c
📒 Files selected for processing (4)
pkg/common/event/ddl_event.gopkg/common/event/ddl_event_test.gopkg/common/table_info.gopkg/common/table_info_test.go
| t.eventSize = int64(len(data)) | ||
|
|
||
| end := len(data) | ||
| multipleTableInfosDataSize := binary.BigEndian.Uint64(data[end-8 : end]) | ||
| for i := 0; i < int(multipleTableInfosDataSize); i++ { | ||
| tableInfoDataSize := binary.BigEndian.Uint64(data[end-8 : end]) | ||
| tableInfoData := data[end-8-int(tableInfoDataSize) : end-8] | ||
| info, err := common.UnmarshalJSONToTableInfo(tableInfoData) | ||
| if err != nil { | ||
| return err | ||
| if end < 8 { | ||
| return fmt.Errorf("invalid DDLEvent data: length %d is too short", len(data)) | ||
| } | ||
|
|
||
| multipleTableInfoCount := binary.BigEndian.Uint64(data[end-8 : end]) | ||
| if multipleTableInfoCount > uint64((end-8)/8) { | ||
| return fmt.Errorf("invalid DDLEvent data: too many multiple table infos, count=%d", multipleTableInfoCount) | ||
| } | ||
| end -= 8 | ||
|
|
||
| t.MultipleTableInfos = t.MultipleTableInfos[:0] |
There was a problem hiding this comment.
Clear PostTxnFlushed on reused receivers.
This reset path handles TableInfo and MultipleTableInfos, but PostTxnFlushed is also excluded from JSON and will survive into the next decode. If the same DDLEvent instance is recycled, a later PostFlush() can execute callbacks from the previous event.
🧹 Proposed fix
func (t *DDLEvent) decodeV1(data []byte) error {
// restData | dispatcherIDData | dispatcherIDDataSize | tableInfoData | tableInfoDataSize | multipleTableInfos | multipleTableInfosDataSize
t.eventSize = int64(len(data))
+ t.PostTxnFlushed = t.PostTxnFlushed[:0]
end := len(data)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.eventSize = int64(len(data)) | |
| end := len(data) | |
| multipleTableInfosDataSize := binary.BigEndian.Uint64(data[end-8 : end]) | |
| for i := 0; i < int(multipleTableInfosDataSize); i++ { | |
| tableInfoDataSize := binary.BigEndian.Uint64(data[end-8 : end]) | |
| tableInfoData := data[end-8-int(tableInfoDataSize) : end-8] | |
| info, err := common.UnmarshalJSONToTableInfo(tableInfoData) | |
| if err != nil { | |
| return err | |
| if end < 8 { | |
| return fmt.Errorf("invalid DDLEvent data: length %d is too short", len(data)) | |
| } | |
| multipleTableInfoCount := binary.BigEndian.Uint64(data[end-8 : end]) | |
| if multipleTableInfoCount > uint64((end-8)/8) { | |
| return fmt.Errorf("invalid DDLEvent data: too many multiple table infos, count=%d", multipleTableInfoCount) | |
| } | |
| end -= 8 | |
| t.MultipleTableInfos = t.MultipleTableInfos[:0] | |
| t.eventSize = int64(len(data)) | |
| t.PostTxnFlushed = t.PostTxnFlushed[:0] | |
| end := len(data) | |
| if end < 8 { | |
| return fmt.Errorf("invalid DDLEvent data: length %d is too short", len(data)) | |
| } | |
| multipleTableInfoCount := binary.BigEndian.Uint64(data[end-8 : end]) | |
| if multipleTableInfoCount > uint64((end-8)/8) { | |
| return fmt.Errorf("invalid DDLEvent data: too many multiple table infos, count=%d", multipleTableInfoCount) | |
| } | |
| end -= 8 | |
| t.MultipleTableInfos = t.MultipleTableInfos[:0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/event/ddl_event.go` around lines 347 - 360, The decode/reset path
clears TableInfo and MultipleTableInfos but fails to clear t.PostTxnFlushed,
allowing callbacks from a previous DDLEvent to survive; add a reset for
t.PostTxnFlushed (e.g. set t.PostTxnFlushed = nil or t.PostTxnFlushed =
t.PostTxnFlushed[:0] as appropriate) alongside the existing clears (near where
t.MultipleTableInfos is reset) so reused DDLEvent instances don't retain prior
callbacks.
| err = json.Unmarshal(data[:restDataEnd], t) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file around lines 415-417
head -n 425 pkg/common/event/ddl_event.go | tail -n 30Repository: pingcap/ticdc
Length of output: 838
🏁 Script executed:
# Also check if errors package is already imported
grep -n "^import\|errors" pkg/common/event/ddl_event.go | head -20Repository: pingcap/ticdc
Length of output: 111
🏁 Script executed:
# Search for json.Unmarshal usage in the file
grep -n "json.Unmarshal" pkg/common/event/ddl_event.goRepository: pingcap/ticdc
Length of output: 105
🏁 Script executed:
# Search for errors.Trace usage pattern in the file
grep -n "errors.Trace\|errors.WrapError" pkg/common/event/ddl_event.goRepository: pingcap/ticdc
Length of output: 39
🏁 Script executed:
# Check the full import section
head -n 30 pkg/common/event/ddl_event.goRepository: pingcap/ticdc
Length of output: 810
🏁 Script executed:
# Search for errors.Trace usage in the codebase to see the pattern
rg "errors\.Trace|errors\.WrapError" -A 1 -B 1 --type go | head -50Repository: pingcap/ticdc
Length of output: 1710
🏁 Script executed:
# Check if github.com/pingcap/errors is imported anywhere in this directory
rg "github.com/pingcap/errors" pkg/common/event/ --type goRepository: pingcap/ticdc
Length of output: 298
🏁 Script executed:
# Look for how other json.Unmarshal calls are handled in this directory
rg "json\.Unmarshal" pkg/common/event/ -A 2 --type goRepository: pingcap/ticdc
Length of output: 389
Wrap the JSON decode error with a stack trace and add missing import.
Line 415-417 returns the json.Unmarshal error unwrapped. Import github.com/pingcap/errors and wrap with errors.Trace(err) to maintain consistency with error handling patterns in peer files like mounter.go in this directory and conform to the coding guideline for third-party library errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/event/ddl_event.go` around lines 415 - 417, The json.Unmarshal
error returned from json.Unmarshal(data[:restDataEnd], t) should be wrapped with
a stack trace using errors.Trace(err) and the package github.com/pingcap/errors
must be imported; update the error handling so that instead of returning the raw
err you return errors.Trace(err) (referencing json.Unmarshal, the t variable and
data[:restDataEnd]) and add the import for github.com/pingcap/errors to match
the pattern used in peer files like mounter.go.
|
/test all |
2 participants
What problem does this PR solve?
Issue Number: close #4415
What is changed and how it works?
This PR fixes a correctness bug in pkg/common/event/DDLEvent.decodeV1 that appears when MultipleTableInfos is non-empty.
The previous implementation decoded the tail layout with an incorrect offset sequence: after reading the final count field, it did not advance the cursor before reading each table-info size. As a result, the decoder could treat the count as a size, mis-parse payload boundaries, and potentially return errors or corrupted data in multi-table DDL scenarios.
What changed
invalid slicing.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Bug Fixes
Tests