Skip to content

Commit

Permalink
[AVRO-3945] Add missing bounds check in the loop (#2812)
Browse files Browse the repository at this point in the history
* [AVRO-3945] Add missing bounds checks for extra increments in the loop

This issue was found by cppcheck:

    impl/json/JsonIO.cc:319:66: warning: Missing bounds check for extra iterator increment in loop. [StlMissingComparison]
        for (string::const_iterator it = s.begin(); it != s.end(); ++it) {
                                                                     ^
    impl/json/JsonIO.cc:350:37: note: Missing bounds check for extra iterator increment in loop.
                            char c = *++it;
                                        ^
    impl/json/JsonIO.cc:319:66: note: Missing bounds check for extra iterator increment in loop.
        for (string::const_iterator it = s.begin(); it != s.end(); ++it) {

The original implementation contained a for-loop that incremented an
iterator on each iteration **and** if a backslash was found. This caused
a situtation when a malicious string could cause an invalid memory access,
because the iterator would reach **after** the `s.cend()` due to additional
increments in the loop body.

This commit fixes the issue.

* build.sh: sort unittests and add forgotten tests
  • Loading branch information
mkmkme committed Mar 25, 2024
1 parent 580e60b commit 20772be
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 9 deletions.
13 changes: 8 additions & 5 deletions lang/c++/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,17 @@ case "$target" in
(cmake -S. -Bbuild -D CMAKE_BUILD_TYPE=Debug -D AVRO_ADD_PROTECTOR_FLAGS=1 && cmake --build build \
&& ./build/buffertest \
&& ./build/unittest \
&& ./build/AvrogencppTestReservedWords \
&& ./build/AvrogencppTests \
&& ./build/CodecTests \
&& ./build/CommonsSchemasTests \
&& ./build/CompilerTests \
&& ./build/StreamTests \
&& ./build/SpecificTests \
&& ./build/AvrogencppTests \
&& ./build/DataFileTests \
&& ./build/SchemaTests \
&& ./build/CommonsSchemasTests)
&& ./build/JsonTests \
&& ./build/LargeSchemaTests \
&& ./build/SchemaTests \
&& ./build/SpecificTests \
&& ./build/StreamTests)
;;

xcode-test)
Expand Down
15 changes: 11 additions & 4 deletions lang/c++/impl/json/JsonIO.cc
Original file line number Diff line number Diff line change
Expand Up @@ -316,10 +316,17 @@ JsonParser::Token JsonParser::tryString() {

string JsonParser::decodeString(const string &s, bool binary) {
string result;
for (string::const_iterator it = s.begin(); it != s.end(); ++it) {
char ch = *it;
const auto readNextByte = [](string::const_iterator &it, const string::const_iterator &end) -> char {
if (it == end)
throw Exception("Unexpected EOF");
return *it++;
};
auto it = s.cbegin();
const auto end = s.cend();
while (it != end) {
char ch = *it++;
if (ch == '\\') {
ch = *++it;
ch = readNextByte(it, end);
switch (ch) {
case '"':
case '\\':
Expand Down Expand Up @@ -347,7 +354,7 @@ string JsonParser::decodeString(const string &s, bool binary) {
char e[4];
for (char &i : e) {
n *= 16;
char c = *++it;
char c = readNextByte(it, end);
i = c;
if (isdigit(c)) {
n += c - '0';
Expand Down
1 change: 1 addition & 0 deletions lang/c++/test/JsonTests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ TestData<const char *> stringData[] = {
{R"("\/")", EntityType::String, "/", R"("\/")"},
{R"("\u20ac")", EntityType::String, "\xe2\x82\xac", R"("\u20ac")"},
{R"("\u03c0")", EntityType::String, "\xcf\x80", R"("\u03c0")"},
{R"("hello\n")", EntityType::String, "hello\n", R"("hello\n")"},
};

void testBool(const TestData<bool> &d) {
Expand Down

0 comments on commit 20772be

Please sign in to comment.