From bbc9bd059a55506ed7b020560b30680eceea8fcc Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 23 Jul 2021 20:35:12 -0400 Subject: [PATCH] Some tweaking. --- include/simdjson/generic/atomparsing.h | 30 +++++++----- .../generic/ondemand/value_iterator-inl.h | 6 +-- src/generic/stage2/tape_builder.h | 6 +-- tests/dom/basictests.cpp | 47 ++++++++++++++++++- 4 files changed, 69 insertions(+), 20 deletions(-) diff --git a/include/simdjson/generic/atomparsing.h b/include/simdjson/generic/atomparsing.h index b398aa124..64fb35777 100644 --- a/include/simdjson/generic/atomparsing.h +++ b/include/simdjson/generic/atomparsing.h @@ -15,46 +15,50 @@ simdjson_really_inline uint32_t string_to_uint32(const char* str) { uint32_t val // Again in str4ncmp we use a memcpy to avoid undefined behavior. The memcpy may appear expensive. // Yet all decent optimizing compilers will compile memcpy to a single instruction, just about. +// It is unsafe because you must ensure that you have at least 4 characters before calling this function! simdjson_warn_unused -simdjson_really_inline uint32_t str4ncmp(const uint8_t *src, const char* atom) { +simdjson_really_inline uint32_t str4ncmp_unsafe(const uint8_t *src, const char* atom) { uint32_t srcval; // we want to avoid unaligned 32-bit loads (undefined in C/C++) std::memcpy(&srcval, src, sizeof(uint32_t)); return srcval ^ string_to_uint32(atom); } +// It is unsafe because you must ensure that you have at least 5 characters before calling this function! simdjson_warn_unused -simdjson_really_inline bool is_valid_true_atom(const uint8_t *src) { - return (str4ncmp(src, "true") | jsoncharutils::is_not_structural_or_whitespace(src[4])) == 0; +simdjson_really_inline bool is_valid_true_atom_unsafe(const uint8_t *src) { + return (str4ncmp_unsafe(src, "true") | jsoncharutils::is_not_structural_or_whitespace(src[4])) == 0; } simdjson_warn_unused simdjson_really_inline bool is_valid_true_atom(const uint8_t *src, size_t len) { - if (len > 4) { return is_valid_true_atom(src); } - else if (len == 4) { return !str4ncmp(src, "true"); } + if (len > 4) { return is_valid_true_atom_unsafe(src); } + else if (len == 4) { return !str4ncmp_unsafe(src, "true"); } else { return false; } } +// It is unsafe because you must ensure that you have at least 5 characters before calling this function! simdjson_warn_unused -simdjson_really_inline bool is_valid_false_atom(const uint8_t *src) { - return (str4ncmp(src+1, "alse") | jsoncharutils::is_not_structural_or_whitespace(src[5])) == 0; +simdjson_really_inline bool is_valid_false_atom_unsafe(const uint8_t *src) { + return (str4ncmp_unsafe(src+1, "alse") | jsoncharutils::is_not_structural_or_whitespace(src[5])) == 0; } simdjson_warn_unused simdjson_really_inline bool is_valid_false_atom(const uint8_t *src, size_t len) { - if (len > 5) { return is_valid_false_atom(src); } - else if (len == 5) { return !str4ncmp(src+1, "alse"); } + if (len > 5) { return is_valid_false_atom_unsafe(src); } + else if (len == 5) { return !str4ncmp_unsafe(src+1, "alse"); } else { return false; } } +// It is unsafe because you must ensure that you have at least 5 characters before calling this function! simdjson_warn_unused -simdjson_really_inline bool is_valid_null_atom(const uint8_t *src) { - return (str4ncmp(src, "null") | jsoncharutils::is_not_structural_or_whitespace(src[4])) == 0; +simdjson_really_inline bool is_valid_null_atom_unsafe(const uint8_t *src) { + return (str4ncmp_unsafe(src, "null") | jsoncharutils::is_not_structural_or_whitespace(src[4])) == 0; } simdjson_warn_unused simdjson_really_inline bool is_valid_null_atom(const uint8_t *src, size_t len) { - if (len > 4) { return is_valid_null_atom(src); } - else if (len == 4) { return !str4ncmp(src, "null"); } + if (len > 4) { return is_valid_null_atom_unsafe(src); } + else if (len == 4) { return !str4ncmp_unsafe(src, "null"); } else { return false; } } diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h index 11e6848cf..621090562 100644 --- a/include/simdjson/generic/ondemand/value_iterator-inl.h +++ b/include/simdjson/generic/ondemand/value_iterator-inl.h @@ -451,11 +451,11 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator simdjson_really_inline simdjson_result value_iterator::parse_bool(uint32_t max_len, const uint8_t *json) const noexcept { if (max_len >= 4) { - if (!atomparsing::str4ncmp(json, "true")) { + if (!atomparsing::str4ncmp_unsafe(json, "true")) { if (max_len == 4 || jsoncharutils::is_structural_or_whitespace(json[4])) { return simdjson_result(true); } - } else if (!atomparsing::str4ncmp(json, "fals") && max_len >= 5 && json[4] == 'e') { + } else if (!atomparsing::str4ncmp_unsafe(json, "fals") && max_len >= 5 && json[4] == 'e') { if (max_len == 5 || jsoncharutils::is_structural_or_whitespace(json[5])) { return simdjson_result(false); } @@ -471,7 +471,7 @@ simdjson_really_inline bool value_iterator::is_null() noexcept { simdjson_really_inline bool value_iterator::parse_null(uint32_t max_len, const uint8_t *json) const noexcept { if (max_len >= 4) { - if (!atomparsing::str4ncmp(json, "null")) { + if (!atomparsing::str4ncmp_unsafe(json, "null")) { if (max_len == 4 || jsoncharutils::is_structural_or_whitespace(json[4])) { return true; } diff --git a/src/generic/stage2/tape_builder.h b/src/generic/stage2/tape_builder.h index 5e7cb2411..29911b118 100644 --- a/src/generic/stage2/tape_builder.h +++ b/src/generic/stage2/tape_builder.h @@ -172,7 +172,7 @@ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_root_ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_true_atom(json_iterator &iter, const uint8_t *value) noexcept { iter.log_value("true"); - if (!atomparsing::is_valid_true_atom(value)) { return T_ATOM_ERROR; } + if (!atomparsing::is_valid_true_atom(value, iter.remaining_len())) { return T_ATOM_ERROR; } tape.append(0, internal::tape_type::TRUE_VALUE); return SUCCESS; } @@ -186,7 +186,7 @@ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_root_ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_false_atom(json_iterator &iter, const uint8_t *value) noexcept { iter.log_value("false"); - if (!atomparsing::is_valid_false_atom(value)) { return F_ATOM_ERROR; } + if (!atomparsing::is_valid_false_atom(value, iter.remaining_len())) { return F_ATOM_ERROR; } tape.append(0, internal::tape_type::FALSE_VALUE); return SUCCESS; } @@ -200,7 +200,7 @@ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_root_ simdjson_warn_unused simdjson_really_inline error_code tape_builder::visit_null_atom(json_iterator &iter, const uint8_t *value) noexcept { iter.log_value("null"); - if (!atomparsing::is_valid_null_atom(value)) { return N_ATOM_ERROR; } + if (!atomparsing::is_valid_null_atom(value, iter.remaining_len())) { return N_ATOM_ERROR; } tape.append(0, internal::tape_type::NULL_VALUE); return SUCCESS; } diff --git a/tests/dom/basictests.cpp b/tests/dom/basictests.cpp index 4ac8514ff..4e71e61df 100644 --- a/tests/dom/basictests.cpp +++ b/tests/dom/basictests.cpp @@ -1978,6 +1978,7 @@ namespace to_string_tests { return true; } + bool print_to_string_large_int() { std::cout << "Running " << __func__ << std::endl; dom::parser parser; @@ -2117,6 +2118,49 @@ namespace to_string_tests { } } +bool simple_overflows() { + std::cout << "Running " << __func__ << std::endl; + simdjson::dom::parser parser; + simdjson::dom::element doc; + + for (const char * val : {"[f]", "{\"a\":f}"}) { + char * tmp = new char[strlen(val)]; + memcpy(tmp, val, strlen(val)); + ASSERT_ERROR( parser.parse(tmp, strlen(val)).get(doc), simdjson::F_ATOM_ERROR); + delete[] tmp; + } + + for (const char * val : {"[t]", "{\"a\":t}"}) { + char * tmp = new char[strlen(val)]; + memcpy(tmp, val, strlen(val)); + ASSERT_ERROR( parser.parse(tmp, strlen(val)).get(doc), simdjson::T_ATOM_ERROR); + delete[] tmp; + } + + for (const char * val : {"[n]", "{\"a\":n}"}) { + char * tmp = new char[strlen(val)]; + memcpy(tmp, val, strlen(val)); + ASSERT_ERROR( parser.parse(tmp, strlen(val)).get(doc), simdjson::N_ATOM_ERROR); + delete[] tmp; + } + + for (const char * val : {"[-]", "{\"a\":-}"}) { + char * tmp = new char[strlen(val)]; + memcpy(tmp, val, strlen(val)); + ASSERT_ERROR( parser.parse(tmp, strlen(val)).get(doc), simdjson::NUMBER_ERROR); + delete[] tmp; + } + + ASSERT_ERROR( parser.parse(std::string("[f]")).get(doc), simdjson::F_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("[t]")).get(doc), simdjson::T_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("[n]")).get(doc), simdjson::N_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("[-]")).get(doc), simdjson::NUMBER_ERROR); + ASSERT_ERROR( parser.parse(std::string("{\"a\":f}")).get(doc), simdjson::F_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("{\"a\":t}")).get(doc), simdjson::T_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("{\"a\":n}")).get(doc), simdjson::N_ATOM_ERROR); + ASSERT_ERROR( parser.parse(std::string("{\"a\":-}")).get(doc), simdjson::NUMBER_ERROR); + return true; +} int main(int argc, char *argv[]) { @@ -2154,7 +2198,8 @@ int main(int argc, char *argv[]) { std::cout << "------------------------------------------------------------" << std::endl; std::cout << "Running basic tests." << std::endl; - if (to_string_tests::run() && + if (simple_overflows() && + to_string_tests::run() && validate_tests::run() && minify_tests::run() && parse_api_tests::run() &&