Add empty check before accessing buffer dumpcontext and parseContextStack
#265
Conversation
|
|
WalkthroughThe changes enhance the error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Protocol as TSimpleJSONProtocol
Caller->>Protocol: Invoke method (e.g., OutputPreValue)
Protocol->>Protocol: Check if dumpContext is empty
alt dumpContext empty
Protocol-->>Caller: Return error
else
Protocol->>Protocol: Proceed with method operations
Protocol-->>Caller: Return success/result
end
Caller->>Protocol: Invoke method (e.g., ParsePreValue)
Protocol->>Protocol: Check if parseContextStack is empty
alt parseContextStack empty
Protocol-->>Caller: Return error
else
Protocol->>Protocol: Proceed with parsing operations
Protocol-->>Caller: Return success/result
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (4)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go (4)
589-591: Fix inconsistent indentation.The indentation uses 8 spaces instead of 4 spaces that are used in the rest of the file.
- return errors.New("dumpContext is empty") + return errors.New("dumpContext is empty")
728-730: Fix inconsistent indentation.The indentation uses 8 spaces instead of 4 spaces that are used in the rest of the file.
- return errors.New("dumpContext is empty") + return errors.New("dumpContext is empty")
754-756: Fix inconsistent indentation.The indentation uses 8 spaces instead of 4 spaces that are used in the rest of the file.
- return errors.New("dumpContext is empty") + return errors.New("dumpContext is empty")
846-848: Fix inconsistent indentation.The indentation uses 8 spaces instead of 4 spaces that are used in the rest of the file.
- return errors.New("parseContextStack is empty") + return errors.New("parseContextStack is empty")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go(11 hunks)
🔇 Additional comments (1)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go (1)
568-1380: Good defensive programming approach.The changes add empty checks before accessing the
dumpContextandparseContextStackslices, which helps prevent potential panic errors. This is a good defensive programming practice that makes the code more robust.
| if len(p.dumpContext) == 0 { | ||
| return errors.New("dumpContext is empty") | ||
| } |
There was a problem hiding this comment.
Missing import for the "errors" package.
The code uses errors.New() in multiple places, but the "errors" package is not imported in the import section (lines 22-31). This will cause a compilation error.
@@ -22,10 +22,11 @@
import (
"bufio"
"bytes"
"encoding/base64"
"encoding/json"
+ "errors"
"fmt"
"io"
"math"
"strconv"
)📝 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.
| if len(p.dumpContext) == 0 { | |
| return errors.New("dumpContext is empty") | |
| } | |
| import ( | |
| "bufio" | |
| "bytes" | |
| "encoding/base64" | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "io" | |
| "math" | |
| "strconv" | |
| ) |
There was a problem hiding this comment.
@chengpeng-wang Great! I can see you've fixed the missing import for the "errors" package in your latest commit. The empty checks you've added before accessing the buffers will help prevent potential panics, making the code more robust. The changes look good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go (3)
620-622: Redundant check in OutputBool, but not harmful.This check is technically redundant since
OutputPreValue()already checks ifdumpContextis empty at line 617, and it's called at the beginning of this function. However, having this double check doesn't hurt and provides an extra safety net.
664-666: Check in OutputF64 should be placed earlier.The empty check for
dumpContextis placed in the middle of the function after other logic has been executed. It would be better to check at the beginning of the function (similar to other methods) or immediately before accessing the slice.func (p *TSimpleJSONProtocol) OutputF64(value float64) error { if e := p.OutputPreValue(); e != nil { return e } + if len(p.dumpContext) == 0 { + return errors.New("dumpContext is empty") + } var v string if math.IsNaN(value) { v = string(JSON_QUOTE) + JSON_NAN + string(JSON_QUOTE) } else if math.IsInf(value, 1) { v = string(JSON_QUOTE) + JSON_INFINITY + string(JSON_QUOTE) } else if math.IsInf(value, -1) { v = string(JSON_QUOTE) + JSON_NEGATIVE_INFINITY + string(JSON_QUOTE) } else { v = strconv.FormatFloat(value, 'g', -1, 64) - if len(p.dumpContext) == 0 { - return errors.New("dumpContext is empty") - } switch _ParseContext(p.dumpContext[len(p.dumpContext)-1]) { case _CONTEXT_IN_OBJECT_FIRST, _CONTEXT_IN_OBJECT_NEXT_KEY: v = string(JSON_QUOTE) + v + string(JSON_QUOTE) default: } }
683-685: Redundant check in OutputI64, but not harmful.This check is technically redundant since
OutputPreValue()already checks ifdumpContextis empty at line 680, and it's called at the beginning of this function. However, having this double check doesn't hurt and provides an extra safety net.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go(12 hunks)
🔇 Additional comments (10)
thirdparty/github.com/apache/thrift/lib/go/thrift/simple_json_protocol.go (10)
25-25: Missing import added correctly.The "errors" package has been properly imported to support the
errors.New()function calls added in this PR.
569-571: Good defensive programming in OutputPreValue.Adding a check for empty
dumpContextbefore accessing its elements prevents potential panic situations when the slice is empty.
590-593: Good defensive programming in OutputPostValue.Adding a check for empty
dumpContextbefore accessing its elements prevents potential panic situations when the slice is empty.
729-731: Good defensive programming in OutputObjectEnd.Adding a check for empty
dumpContextbefore accessing its elements prevents potential panic situations when the slice is empty.
755-757: Good defensive programming in OutputListEnd.Adding a check for empty
dumpContextbefore accessing its elements prevents potential panic situations when the slice is empty.
783-785: Good defensive programming in ParsePreValue.Adding a check for empty
parseContextStackbefore accessing its elements prevents potential panic situations when the slice is empty.
847-849: Good defensive programming in ParsePostValue.Adding a check for empty
parseContextStackbefore accessing its elements prevents potential panic situations when the slice is empty.
1030-1032: Good defensive programming in ParseObjectEnd.Adding a check for empty
parseContextStackbefore accessing its elements prevents potential panic situations when the slice is empty.
1095-1097: Good defensive programming in ParseListEnd.Adding a check for empty
parseContextStackbefore accessing its elements prevents potential panic situations when the slice is empty.
569-1097: Consistent error messages across different functions.The error messages are consistently worded across all the empty checks, which is good for maintainability and error handling.
🧰 Tools
🪛 golangci-lint (1.62.2)
579-579: S1023: redundant break statement
(gosimple)
2 participants
Description:
This PR introduces additional checks in several functions within
simple_json_protocol.goto ensure thatdumpContextandparseContextStackare not used in the slicing or accessed in the buffer access operation when they are empty.Impact:
This patch does not alter existing logic but rather introduces defensive checks to make the project more predictable and resilient. In total, there are 21 slicing operations and buffer access operations upon
dumpcontextandparseContextStack. In this way, potential bof(Buffer-Overflow) panic can be avoided. We believe these changes align with best practices for defensive programming and may help streamline debugging and future development.Looking forward to your feedback, and happy to refine the changes if needed. Thanks!
Summary by CodeRabbit