Fix bugs and add code review documentation #81
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brofield
commented
Nov 13, 2025
SimpleIni.h
Outdated
|
|
||
|
|
||
| // check for integer overflow before allocation | ||
| if (lSize == LONG_MAX || static_cast<size_t>(lSize) > SIZE_MAX - 1) { |
Owner
Author
There was a problem hiding this comment.
No need to check lSize == LONG_MAX, the next test will catch that.
SimpleIni.h
Outdated
| } | ||
|
|
||
| // check for integer overflow before allocation | ||
| if (uLen >= SIZE_MAX / sizeof(SI_CHAR) - 1) { |
Owner
Author
There was a problem hiding this comment.
parenthesis would make it more clear
SimpleIni.h
Outdated
| // temporarily null terminate, convert and output the line | ||
| *const_cast<SI_CHAR*>(pEndOfLine) = 0; | ||
| if (!a_oConverter.ConvertToStore(a_pText)) { | ||
| // calculate line length and create a temporary copy for conversion |
Owner
Author
There was a problem hiding this comment.
The new copy code is unnecessary. ConvertToStore makes a copy.
This commit addresses multiple critical bugs and code quality issues identified during a thorough code review of the SimpleIni library. ## Critical Bug Fixes: 1. **Buffer underflow in IsSingleLineQuotedValue() and IsMultiLineData()** - Fixed pointer decrement without bounds checking (lines 1806, 1841) - Added proper bounds checking to prevent reading before buffer start - Prevents potential crashes and security vulnerabilities 2. **Integer overflow in LoadFile() and LoadData()** - Added overflow checks before memory allocation (lines 1462, 1520) - Added parentheses for clarity in overflow check (line 1520) - Prevents allocation of incorrectly sized buffers - Protects against heap corruption from oversized allocations 3. **Logic error in GetDoubleValue()** - Fixed strtod error checking (line 2267) - Properly detects when no conversion was performed - Ensures invalid numeric strings return default value 4. **Null pointer check in DeleteString()** - Added check for null m_pData before pointer arithmetic (line 2839) - Prevents undefined behavior during string deletion ## High Priority Fixes: 5. **Error handling in AddEntry()** - Added error check after CopyString() call (line 2098) - Prevents use-after-free if memory allocation fails 6. **LoadOrder comparator** - Fixed incorrect comparator usage (line 360) - Now properly calls KeyOrder with Entry objects instead of pointers ## Code Quality Improvements: 7. **Removed unused loop variables** - Cleaned up unused 'n' variable in GetAllSections, GetAllKeys, GetSectionSize - Eliminates compiler warnings and dead code ## Documentation: Added comprehensive CODE_REVIEW_FINDINGS.md document detailing: - 20+ identified issues with severity classifications - Detailed impact analysis for each issue - Specific recommendations for fixes - Testing recommendations - Summary of positive aspects of the codebase ## Review Feedback Addressed: - Removed redundant lSize == LONG_MAX check (line 1462) - Added parentheses for clarity in overflow check (line 1520) - Note: OutputMultiLineText const_cast is safe as ConvertToStore makes internal copy All fixes have been tested to ensure they don't break existing functionality while addressing the identified security and correctness issues.
brofield
force-pushed
the
claude/code-review-details-011CV5PN7p7U5vNSGRmBzKrq
branch
from
f39c754 to
229a429
Compare
brofield
deleted the
claude/code-review-details-011CV5PN7p7U5vNSGRmBzKrq
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit addresses multiple critical bugs and code quality issues identified during a thorough code review of the SimpleIni library.
Critical Bug Fixes:
Buffer underflow in IsSingleLineQuotedValue() and IsMultiLineData()
Integer overflow in LoadFile() and LoadData()
Undefined behavior in OutputMultiLineText()
Logic error in GetDoubleValue()
Null pointer check in DeleteString()
High Priority Fixes:
Error handling in AddEntry()
LoadOrder comparator
Code Quality Improvements:
Documentation:
Added comprehensive CODE_REVIEW_FINDINGS.md document detailing:
All fixes have been tested to ensure they don't break existing functionality while addressing the identified security and correctness issues.