ZNCString: guard Replace/Split against empty-width arguments#2015
Conversation
CString::Replace with an empty sReplace underflowed 'p += uReplaceWidth - 1' to SIZE_MAX and then the per-iteration 'p++' brought p back to the same byte, so the function looped forever appending sWith to the output and eventually OOMed. CString::Split with empty sDelim and bAllowEmpty=false spun in the prefix-skip loops because strncasecmp(p, "", 0) is unconditionally 0 and p += 0 never advances. No in-tree caller currently passes the bad argument, but the public library API should not be a bear trap for module authors. Same shape as znc#1994.
|
All these prs need some unit tests |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2015 +/- ##
==========================================
+ Coverage 50.97% 51.23% +0.25%
==========================================
Files 140 141 +1
Lines 30408 30533 +125
Branches 4106 4109 +3
==========================================
+ Hits 15501 15643 +142
+ Misses 14906 14889 -17
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Alexey Sokolov <alexey+github@asokolov.org>
Co-authored-by: Alexey Sokolov <alexey+github@asokolov.org>
|
have you noticed failing CI? |
StringTest.cpp does not include gmock, so EXPECT_THAT/ElementsAre/IsEmpty do not compile and the unittest target fails on every CI configuration. Use EXPECT_EQ against a VCString and EXPECT_TRUE(empty()) instead, which keeps the test scope identical without dragging gmock into this file.
|
Sorry, missed the CI mail. Just pushed a fix: those EXPECT_THAT lines came from your own suggestion, but StringTest.cpp doesn't include gmock, so the file wouldn't compile on any cpp job. Swapped them for EXPECT_EQ against a VCString and EXPECT_TRUE(vempty.empty()) to keep the file gtest-only. Want me to pull gmock in instead and put the matchers back? |
|
Um, sure. I thought it's obvious enough that using gmock requires including gmock :) |
|
The problem with EXPECT_TRUE is that the error message it gives only says "not true", while EXPECT_THAT outputs the actual list of values |
Pull in gmock so the empty-delimiter Split assertions can keep using EXPECT_THAT(..., ElementsAre(...)) and IsEmpty(). On failure the matcher prints the actual vector contents, which EXPECT_TRUE(vempty.empty()) hides behind a bare 'not true'.
|
yeah fair, I overcorrected. should've just pulled in gmock instead of swapping the matchers. point about the failure diagnostics is right too. put the EXPECT_THAT lines back with #include <gmock/gmock.h> and the using ::testing::* in 80699c4, same shape as MessageTest.cpp. |
Close #2009:
CString::Replace with an empty sReplace underflowed 'p += uReplaceWidth - 1' to SIZE_MAX and then the per-iteration 'p++' brought p back to the same byte, so the function looped forever appending sWith to the output and eventually OOMed.
CString::Split with empty sDelim and bAllowEmpty=false spun in the prefix-skip loops because strncasecmp(p, "", 0) is unconditionally 0 and p += 0 never advances.
No in-tree caller currently passes the bad argument, but the public library API should not be a bear trap for module authors. Same shape as #1994.