Fix UB in dev checks when iterating empty object (#1587)

When find_field_unordered is used on an empty object, it calls
json_iterator::reenter_child. reenter_child asserts that it doesn't
rewind too far back by consulting parser->start_positions.

When the On Demand parser sees an empty object, it fails to update
parser->start_positions. This means that the assertion in
json_iterator::reenter_child reads stale data, or potentially
uninitialized memory. Reading uninitialized memory can cause spurious
assertion failures and Valgrind memcheck reports:

    Running missing_keys_for_empty_top_level_object ...
    ==170679== Conditional jump or move depends on uninitialised value(s)
    ==170679==    at 0x4943D7: reenter_child (json_iterator-inl.h:208)
    ==170679==    by 0x4943D7: find_field_unordered_raw (value_iterator-inl.h:197)
    ==170679==    by 0x4943D7: find_field_unordered (object-inl.h:13)
    ==170679==    by 0x4943D7: find_field_unordered (object-inl.h:96)
    ==170679==    by 0x4943D7: find_field_unordered (value-inl.h:110)
    ==170679==    by 0x4943D7: find_field_unordered (document-inl.h:105)
    ==170679==    by 0x4943D7: object_tests::missing_keys_for_empty_top_level_object() (ondemand_object_tests.cpp:117)
    ==170679==    by 0x4CA761: object_tests::run() (ondemand_object_tests.cpp:1085)
    ==170679==    by 0x8BA314: int test_main<bool ()>(int, char**, bool ( const&)()) (test_ondemand.h:81)
    ==170679==    by 0x4CA9C8: main (ondemand_object_tests.cpp:1119)
    ==170679==

Fix the read of uninitialized or stale memory by updating
parser->start_positions regardless of whether we see an empty object or
an object with some keys.

This commit only affects builds where development checks
(SIMDJSON_DEVELOPMENT_CHECKS) are enabled. Builds where development
checks are disabled are unaffected by this bug.
This commit is contained in:
strager 2021-05-27 05:34:28 -07:00 committed by GitHub
parent 2ec23bdf37
commit 16e2323153
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 19 additions and 3 deletions

View File

@ -25,6 +25,9 @@ simdjson_warn_unused simdjson_really_inline simdjson_result<bool> value_iterator
simdjson_warn_unused simdjson_really_inline bool value_iterator::started_object() noexcept {
assert_at_container_start();
#ifdef SIMDJSON_DEVELOPMENT_CHECKS
_json_iter->set_start_position(_depth, _start_position);
#endif
if (*_json_iter->peek() == '}') {
logger::log_value(*_json_iter, "empty object");
_json_iter->advance();
@ -32,9 +35,6 @@ simdjson_warn_unused simdjson_really_inline bool value_iterator::started_object(
return false;
}
logger::log_start_value(*_json_iter, "object");
#ifdef SIMDJSON_DEVELOPMENT_CHECKS
_json_iter->set_start_position(_depth, _start_position);
#endif
return true;
}

View File

@ -107,6 +107,21 @@ namespace object_tests {
return true;
}
bool missing_keys_for_empty_top_level_object() {
TEST_START();
simdjson::ondemand::parser parser;
simdjson::padded_string docdata = "{}"_padded;
simdjson::ondemand::document doc;
auto error = parser.iterate(docdata).get(doc);
if(error != simdjson::SUCCESS) { return false; }
error = doc.find_field_unordered("keynotfound").error();
if(error != simdjson::NO_SUCH_FIELD) {
std::cout << error << std::endl;
return false;
}
return true;
}
#if SIMDJSON_EXCEPTIONS
// used in issue_1521
// difficult to use as a lambda because it is recursive.
@ -1067,6 +1082,7 @@ namespace object_tests {
missing_key_continue() &&
no_missing_keys() &&
missing_keys() &&
missing_keys_for_empty_top_level_object() &&
#if SIMDJSON_EXCEPTIONS
fixed_broken_issue_1521() &&
issue_1521() &&