From 16e23231537d9415aec71b94f8a266076452380b Mon Sep 17 00:00:00 2001
From: strager <strager.nds@gmail.com>
Date: Thu, 27 May 2021 05:34:28 -0700
Subject: [PATCH] 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.
---
 .../generic/ondemand/value_iterator-inl.h        |  6 +++---
 tests/ondemand/ondemand_object_tests.cpp         | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h
index e97cb265..007c6d4b 100644
--- a/include/simdjson/generic/ondemand/value_iterator-inl.h
+++ b/include/simdjson/generic/ondemand/value_iterator-inl.h
@@ -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;
 }
 
diff --git a/tests/ondemand/ondemand_object_tests.cpp b/tests/ondemand/ondemand_object_tests.cpp
index 78210a47..b405d493 100644
--- a/tests/ondemand/ondemand_object_tests.cpp
+++ b/tests/ondemand/ondemand_object_tests.cpp
@@ -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() &&