Fixing two buffer overflow issues #592
Conversation
The snprintf using it should work with any int, which means this string has to fit ",-2147483648\0", which is 12 characters + a null terminator. Otherwise it could lead to a buffer overflow, corrupting other variables on the stack, and maybe causing a crash too?
This got broken during a refactoring in de71282. We need the plus 2 items because of the overflow/underflow bucket, without that we can possibly overindex this array and corrupt other counters. In earlier versions we even had the encoding variable stored directly after this array, which possibly caused crashes or incorrectly working string functions.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #592 +/- ##
==========================================
- Coverage 85.48% 85.46% -0.02%
==========================================
Files 3 3
Lines 1343 1342 -1
Branches 215 215
==========================================
- Hits 1148 1147 -1
Misses 92 92
Partials 103 103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| int j; | ||
| char str[1024]; | ||
| char tmp[10]; | ||
| char tmp[13]; |
There was a problem hiding this comment.
I don't think this is a buffer overflow issue as both calls to snprintf that uses it specifies it length as 10. We should however let it write the whole number of course, so this commit should also fix the calls to snprintf below.
Your example would result in tmp containing ",-2147483\0" even after this change, wouldn't it?
AndersAstrand
left a comment
AndersAstrand
left a comment
There was a problem hiding this comment.
We need to update the snprintf() calls to use the whole new tmp buffer, and also update the commit message to not suggest that snprintf() would write outside of the buffer.
| continue; | ||
| } | ||
| snprintf(tmp, 10, ",%d", arr[j]); | ||
| strcat(str, tmp); |
There was a problem hiding this comment.
strcat here, however, can lead to a buffer overflow if len is too large. it should probably be fixed even if len is known to always be small.
3 participants
Correct the size of tmp array
The snprintf using it should work with any int, which means this string
has to fit ",-2147483648\0", which is 12 characters + a null terminator.
Otherwise it could lead to a buffer overflow, corrupting other variables
on the stack, and maybe causing a crash too?
Fix resp_calls array size
This got broken during a refactoring in de71282. We need the plus 2
items because of the overflow/underflow bucket, without that we can
possibly overindex this array and corrupt other counters.
In earlier versions we even had the encoding variable stored directly
after this array, which possibly caused crashes or incorrectly working
string functions.