From 22b3ea93a872668da51721c285b2b95ab475ed82 Mon Sep 17 00:00:00 2001 From: John Keiser Date: Sun, 24 Jan 2021 18:25:34 -0800 Subject: [PATCH 1/3] Emit an error if user tries to iterate arrays out of order --- include/simdjson/error.h | 1 + .../simdjson/generic/ondemand/object-inl.h | 1 + .../generic/ondemand/value_iterator-inl.h | 23 +++++++++++++------ .../generic/ondemand/value_iterator.h | 2 +- src/internal/error_tables.cpp | 3 ++- 5 files changed, 21 insertions(+), 9 deletions(-) diff --git a/include/simdjson/error.h b/include/simdjson/error.h index e4792c083..6030cbccb 100644 --- a/include/simdjson/error.h +++ b/include/simdjson/error.h @@ -35,6 +35,7 @@ enum error_code { INVALID_URI_FRAGMENT, ///< Invalid URI fragment UNEXPECTED_ERROR, ///< indicative of a bug in simdjson PARSER_IN_USE, ///< parser is already in use. + OUT_OF_ORDER_ITERATION, ///< tried to iterate an array or object out of order /** @private Number of error codes */ NUM_ERROR_CODES }; diff --git a/include/simdjson/generic/ondemand/object-inl.h b/include/simdjson/generic/ondemand/object-inl.h index 8e5785ba8..755368627 100644 --- a/include/simdjson/generic/ondemand/object-inl.h +++ b/include/simdjson/generic/ondemand/object-inl.h @@ -52,6 +52,7 @@ simdjson_really_inline object::object(const value_iterator &_iter) noexcept } simdjson_really_inline object_iterator object::begin() noexcept { + // TODO make it a runtime error if this is violated // Expanded version of SIMDJSON_ASSUME( iter.at_field_start() || !iter.is_open() ) SIMDJSON_ASSUME( (iter._json_iter->token.index == iter._start_position + 1) || (iter._json_iter->_depth < iter._depth) ); return iter; diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h index fd21ea0bd..c3b182c31 100644 --- a/include/simdjson/generic/ondemand/value_iterator-inl.h +++ b/include/simdjson/generic/ondemand/value_iterator-inl.h @@ -10,7 +10,9 @@ simdjson_really_inline value_iterator::value_iterator(json_iterator *json_iter, } simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator::start_object() noexcept { - if (*advance_container_start("object") != '{') { return incorrect_type_error("Not an object"); } + const uint8_t *json; + SIMDJSON_TRY( advance_container_start("object", json) ); + if (*json != '{') { return incorrect_type_error("Not an object"); } return started_object(); } @@ -248,7 +250,9 @@ simdjson_warn_unused simdjson_really_inline error_code value_iterator::field_val } simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator::start_array() noexcept { - if (*advance_container_start("array") != '[') { return incorrect_type_error("Not an array"); } + const uint8_t *json; + SIMDJSON_TRY( advance_container_start("array", json) ); + if (*json != '[') { return incorrect_type_error("Not an array"); } return started_array(); } @@ -428,15 +432,20 @@ simdjson_really_inline const uint8_t *value_iterator::advance_start(const char * _json_iter->ascend_to(depth()-1); return result; } -simdjson_really_inline const uint8_t *value_iterator::advance_container_start(const char *type) const noexcept { - // If we're not at the position anymore, we don't want to advance the cursor. - if (is_at_container_start()) { return peek_start(); } - +simdjson_really_inline error_code value_iterator::advance_container_start(const char *type, const uint8_t *&json) const noexcept { logger::log_start_value(*_json_iter, _start_position, depth(), type); + // If we're not at the position anymore, we don't want to advance the cursor. + if (!is_at_start()) { + if (!is_at_container_start()) { return OUT_OF_ORDER_ITERATION; } + json = peek_start(); + return SUCCESS; + } + // Get the JSON and advance the cursor, decreasing depth to signify that we have retrieved the value. assert_at_start(); - return _json_iter->advance(); + json = _json_iter->advance(); + return SUCCESS; } simdjson_really_inline const uint8_t *value_iterator::advance_root_scalar(const char *type) const noexcept { logger::log_value(*_json_iter, _start_position, depth(), type); diff --git a/include/simdjson/generic/ondemand/value_iterator.h b/include/simdjson/generic/ondemand/value_iterator.h index a11d1e2ec..14dfbf4f8 100644 --- a/include/simdjson/generic/ondemand/value_iterator.h +++ b/include/simdjson/generic/ondemand/value_iterator.h @@ -274,7 +274,7 @@ protected: simdjson_really_inline const uint8_t *peek_start() const noexcept; simdjson_really_inline uint32_t peek_start_length() const noexcept; simdjson_really_inline const uint8_t *advance_start(const char *type) const noexcept; - simdjson_really_inline const uint8_t *advance_container_start(const char *type) const noexcept; + simdjson_really_inline error_code advance_container_start(const char *type, const uint8_t *&json) const noexcept; simdjson_really_inline const uint8_t *advance_root_scalar(const char *type) const noexcept; simdjson_really_inline const uint8_t *advance_non_root_scalar(const char *type) const noexcept; diff --git a/src/internal/error_tables.cpp b/src/internal/error_tables.cpp index c08bb6c75..f0d5e7b0d 100644 --- a/src/internal/error_tables.cpp +++ b/src/internal/error_tables.cpp @@ -28,7 +28,8 @@ namespace internal { { INVALID_JSON_POINTER, "Invalid JSON pointer syntax." }, { INVALID_URI_FRAGMENT, "Invalid URI fragment syntax." }, { UNEXPECTED_ERROR, "Unexpected error, consider reporting this problem as you may have found a bug in simdjson" }, - { PARSER_IN_USE, "Cannot parse a new document while a document is still in use." } + { PARSER_IN_USE, "Cannot parse a new document while a document is still in use." }, + { OUT_OF_ORDER_ITERATION, "Objects and arrays can only be iterated when they are first encountered." } }; // error_messages[] } // namespace internal From c5b44f44f97c81bade849186287e127c4432a992 Mon Sep 17 00:00:00 2001 From: John Keiser Date: Sun, 24 Jan 2021 19:44:45 -0800 Subject: [PATCH 2/3] Add partial out-of-order check for field lookup --- include/simdjson/generic/ondemand/value_iterator-inl.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h index c3b182c31..1bbc1d478 100644 --- a/include/simdjson/generic/ondemand/value_iterator-inl.h +++ b/include/simdjson/generic/ondemand/value_iterator-inl.h @@ -68,7 +68,13 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator // ^ (depth 0, index 2) // ``` // - if (!is_open()) { return false; } + if (!is_open()) { + // If we're past the end of the object, we're being iterated out of order. + // Note: this isn't perfect detection. It's possible the user is inside some other object; if so, + // this object iterator will blithely scan that object for fields. + if (_json_iter->depth() < depth() - 1) { return OUT_OF_ORDER_ITERATION; } + return false; + } if (at_first_field()) { has_value = true; From 1bfbb6448a0d36cb980ec2fd8d3150672c5c7ea2 Mon Sep 17 00:00:00 2001 From: John Keiser Date: Tue, 26 Jan 2021 20:49:14 -0800 Subject: [PATCH 3/3] Check out-of-order error in object index --- include/simdjson/generic/ondemand/array-inl.h | 9 +- include/simdjson/generic/ondemand/array.h | 4 +- .../simdjson/generic/ondemand/object-inl.h | 12 +- include/simdjson/generic/ondemand/object.h | 4 +- .../generic/ondemand/value_iterator-inl.h | 15 +- tests/ondemand/ondemand_error_tests.cpp | 141 +++++++++++++++++- 6 files changed, 162 insertions(+), 23 deletions(-) diff --git a/include/simdjson/generic/ondemand/array-inl.h b/include/simdjson/generic/ondemand/array-inl.h index f366e3bdc..3d11c5271 100644 --- a/include/simdjson/generic/ondemand/array-inl.h +++ b/include/simdjson/generic/ondemand/array-inl.h @@ -55,11 +55,12 @@ simdjson_really_inline array array::started(value_iterator &iter) noexcept { return array(iter); } -simdjson_really_inline array_iterator array::begin() noexcept { - return iter; +simdjson_really_inline simdjson_result array::begin() noexcept { + if (!iter.is_at_container_start()) { return OUT_OF_ORDER_ITERATION; } + return array_iterator(iter); } -simdjson_really_inline array_iterator array::end() noexcept { - return {}; +simdjson_really_inline simdjson_result array::end() noexcept { + return array_iterator(); } } // namespace ondemand diff --git a/include/simdjson/generic/ondemand/array.h b/include/simdjson/generic/ondemand/array.h index 60f398527..867d93af6 100644 --- a/include/simdjson/generic/ondemand/array.h +++ b/include/simdjson/generic/ondemand/array.h @@ -24,13 +24,13 @@ public: * * Part of the std::iterable interface. */ - simdjson_really_inline array_iterator begin() noexcept; + simdjson_really_inline simdjson_result begin() noexcept; /** * Sentinel representing the end of the array. * * Part of the std::iterable interface. */ - simdjson_really_inline array_iterator end() noexcept; + simdjson_really_inline simdjson_result end() noexcept; protected: /** diff --git a/include/simdjson/generic/ondemand/object-inl.h b/include/simdjson/generic/ondemand/object-inl.h index 755368627..779542816 100644 --- a/include/simdjson/generic/ondemand/object-inl.h +++ b/include/simdjson/generic/ondemand/object-inl.h @@ -51,14 +51,12 @@ simdjson_really_inline object::object(const value_iterator &_iter) noexcept { } -simdjson_really_inline object_iterator object::begin() noexcept { - // TODO make it a runtime error if this is violated - // Expanded version of SIMDJSON_ASSUME( iter.at_field_start() || !iter.is_open() ) - SIMDJSON_ASSUME( (iter._json_iter->token.index == iter._start_position + 1) || (iter._json_iter->_depth < iter._depth) ); - return iter; +simdjson_really_inline simdjson_result object::begin() noexcept { + if (!iter.is_at_container_start()) { return OUT_OF_ORDER_ITERATION; } + return object_iterator(iter); } -simdjson_really_inline object_iterator object::end() noexcept { - return {}; +simdjson_really_inline simdjson_result object::end() noexcept { + return object_iterator(); } } // namespace ondemand diff --git a/include/simdjson/generic/ondemand/object.h b/include/simdjson/generic/ondemand/object.h index f4a57db3c..23e49090f 100644 --- a/include/simdjson/generic/ondemand/object.h +++ b/include/simdjson/generic/ondemand/object.h @@ -16,8 +16,8 @@ public: */ simdjson_really_inline object() noexcept = default; - simdjson_really_inline object_iterator begin() noexcept; - simdjson_really_inline object_iterator end() noexcept; + simdjson_really_inline simdjson_result begin() noexcept; + simdjson_really_inline simdjson_result end() noexcept; /** * Look up a field by name on an object (order-sensitive). diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h index 1bbc1d478..d16819b11 100644 --- a/include/simdjson/generic/ondemand/value_iterator-inl.h +++ b/include/simdjson/generic/ondemand/value_iterator-inl.h @@ -58,6 +58,10 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator // { "a": [ 1, 2 ], "b": [ 3, 4 ] } // ^ (depth 2, index 1) // ``` + // + if (at_first_field()) { + has_value = true; + // // 2. When a previous search did not yield a value or the object is empty: // @@ -68,15 +72,12 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator // ^ (depth 0, index 2) // ``` // - if (!is_open()) { + } else if (!is_open()) { // If we're past the end of the object, we're being iterated out of order. // Note: this isn't perfect detection. It's possible the user is inside some other object; if so, // this object iterator will blithely scan that object for fields. if (_json_iter->depth() < depth() - 1) { return OUT_OF_ORDER_ITERATION; } - return false; - } - if (at_first_field()) { - has_value = true; + has_value = false; // 3. When a previous search found a field or an iterator yielded a value: // @@ -146,6 +147,10 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator // ``` // } else if (!is_open()) { + // If we're past the end of the object, we're being iterated out of order. + // Note: this isn't perfect detection. It's possible the user is inside some other object; if so, + // this object iterator will blithely scan that object for fields. + if (_json_iter->depth() < depth() - 1) { return OUT_OF_ORDER_ITERATION; } has_value = false; // 3. When a previous search found a field or an iterator yielded a value: diff --git a/tests/ondemand/ondemand_error_tests.cpp b/tests/ondemand/ondemand_error_tests.cpp index 5783ad50d..d4bbe0a0d 100644 --- a/tests/ondemand/ondemand_error_tests.cpp +++ b/tests/ondemand/ondemand_error_tests.cpp @@ -562,10 +562,140 @@ namespace error_tests { ASSERT_EQUAL( val.is_null(), true ); TEST_SUCCEED(); })); + TEST_SUCCEED(); + } - // - // Do it again for bool - // + bool out_of_order_array_iteration_error() { + TEST_START(); + auto json = R"([ [ 1, 2 ] ])"_padded; + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + for (auto subelement : element) { ASSERT_SUCCESS(subelement); } + ASSERT_ERROR( element.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + SUBTEST("value", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + ondemand::value val; + ASSERT_SUCCESS( element.get(val) ); + for (auto subelement : val) { ASSERT_SUCCESS(subelement); } + ASSERT_ERROR( val.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + auto arr = element.get_array(); + for (auto subelement : arr) { ASSERT_SUCCESS(subelement); } + ASSERT_ERROR( arr.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + SUBTEST("array", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + ondemand::array arr; + ASSERT_SUCCESS( element.get(arr) ); + for (auto subelement : arr) { ASSERT_SUCCESS(subelement); } + ASSERT_ERROR( arr.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + TEST_SUCCEED(); + } + + bool out_of_order_object_iteration_error() { + TEST_START(); + auto json = R"([ { "x": 1, "y": 2 } ])"_padded; + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + auto obj = element.get_object(); + for (auto field : obj) { ASSERT_SUCCESS(field); } + ASSERT_ERROR( obj.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + SUBTEST("object", test_ondemand_doc(json, [&](auto doc) { + for (auto element : doc) { + ondemand::object obj; + ASSERT_SUCCESS( element.get(obj) ); + for (auto field : obj) { ASSERT_SUCCESS(field); } + ASSERT_ERROR( obj.begin(), OUT_OF_ORDER_ITERATION ); + } + return true; + })); + TEST_SUCCEED(); + } + + bool out_of_order_object_index_error() { + TEST_START(); + auto json = R"([ { "x": 1, "y": 2 } ])"_padded; + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + simdjson_result obj; + for (auto element : doc) { + obj = element.get_object(); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj["x"], OUT_OF_ORDER_ITERATION ); + return true; + })); + SUBTEST("object", test_ondemand_doc(json, [&](auto doc) { + ondemand::object obj; + for (auto element : doc) { + ASSERT_SUCCESS( element.get(obj) ); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj["x"], OUT_OF_ORDER_ITERATION ); + return true; + })); + TEST_SUCCEED(); + } + + bool out_of_order_object_find_field_error() { + TEST_START(); + auto json = R"([ { "x": 1, "y": 2 } ])"_padded; + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + simdjson_result obj; + for (auto element : doc) { + obj = element.get_object(); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj.find_field("x"), OUT_OF_ORDER_ITERATION ); + return true; + })); + SUBTEST("object", test_ondemand_doc(json, [&](auto doc) { + ondemand::object obj; + for (auto element : doc) { + ASSERT_SUCCESS( element.get(obj) ); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj.find_field("x"), OUT_OF_ORDER_ITERATION ); + return true; + })); + TEST_SUCCEED(); + } + + bool out_of_order_object_find_field_unordered_error() { + TEST_START(); + auto json = R"([ { "x": 1, "y": 2 } ])"_padded; + SUBTEST("simdjson_result", test_ondemand_doc(json, [&](auto doc) { + simdjson_result obj; + for (auto element : doc) { + obj = element.get_object(); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj.find_field_unordered("x"), OUT_OF_ORDER_ITERATION ); + return true; + })); + SUBTEST("object", test_ondemand_doc(json, [&](auto doc) { + ondemand::object obj; + for (auto element : doc) { + ASSERT_SUCCESS( element.get(obj) ); + for (auto field : obj) { ASSERT_SUCCESS(field); } + } + ASSERT_ERROR( obj.find_field_unordered("x"), OUT_OF_ORDER_ITERATION ); + return true; + })); TEST_SUCCEED(); } @@ -588,6 +718,11 @@ namespace error_tests { object_lookup_miss_next_error() && get_fail_then_succeed_bool() && get_fail_then_succeed_null() && + out_of_order_array_iteration_error() && + out_of_order_object_iteration_error() && + out_of_order_object_index_error() && + out_of_order_object_find_field_error() && + out_of_order_object_find_field_unordered_error() && true; } }