diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml index c7804079c..d8c27c621 100644 --- a/.github/workflows/ubuntu.yml +++ b/.github/workflows/ubuntu.yml @@ -100,7 +100,7 @@ jobs: container: ubuntu:focal strategy: matrix: - target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls, ci_test_disableenumserialization, ci_test_skiplibraryversioncheck, ci_test_simdutf, ci_test_strict_nul_handling] + target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls, ci_test_disableenumserialization, ci_test_skiplibraryversioncheck, ci_test_simdutf, ci_test_strict_nul_handling, ci_test_no_thread_local] steps: - name: Install build-essential run: apt-get update ; apt-get install -y build-essential unzip wget git libssl-dev diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 068c5a0f1..99a8aa3ca 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -158,6 +158,10 @@ jobs: # to fit: IMAGE_REL_AMD64_SECREL against `.debug_line'" because the # MinGW linker cannot relocate the debug sections this test produces. # The tests are only built and run here, so the debug info is not used. + # Do not add -O1 here to shrink the objects further: it does make them + # link, but the binaries clang 11.0.1 and clang 18.1.8 then produce crash + # before doctest prints its first line - 39 of 102 tests on clang 18. + # Keep the objects small by splitting the test files instead. - name: Run CMake run: cmake -S . -B build ^ -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang++.exe" ^ diff --git a/cmake/ci.cmake b/cmake/ci.cmake index 2d4bbf6d2..f854138b6 100644 --- a/cmake/ci.cmake +++ b/cmake/ci.cmake @@ -311,6 +311,25 @@ add_custom_target(ci_test_skiplibraryversioncheck COMMENT "Compile and run a translation unit simulating a mismatched library version, with JSON_SKIP_LIBRARY_VERSION_CHECK defined" ) +############################################################################### +# Disable thread-local storage. +############################################################################### + +# Without thread-local storage, the copy constructor cannot bound its descent +# and copies every object and array without the call stack. That path is +# otherwise only reached by values nested deeper than the bound, so this target +# is what runs the whole test suite through it. +add_custom_target(ci_test_no_thread_local + COMMAND ${CMAKE_COMMAND} + -DCMAKE_BUILD_TYPE=Debug -GNinja + -DJSON_BuildTests=ON + -DCMAKE_CXX_FLAGS=-DJSON_NO_THREAD_LOCAL + -S${PROJECT_SOURCE_DIR} -B${PROJECT_BINARY_DIR}/build_no_thread_local + COMMAND ${CMAKE_COMMAND} --build ${PROJECT_BINARY_DIR}/build_no_thread_local + COMMAND cd ${PROJECT_BINARY_DIR}/build_no_thread_local && ${CMAKE_CTEST_COMMAND} --parallel ${N} --output-on-failure + COMMENT "Compile and test without thread-local storage" +) + ############################################################################### # Coverage. ############################################################################### diff --git a/docs/mkdocs/docs/api/macros/index.md b/docs/mkdocs/docs/api/macros/index.md index ca3af720c..70f02a7a6 100644 --- a/docs/mkdocs/docs/api/macros/index.md +++ b/docs/mkdocs/docs/api/macros/index.md @@ -27,6 +27,7 @@ header. See also the [macro overview page](../../features/macros.md). - [**JSON_HAS_STD_FORMAT**](json_has_std_format.md) - control `std::format`/`std::formatter` support - [**JSON_HAS_THREE_WAY_COMPARISON**](json_has_three_way_comparison.md) - control 3-way comparison support - [**JSON_NO_IO**](json_no_io.md) - switch off functions relying on certain C++ I/O headers +- [**JSON_NO_THREAD_LOCAL**](json_no_thread_local.md) - switch off the use of `thread_local` storage - [**JSON_SKIP_UNSUPPORTED_COMPILER_CHECK**](json_skip_unsupported_compiler_check.md) - do not warn about unsupported compilers - [**JSON_USE_GLOBAL_UDLS**](json_use_global_udls.md) - place user-defined string literals (UDLs) into the global namespace - [**JSON_USE_SIMDUTF**](json_use_simdutf.md) - use the simdutf library to accelerate UTF-8 validation diff --git a/docs/mkdocs/docs/api/macros/json_no_thread_local.md b/docs/mkdocs/docs/api/macros/json_no_thread_local.md new file mode 100644 index 000000000..126116ec3 --- /dev/null +++ b/docs/mkdocs/docs/api/macros/json_no_thread_local.md @@ -0,0 +1,47 @@ +# JSON_NO_THREAD_LOCAL + +```cpp +#define JSON_NO_THREAD_LOCAL +``` + +When defined, the library does not use `#!cpp thread_local` storage. This is relevant for the few environments whose +toolchain does not support it. + +The copy constructor copies the first levels of a value by copying the containers, which copy their elements, and +completes whatever is nested deeper than that without the call stack, so that copying a value cannot exhaust the stack +however deeply it is nested. It counts the levels it has descended into in a `#!cpp thread_local` variable, as a counter +shared between threads would be raced. + +Without that counter, no descent can be bounded safely, so objects and arrays are copied without the call stack right +away. Copying keeps working exactly as it does otherwise - the same values come out, and deeply nested values are copied +just as safely - but copying is slower, because the containers no longer copy themselves. Copying the benchmark +documents takes 9% (`canada.json`) to 34% (`twitter.json`) longer; values built mostly from objects are affected the +most. + +## Default definition + +By default, `#!cpp JSON_NO_THREAD_LOCAL` is not defined. + +```cpp +#undef JSON_NO_THREAD_LOCAL +``` + +The library defines it by itself for Clang targeting MinGW, which does not survive the `#!cpp thread_local` storage: +copying a value segfaults there, with both old and current Clang versions, while GCC targeting MinGW is unaffected. + +## Examples + +??? example + + The code below forces the library not to use `#!cpp thread_local` storage. + + ```cpp + #define JSON_NO_THREAD_LOCAL 1 + #include + + ... + ``` + +## Version history + +- Added in version 3.12.1. diff --git a/docs/mkdocs/docs/features/macros.md b/docs/mkdocs/docs/features/macros.md index 927e00df3..e7baba0ae 100644 --- a/docs/mkdocs/docs/features/macros.md +++ b/docs/mkdocs/docs/features/macros.md @@ -91,6 +91,13 @@ security reasons (e.g., Intel Software Guard Extensions (SGX)). See [full documentation of `JSON_NO_IO`](../api/macros/json_no_io.md). +## `JSON_NO_THREAD_LOCAL` + +When defined, the library does not use `#!cpp thread_local` storage. Copying a value then always avoids the call stack +rather than descending into a bounded number of levels first, which is slower but yields the same values. + +See [full documentation of `JSON_NO_THREAD_LOCAL`](../api/macros/json_no_thread_local.md). + ## `JSON_SKIP_LIBRARY_VERSION_CHECK` When defined, the library will not create a compiler warning when a different version of the library was already diff --git a/docs/mkdocs/mkdocs.yml b/docs/mkdocs/mkdocs.yml index 856d86e6d..8f92a838f 100644 --- a/docs/mkdocs/mkdocs.yml +++ b/docs/mkdocs/mkdocs.yml @@ -292,6 +292,7 @@ nav: - 'JSON_HAS_THREE_WAY_COMPARISON': api/macros/json_has_three_way_comparison.md - 'JSON_NOEXCEPTION': api/macros/json_noexception.md - 'JSON_NO_IO': api/macros/json_no_io.md + - 'JSON_NO_THREAD_LOCAL': api/macros/json_no_thread_local.md - 'JSON_SKIP_LIBRARY_VERSION_CHECK': api/macros/json_skip_library_version_check.md - 'JSON_SKIP_UNSUPPORTED_COMPILER_CHECK': api/macros/json_skip_unsupported_compiler_check.md - 'JSON_STRICT_NUL_HANDLING': api/macros/json_strict_nul_handling.md diff --git a/include/nlohmann/detail/macro_scope.hpp b/include/nlohmann/detail/macro_scope.hpp index 8aacc0c51..def9da6f8 100644 --- a/include/nlohmann/detail/macro_scope.hpp +++ b/include/nlohmann/detail/macro_scope.hpp @@ -186,6 +186,15 @@ #define JSON_NO_UNIQUE_ADDRESS #endif +// Clang targeting MinGW does not survive the thread_local storage the copy +// constructor uses to bound its descent: every test that copies a value +// segfaults with clang 11.0.1 and clang 18.1.8, while the same tests pass with +// GCC targeting MinGW and with every other toolchain the library is tested on. +// Copying works the same way without the counter, only more slowly. +#if !defined(JSON_NO_THREAD_LOCAL) && defined(__clang__) && defined(__MINGW32__) + #define JSON_NO_THREAD_LOCAL 1 +#endif + // disable documentation warnings on clang #if defined(__clang__) #pragma clang diagnostic push diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index a63363fbf..9c1d821e1 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -28,14 +28,14 @@ #pragma GCC diagnostic ignored "-Wignored-attributes" #endif -#include // all_of, find, for_each +#include // all_of, find, for_each, none_of #include // nullptr_t, ptrdiff_t, size_t #include // hash, less #include // initializer_list #ifndef JSON_NO_IO #include // istream, ostream #endif // JSON_NO_IO -#include // random_access_iterator_tag +#include // make_move_iterator, random_access_iterator_tag #include // unique_ptr #include // string, stoi, to_string #include // declval, forward, move, pair, swap @@ -896,6 +896,352 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec return j; } +#ifndef JSON_NO_THREAD_LOCAL + /// the number of levels an operation descends into before it finishes the + /// value below it without the call stack + static constexpr std::uint8_t nesting_depth_limit() + { + return 128; + } + + /*! + @brief how many levels the operation going on in this thread has descended into + + Copying a value and comparing two values share this count. The library never + nests one inside the other - copying a value does not compare one, and + comparing two values does not copy them - and where user code nests them + anyway, sharing the count only ends a descent sooner than it had to, which + costs a little speed and is never wrong. + + A byte is enough: the count never exceeds the limit by more than the single + level that notices the limit has been reached. + */ + static std::uint8_t& nesting_depth() noexcept + { + static thread_local std::uint8_t depth = 0; // NOLINT(misc-use-internal-linkage) + return depth; + } +#endif + + /*! + @brief counts one level of a bounded descent for as long as it runs, and + reports whether the descent was still within the limit when it began + + Looks the count up and tests it against the limit itself, rather than + leaving that to the caller: either way it is reached exactly once, so + there is nothing to be gained by making the caller do it. + + Does nothing and is never @ref okay without thread-local storage, where no + descent can be bounded at all: a caller that only descends while this says + it may always ends up finishing without the call stack, exactly as if every + value were nested past the limit. + */ + class nesting_depth_guard + { + public: + nesting_depth_guard() noexcept +#ifdef JSON_NO_THREAD_LOCAL + : m_okay(false) +#else + : m_okay(nesting_depth() < nesting_depth_limit()) +#endif + { +#ifndef JSON_NO_THREAD_LOCAL + ++nesting_depth(); +#endif + } + + ~nesting_depth_guard() + { +#ifndef JSON_NO_THREAD_LOCAL + --nesting_depth(); +#endif + } + + nesting_depth_guard(const nesting_depth_guard&) = delete; + nesting_depth_guard& operator=(const nesting_depth_guard&) = delete; + nesting_depth_guard(nesting_depth_guard&&) = delete; + nesting_depth_guard& operator=(nesting_depth_guard&&) = delete; + + bool okay() const noexcept + { + return m_okay; + } + + private: + bool m_okay; + }; + + /// an entry of the iterative deep copy's worklist: a structured value and + /// the value that is to become its copy + using copy_worklist_t = std::vector>; + + /// scratch space to build the key skeleton of an object copy in one go + using copy_scratch_t = std::vector>; + + /// @brief copy everything of @a src into @a dst but its type and value + static void copy_metadata(const basic_json& src, basic_json& dst) + { + // a custom base class is only required to be copy-constructible and + // move-assignable, so the copy has to go through a temporary + static_cast(dst) = json_base_class_t(static_cast(src)); + +#if JSON_DIAGNOSTIC_POSITIONS + dst.start_position = src.start_position; + dst.end_position = src.end_position; +#endif + } + + /*! + @brief copy the value of @a src into @a dst, which must not be structured + + Objects and arrays are left alone: creating those is the one thing the copy + constructor and @ref copy_shallow do differently from one another, and it is + the reason copying a value can descend at all. + */ + /// @note inlined on purpose: both callers have already told an object or an + /// array apart from the rest, and letting the compiler fold that test + /// into this switch is worth a few percent when copying a value made + /// mostly of numbers + JSON_HEDLEY_ALWAYS_INLINE + static void copy_leaf_value(const basic_json& src, basic_json& dst) + { + switch (src.m_data.m_type) + { + case value_t::string: + { + dst.m_data.m_value = *src.m_data.m_value.string; + break; + } + + case value_t::binary: + { + dst.m_data.m_value = *src.m_data.m_value.binary; + break; + } + + case value_t::boolean: + { + dst.m_data.m_value = src.m_data.m_value.boolean; + break; + } + + case value_t::number_integer: + { + dst.m_data.m_value = src.m_data.m_value.number_integer; + break; + } + + case value_t::number_unsigned: + { + dst.m_data.m_value = src.m_data.m_value.number_unsigned; + break; + } + + case value_t::number_float: + { + dst.m_data.m_value = src.m_data.m_value.number_float; + break; + } + + case value_t::object: + case value_t::array: + case value_t::null: + case value_t::discarded: + default: + break; + } + } + + /*! + @brief copy everything of @a src into the null value @a dst but the children + + Objects and arrays are not copied here; they are appended to @a worklist to + be created later by @ref copy_iteratively. Until that happens, @a dst remains + a null value, so that a partially built copy can be destroyed at any point + without ever violating the class invariants. + */ + static void copy_shallow(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) + { + copy_metadata(src, dst); + + if (src.m_data.m_type == value_t::object || src.m_data.m_type == value_t::array) + { + // defer: dst stays a null value until its container exists + worklist.emplace_back(&src, &dst); + return; + } + + copy_leaf_value(src, dst); + + // only now that the value exists may the type be set: had the creation + // of the value thrown, dst would have been left as a valid null value + dst.m_data.m_type = src.m_data.m_type; + } + + /// @brief create the copy of the array @a src in @a dst + /// @note structured elements are appended to @a worklist instead + static void copy_array_level(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) + { + const array_t& src_array = *src.m_data.m_value.array; + + // create all elements up front: growing the array afterwards could + // invalidate the pointers that are handed to the worklist; resize() + // rather than the fill constructor, because not every array type + // provides the latter (e.g., ones without a matching allocator-aware + // fill constructor) + dst.m_data.m_value.array = create(); + dst.m_data.m_value.array->resize(src_array.size()); + + auto dst_it = dst.m_data.m_value.array->begin(); + for (auto src_it = src_array.cbegin(); src_it != src_array.cend(); ++src_it, ++dst_it) + { + copy_shallow(*src_it, *dst_it, worklist); + } + } + + /// @brief create the copy of the object @a src in @a dst + /// @note structured values are appended to @a worklist instead + static void copy_object_level(const basic_json& src, basic_json& dst, + copy_worklist_t& worklist, copy_scratch_t& scratch) + { + const object_t& src_object = *src.m_data.m_value.object; + + // build the complete key skeleton and hand it to the object's range + // constructor: adding the keys one by one would be quadratic for object + // types that are backed by a vector, such as nlohmann::ordered_map + scratch.clear(); + scratch.reserve(src_object.size()); + for (const auto& element : src_object) + { + scratch.emplace_back(element.first, basic_json()); + } + + dst.m_data.m_value.object = create(std::make_move_iterator(scratch.begin()), + std::make_move_iterator(scratch.end())); + scratch.clear(); + + // pair every value of the copy with its counterpart in the original; + // both are enumerated in the same order for every object type with a + // deterministic order, so the lookup is only needed for exotic ones + auto src_it = src_object.cbegin(); + for (auto& element : *dst.m_data.m_value.object) + { + if (JSON_HEDLEY_LIKELY(src_it != src_object.cend() && src_it->first == element.first)) + { + copy_shallow(src_it->second, element.second, worklist); + ++src_it; + } + else + { + const auto found = src_object.find(element.first); + JSON_ASSERT(found != src_object.cend()); + copy_shallow(found->second, element.second, worklist); + } + } + } + + /*! + @brief deep-copy the object or array @a src into this value without recursing + + The values whose copy has not been created yet are kept on an explicit + worklist rather than on the call stack. This is only reached for values + nested deeper than @ref nesting_depth_limit levels, which is why it copies + every container by hand instead of letting the container do it: the fast + ways of doing so would descend into the elements and defeat the purpose. + */ + void copy_iteratively(const basic_json& src) + { + copy_worklist_t worklist; + copy_scratch_t scratch; + + const basic_json* src_value = &src; + basic_json* dst_value = this; + + for (;;) + { + if (src_value->m_data.m_type == value_t::array) + { + copy_array_level(*src_value, *dst_value, worklist); + } + else + { + copy_object_level(*src_value, *dst_value, worklist, scratch); + } + + // the container is complete and will not be modified again + dst_value->set_parents(); + + if (worklist.empty()) + { + break; + } + + const auto& next = worklist.back(); + src_value = next.first; + dst_value = next.second; + worklist.pop_back(); + + // the value stops being a null value exactly here + dst_value->m_data.m_type = src_value->m_data.m_type; + } + } + + /*! + @brief copy one level of the object or array @a src into this value + + The container copies its own elements, which is the fastest way to fill it. + Every element that is structured itself comes back to @ref copy_structured. + */ + void copy_level(const basic_json& src) + { + if (m_data.m_type == value_t::object) + { + m_data.m_value = *src.m_data.m_value.object; + } + else + { + m_data.m_value = *src.m_data.m_value.array; + } + + set_parents(); + } + + /*! + @brief deep-copy the object or array @a src into this value + + Copying a container copies its elements, so a value nested deeply enough + used to exhaust the call stack. The descent is bounded here: the first + @ref nesting_depth_limit levels are copied by the containers themselves, just + as they always were, and anything below that is copied without the call + stack by @ref copy_iteratively. Copying a value can therefore no longer + exhaust the stack, however deeply it is nested, just like destroying one + cannot since #1436. + + Nothing has to be scanned or built by hand to reach that: a value that is + not nested deeper than the limit - all but a vanishing minority - is copied + exactly as it was before, and this whole detour costs it one counter. + + @sa https://github.com/nlohmann/json/issues/5387 + */ + void copy_structured(const basic_json& src) + { + const nesting_depth_guard guard; + + if (JSON_HEDLEY_LIKELY(guard.okay())) + { + copy_level(src); + return; + } + + // Finish this value without descending any further. It is completed + // before this returns, so a copy made by a custom base class - or by + // anything else that runs while a copy is going on - is unaffected by + // the copy it is nested in. + copy_iteratively(src); + } + + public: ////////////////////////// // JSON parser callback // @@ -1275,60 +1621,15 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // check of passed value is valid other.assert_invariant(); - switch (m_data.m_type) + if (m_data.m_type == value_t::object || m_data.m_type == value_t::array) { - case value_t::object: - { - m_data.m_value = *other.m_data.m_value.object; - break; - } - - case value_t::array: - { - m_data.m_value = *other.m_data.m_value.array; - break; - } - - case value_t::string: - { - m_data.m_value = *other.m_data.m_value.string; - break; - } - - case value_t::boolean: - { - m_data.m_value = other.m_data.m_value.boolean; - break; - } - - case value_t::number_integer: - { - m_data.m_value = other.m_data.m_value.number_integer; - break; - } - - case value_t::number_unsigned: - { - m_data.m_value = other.m_data.m_value.number_unsigned; - break; - } - - case value_t::number_float: - { - m_data.m_value = other.m_data.m_value.number_float; - break; - } - - case value_t::binary: - { - m_data.m_value = *other.m_data.m_value.binary; - break; - } - - case value_t::null: - case value_t::discarded: - default: - break; + // copying the container directly would call this constructor again + // for every element, once per nesting level + copy_structured(other); + } + else + { + copy_leaf_value(other, *this); } set_parents(); diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 4418a6c19..81b0e09f6 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -28,14 +28,14 @@ #pragma GCC diagnostic ignored "-Wignored-attributes" #endif -#include // all_of, find, for_each +#include // all_of, find, for_each, none_of #include // nullptr_t, ptrdiff_t, size_t #include // hash, less #include // initializer_list #ifndef JSON_NO_IO #include // istream, ostream #endif // JSON_NO_IO -#include // random_access_iterator_tag +#include // make_move_iterator, random_access_iterator_tag #include // unique_ptr #include // string, stoi, to_string #include // declval, forward, move, pair, swap @@ -2564,6 +2564,15 @@ JSON_HEDLEY_DIAGNOSTIC_POP #define JSON_NO_UNIQUE_ADDRESS #endif +// Clang targeting MinGW does not survive the thread_local storage the copy +// constructor uses to bound its descent: every test that copies a value +// segfaults with clang 11.0.1 and clang 18.1.8, while the same tests pass with +// GCC targeting MinGW and with every other toolchain the library is tested on. +// Copying works the same way without the counter, only more slowly. +#if !defined(JSON_NO_THREAD_LOCAL) && defined(__clang__) && defined(__MINGW32__) + #define JSON_NO_THREAD_LOCAL 1 +#endif + // disable documentation warnings on clang #if defined(__clang__) #pragma clang diagnostic push @@ -25152,6 +25161,352 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec return j; } +#ifndef JSON_NO_THREAD_LOCAL + /// the number of levels an operation descends into before it finishes the + /// value below it without the call stack + static constexpr std::uint8_t nesting_depth_limit() + { + return 128; + } + + /*! + @brief how many levels the operation going on in this thread has descended into + + Copying a value and comparing two values share this count. The library never + nests one inside the other - copying a value does not compare one, and + comparing two values does not copy them - and where user code nests them + anyway, sharing the count only ends a descent sooner than it had to, which + costs a little speed and is never wrong. + + A byte is enough: the count never exceeds the limit by more than the single + level that notices the limit has been reached. + */ + static std::uint8_t& nesting_depth() noexcept + { + static thread_local std::uint8_t depth = 0; // NOLINT(misc-use-internal-linkage) + return depth; + } +#endif + + /*! + @brief counts one level of a bounded descent for as long as it runs, and + reports whether the descent was still within the limit when it began + + Looks the count up and tests it against the limit itself, rather than + leaving that to the caller: either way it is reached exactly once, so + there is nothing to be gained by making the caller do it. + + Does nothing and is never @ref okay without thread-local storage, where no + descent can be bounded at all: a caller that only descends while this says + it may always ends up finishing without the call stack, exactly as if every + value were nested past the limit. + */ + class nesting_depth_guard + { + public: + nesting_depth_guard() noexcept +#ifdef JSON_NO_THREAD_LOCAL + : m_okay(false) +#else + : m_okay(nesting_depth() < nesting_depth_limit()) +#endif + { +#ifndef JSON_NO_THREAD_LOCAL + ++nesting_depth(); +#endif + } + + ~nesting_depth_guard() + { +#ifndef JSON_NO_THREAD_LOCAL + --nesting_depth(); +#endif + } + + nesting_depth_guard(const nesting_depth_guard&) = delete; + nesting_depth_guard& operator=(const nesting_depth_guard&) = delete; + nesting_depth_guard(nesting_depth_guard&&) = delete; + nesting_depth_guard& operator=(nesting_depth_guard&&) = delete; + + bool okay() const noexcept + { + return m_okay; + } + + private: + bool m_okay; + }; + + /// an entry of the iterative deep copy's worklist: a structured value and + /// the value that is to become its copy + using copy_worklist_t = std::vector>; + + /// scratch space to build the key skeleton of an object copy in one go + using copy_scratch_t = std::vector>; + + /// @brief copy everything of @a src into @a dst but its type and value + static void copy_metadata(const basic_json& src, basic_json& dst) + { + // a custom base class is only required to be copy-constructible and + // move-assignable, so the copy has to go through a temporary + static_cast(dst) = json_base_class_t(static_cast(src)); + +#if JSON_DIAGNOSTIC_POSITIONS + dst.start_position = src.start_position; + dst.end_position = src.end_position; +#endif + } + + /*! + @brief copy the value of @a src into @a dst, which must not be structured + + Objects and arrays are left alone: creating those is the one thing the copy + constructor and @ref copy_shallow do differently from one another, and it is + the reason copying a value can descend at all. + */ + /// @note inlined on purpose: both callers have already told an object or an + /// array apart from the rest, and letting the compiler fold that test + /// into this switch is worth a few percent when copying a value made + /// mostly of numbers + JSON_HEDLEY_ALWAYS_INLINE + static void copy_leaf_value(const basic_json& src, basic_json& dst) + { + switch (src.m_data.m_type) + { + case value_t::string: + { + dst.m_data.m_value = *src.m_data.m_value.string; + break; + } + + case value_t::binary: + { + dst.m_data.m_value = *src.m_data.m_value.binary; + break; + } + + case value_t::boolean: + { + dst.m_data.m_value = src.m_data.m_value.boolean; + break; + } + + case value_t::number_integer: + { + dst.m_data.m_value = src.m_data.m_value.number_integer; + break; + } + + case value_t::number_unsigned: + { + dst.m_data.m_value = src.m_data.m_value.number_unsigned; + break; + } + + case value_t::number_float: + { + dst.m_data.m_value = src.m_data.m_value.number_float; + break; + } + + case value_t::object: + case value_t::array: + case value_t::null: + case value_t::discarded: + default: + break; + } + } + + /*! + @brief copy everything of @a src into the null value @a dst but the children + + Objects and arrays are not copied here; they are appended to @a worklist to + be created later by @ref copy_iteratively. Until that happens, @a dst remains + a null value, so that a partially built copy can be destroyed at any point + without ever violating the class invariants. + */ + static void copy_shallow(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) + { + copy_metadata(src, dst); + + if (src.m_data.m_type == value_t::object || src.m_data.m_type == value_t::array) + { + // defer: dst stays a null value until its container exists + worklist.emplace_back(&src, &dst); + return; + } + + copy_leaf_value(src, dst); + + // only now that the value exists may the type be set: had the creation + // of the value thrown, dst would have been left as a valid null value + dst.m_data.m_type = src.m_data.m_type; + } + + /// @brief create the copy of the array @a src in @a dst + /// @note structured elements are appended to @a worklist instead + static void copy_array_level(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) + { + const array_t& src_array = *src.m_data.m_value.array; + + // create all elements up front: growing the array afterwards could + // invalidate the pointers that are handed to the worklist; resize() + // rather than the fill constructor, because not every array type + // provides the latter (e.g., ones without a matching allocator-aware + // fill constructor) + dst.m_data.m_value.array = create(); + dst.m_data.m_value.array->resize(src_array.size()); + + auto dst_it = dst.m_data.m_value.array->begin(); + for (auto src_it = src_array.cbegin(); src_it != src_array.cend(); ++src_it, ++dst_it) + { + copy_shallow(*src_it, *dst_it, worklist); + } + } + + /// @brief create the copy of the object @a src in @a dst + /// @note structured values are appended to @a worklist instead + static void copy_object_level(const basic_json& src, basic_json& dst, + copy_worklist_t& worklist, copy_scratch_t& scratch) + { + const object_t& src_object = *src.m_data.m_value.object; + + // build the complete key skeleton and hand it to the object's range + // constructor: adding the keys one by one would be quadratic for object + // types that are backed by a vector, such as nlohmann::ordered_map + scratch.clear(); + scratch.reserve(src_object.size()); + for (const auto& element : src_object) + { + scratch.emplace_back(element.first, basic_json()); + } + + dst.m_data.m_value.object = create(std::make_move_iterator(scratch.begin()), + std::make_move_iterator(scratch.end())); + scratch.clear(); + + // pair every value of the copy with its counterpart in the original; + // both are enumerated in the same order for every object type with a + // deterministic order, so the lookup is only needed for exotic ones + auto src_it = src_object.cbegin(); + for (auto& element : *dst.m_data.m_value.object) + { + if (JSON_HEDLEY_LIKELY(src_it != src_object.cend() && src_it->first == element.first)) + { + copy_shallow(src_it->second, element.second, worklist); + ++src_it; + } + else + { + const auto found = src_object.find(element.first); + JSON_ASSERT(found != src_object.cend()); + copy_shallow(found->second, element.second, worklist); + } + } + } + + /*! + @brief deep-copy the object or array @a src into this value without recursing + + The values whose copy has not been created yet are kept on an explicit + worklist rather than on the call stack. This is only reached for values + nested deeper than @ref nesting_depth_limit levels, which is why it copies + every container by hand instead of letting the container do it: the fast + ways of doing so would descend into the elements and defeat the purpose. + */ + void copy_iteratively(const basic_json& src) + { + copy_worklist_t worklist; + copy_scratch_t scratch; + + const basic_json* src_value = &src; + basic_json* dst_value = this; + + for (;;) + { + if (src_value->m_data.m_type == value_t::array) + { + copy_array_level(*src_value, *dst_value, worklist); + } + else + { + copy_object_level(*src_value, *dst_value, worklist, scratch); + } + + // the container is complete and will not be modified again + dst_value->set_parents(); + + if (worklist.empty()) + { + break; + } + + const auto& next = worklist.back(); + src_value = next.first; + dst_value = next.second; + worklist.pop_back(); + + // the value stops being a null value exactly here + dst_value->m_data.m_type = src_value->m_data.m_type; + } + } + + /*! + @brief copy one level of the object or array @a src into this value + + The container copies its own elements, which is the fastest way to fill it. + Every element that is structured itself comes back to @ref copy_structured. + */ + void copy_level(const basic_json& src) + { + if (m_data.m_type == value_t::object) + { + m_data.m_value = *src.m_data.m_value.object; + } + else + { + m_data.m_value = *src.m_data.m_value.array; + } + + set_parents(); + } + + /*! + @brief deep-copy the object or array @a src into this value + + Copying a container copies its elements, so a value nested deeply enough + used to exhaust the call stack. The descent is bounded here: the first + @ref nesting_depth_limit levels are copied by the containers themselves, just + as they always were, and anything below that is copied without the call + stack by @ref copy_iteratively. Copying a value can therefore no longer + exhaust the stack, however deeply it is nested, just like destroying one + cannot since #1436. + + Nothing has to be scanned or built by hand to reach that: a value that is + not nested deeper than the limit - all but a vanishing minority - is copied + exactly as it was before, and this whole detour costs it one counter. + + @sa https://github.com/nlohmann/json/issues/5387 + */ + void copy_structured(const basic_json& src) + { + const nesting_depth_guard guard; + + if (JSON_HEDLEY_LIKELY(guard.okay())) + { + copy_level(src); + return; + } + + // Finish this value without descending any further. It is completed + // before this returns, so a copy made by a custom base class - or by + // anything else that runs while a copy is going on - is unaffected by + // the copy it is nested in. + copy_iteratively(src); + } + + public: ////////////////////////// // JSON parser callback // @@ -25531,60 +25886,15 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // check of passed value is valid other.assert_invariant(); - switch (m_data.m_type) + if (m_data.m_type == value_t::object || m_data.m_type == value_t::array) { - case value_t::object: - { - m_data.m_value = *other.m_data.m_value.object; - break; - } - - case value_t::array: - { - m_data.m_value = *other.m_data.m_value.array; - break; - } - - case value_t::string: - { - m_data.m_value = *other.m_data.m_value.string; - break; - } - - case value_t::boolean: - { - m_data.m_value = other.m_data.m_value.boolean; - break; - } - - case value_t::number_integer: - { - m_data.m_value = other.m_data.m_value.number_integer; - break; - } - - case value_t::number_unsigned: - { - m_data.m_value = other.m_data.m_value.number_unsigned; - break; - } - - case value_t::number_float: - { - m_data.m_value = other.m_data.m_value.number_float; - break; - } - - case value_t::binary: - { - m_data.m_value = *other.m_data.m_value.binary; - break; - } - - case value_t::null: - case value_t::discarded: - default: - break; + // copying the container directly would call this constructor again + // for every element, once per nesting level + copy_structured(other); + } + else + { + copy_leaf_value(other, *this); } set_parents(); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 86b4825d7..7a9bc5471 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -75,7 +75,12 @@ target_compile_options(test_main PUBLIC # is annotated JSON_HEDLEY_NO_RETURN (it always throws), which # makes MSVC flag the code following its call in binary_reader.hpp # as unreachable for that instantiation, in both Debug and Release - $<$:/W4;/wd4566;/wd4996;/wd4702> + # Disable warning C4503: decorated name length exceeded, name was truncated; the deep + # copy support added for #5387 pushes the mangled name of + # std::allocator_traits<...>::construct for the custom-base-class + # test's map type past VS2015's limit. The name is only used for + # debug info, so truncation does not affect the build. + $<$:/W4;/wd4566;/wd4996;/wd4702;/wd4503> # https://github.com/nlohmann/json/issues/1114 $<$:/bigobj> $<$:-Wa,-mbig-obj> diff --git a/tests/src/unit-allocator.cpp b/tests/src/unit-allocator.cpp index 2dbb746b0..5c7b4230f 100644 --- a/tests/src/unit-allocator.cpp +++ b/tests/src/unit-allocator.cpp @@ -216,6 +216,57 @@ TEST_CASE("controlled bad_alloc") CHECK_THROWS_AS(my_json(s), std::bad_alloc&); next_construct_fails = false; } + + SECTION("basic_json(const basic_json&) of a deeply nested value (#5387)") + { + // Copying a value nested deeper than the descent bound builds the + // copy from the top down: every value whose own copy has not been + // made yet stays a null value until it is. Failing an allocation + // part-way through is what proves such a half-built copy can still + // be destroyed. + // + // Which path the failure lands in depends on the build: the first + // allocation of a copy belongs to the outermost level, so here it + // is the descending one. Built with JSON_NO_THREAD_LOCAL - as the + // ci_test_no_thread_local target builds the whole suite - no + // descent is made at all and the very same failure lands in the + // iterative path instead, part-way through its worklist. + const auto check_deep_copy = [](bool objects) + { + CAPTURE(objects); + + next_construct_fails = false; + + // deeper than the 128 levels the copy constructor descends into + const std::size_t depth = 300; + + my_json j = 1; + for (std::size_t i = 0; i < depth; ++i) + { + if (objects) + { + my_json wrapper = my_json::object(); + wrapper["a"] = std::move(j); + j = std::move(wrapper); + } + else + { + j = my_json::array({std::move(j)}); + } + } + + // NOLINTNEXTLINE(performance-unnecessary-copy-initialization): the copy is what is tested + CHECK_NOTHROW(my_json(j)); + + next_construct_fails = true; + // NOLINTNEXTLINE(performance-unnecessary-copy-initialization): the copy is what is tested + CHECK_THROWS_AS(my_json(j), std::bad_alloc&); + next_construct_fails = false; + }; + + check_deep_copy(false); + check_deep_copy(true); + } } } diff --git a/tests/src/unit-diagnostic-positions.cpp b/tests/src/unit-diagnostic-positions.cpp index 4d2f50a98..d607e935c 100644 --- a/tests/src/unit-diagnostic-positions.cpp +++ b/tests/src/unit-diagnostic-positions.cpp @@ -75,6 +75,72 @@ TEST_CASE("Better diagnostics with positions") CHECK(j.end_pos() == root.size()); } + SECTION("copying keeps the positions of nested values (#5387)") + { + // Values nested deeper than the copy constructor's descent bound are + // copied without the call stack, on a path that has to carry the + // positions over itself; shallower ones copy their containers, which + // bring the positions along. Both sides of the bound are checked here. + const auto check_copy = [](std::size_t depth, bool objects) + { + CAPTURE(depth) + CAPTURE(objects) + + const std::string opening = objects ? R"({"a":)" : "["; + const std::string closing = objects ? "}" : "]"; + + std::string text; + for (std::size_t i = 0; i < depth; ++i) + { + text += opening; + } + text += "12"; + for (std::size_t i = 0; i < depth; ++i) + { + text += closing; + } + + const json original = json::parse(text); + const json copy(original); // NOLINT(performance-unnecessary-copy-initialization) + + const json* o = &original; + const json* c = © + for (std::size_t level = 0; level <= depth; ++level) + { + CAPTURE(level) + REQUIRE(c->start_pos() == o->start_pos()); + REQUIRE(c->end_pos() == o->end_pos()); + + if (level < depth) + { + o = objects ? &o->at("a") : &o->at(0); + c = objects ? &c->at("a") : &c->at(0); + } + } + }; + + const auto check_arrays = [&check_copy](std::size_t depth) + { + check_copy(depth, false); + }; + const auto check_objects = [&check_copy](std::size_t depth) + { + check_copy(depth, true); + }; + + check_arrays(1); + check_arrays(127); + check_arrays(128); + check_arrays(129); + check_arrays(300); + + check_objects(1); + check_objects(127); + check_objects(128); + check_objects(129); + check_objects(300); + } + SECTION("JSON patch add to primitive parent (#4292)") { // the JSON Patch "add" target /foo/bar/baz has a string parent diff --git a/tests/src/unit-diagnostics.cpp b/tests/src/unit-diagnostics.cpp index 135ecf9d6..389a7a3d7 100644 --- a/tests/src/unit-diagnostics.cpp +++ b/tests/src/unit-diagnostics.cpp @@ -274,6 +274,63 @@ TEST_CASE("Regression tests for extended diagnostics") CHECK(j1["string"] == "t"); } + SECTION("Regression test for issue #5387 - copying keeps the parents of nested values") + { + // A value nested deeper than the copy constructor's descent bound is + // copied without the call stack. Every container that path creates has + // to have the parents of its children set, or the JSON Pointer in the + // diagnostic is cut short. + const std::size_t depth = 300; + + SECTION("objects") + { + json j = "not a number"; + std::string pointer; + for (std::size_t i = 0; i < depth; ++i) + { + j = json{{"a", j}}; + pointer += "/a"; + } + + json const copy(j); // NOLINT(performance-unnecessary-copy-initialization) + + const json* inner = © + for (std::size_t i = 0; i < depth; ++i) + { + inner = &inner->at("a"); + } + + std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string"; + int i = 0; + CHECK_THROWS_WITH_AS(i = inner->get(), expected.c_str(), json::type_error); + CHECK(i == 0); + } + + SECTION("arrays") + { + json j = "not a number"; + std::string pointer; + for (std::size_t i = 0; i < depth; ++i) + { + j = json::array({j}); + pointer += "/0"; + } + + json const copy(j); // NOLINT(performance-unnecessary-copy-initialization) + + const json* inner = © + for (std::size_t i = 0; i < depth; ++i) + { + inner = &inner->at(0); + } + + std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string"; + int i = 0; + CHECK_THROWS_WITH_AS(i = inner->get(), expected.c_str(), json::type_error); + CHECK(i == 0); + } + } + SECTION("Regression test - swap(array_t&)/swap(object_t&) must update JSON_DIAGNOSTICS parent pointers") { // swap(array_t&) diff --git a/tests/src/unit-large_json.cpp b/tests/src/unit-large_json.cpp index 98d16e336..f10c8be41 100644 --- a/tests/src/unit-large_json.cpp +++ b/tests/src/unit-large_json.cpp @@ -12,6 +12,7 @@ using nlohmann::json; #include +#include TEST_CASE("tests on very large JSONs") { @@ -27,3 +28,153 @@ TEST_CASE("tests on very large JSONs") } } +namespace +{ + +// Descend a chain of single-element containers and return the value at its end, +// reporting the number of levels traversed in @a depth. +// +// The values in the test case below are nested far deeper than the call stack +// can follow, so they must not be inspected with operator== or dump(): both are +// still recursive and would overflow the stack themselves. +const json* innermost_value(const json& j, std::size_t& depth) +{ + const json* current = &j; + depth = 0; + + while ((current->is_array() || current->is_object()) && !current->empty()) + { + current = current->is_array() + ? ¤t->front() + : ¤t->begin().value(); + ++depth; + } + + return current; +} + +} // namespace + +TEST_CASE("tests on deeply nested JSONs") +{ + // deep enough to exhaust the call stack, but small enough to stay cheap: + // parsing is iterative, so building the values below costs little + const std::size_t depth = 100000; + + SECTION("issue #5387 - stack overflow in the copy constructor") + { + SECTION("array") + { + const json j = json::parse(std::string(depth, '[') + '0' + std::string(depth, ']')); + + const json copy(j); // NOLINT(performance-unnecessary-copy-initialization): the copy is what is tested + + std::size_t copy_depth = 0; + CHECK(*innermost_value(copy, copy_depth) == 0); + CHECK(copy_depth == depth); + } + + SECTION("object") + { + std::string s; + s.reserve((6 * depth) + 1); + for (std::size_t i = 0; i < depth; ++i) + { + s += "{\"a\":"; + } + s += '1'; + s.append(depth, '}'); + + const json j = json::parse(s); + + const json copy(j); // NOLINT(performance-unnecessary-copy-initialization): the copy is what is tested + + std::size_t copy_depth = 0; + CHECK(*innermost_value(copy, copy_depth) == 1); + CHECK(copy_depth == depth); + } + + SECTION("copy assignment") + { + // operator=(basic_json) takes its argument by value, so the deep + // copy happens in the copy constructor + const json j = json::parse(std::string(depth, '[') + '0' + std::string(depth, ']')); + + json target; + target = j; + + std::size_t target_depth = 0; + CHECK(*innermost_value(target, target_depth) == 0); + CHECK(target_depth == depth); + } + + SECTION("depths around the bound of the recursive descent") + { + // The copy constructor descends into a bounded number of levels and + // completes whatever is below that without the call stack. Cover + // every depth around that bound, so that the two ways of copying + // are known to meet cleanly - wherever the bound is set. + for (std::size_t d = 1; d <= 300; ++d) + { + CAPTURE(d); + + const json array = json::parse(std::string(d, '[') + '0' + std::string(d, ']')); + const json array_copy(array); // NOLINT(performance-unnecessary-copy-initialization): the copy is what is tested + std::size_t array_depth = 0; + CHECK(*innermost_value(array_copy, array_depth) == 0); + CHECK(array_depth == d); + + std::string object_text; + for (std::size_t i = 0; i < d; ++i) + { + object_text += "{\"a\":"; + } + object_text += '1'; + object_text.append(d, '}'); + + const json object = json::parse(object_text); + const json object_copy(object); // NOLINT(performance-unnecessary-copy-initialization): the copy is what is tested + std::size_t object_depth = 0; + CHECK(*innermost_value(object_copy, object_depth) == 1); + CHECK(object_depth == d); + } + } + + SECTION("a value that is deep in one place only") + { + json j = json::object(); + j["shallow"] = 1; + j["deep"] = json::parse(std::string(depth, '[') + '0' + std::string(depth, ']')); + j["also_shallow"] = json::array({1, 2, 3}); + + const json copy(j); + + CHECK(copy["shallow"] == 1); + CHECK(copy["also_shallow"] == json::array({1, 2, 3})); + + std::size_t deep_depth = 0; + CHECK(*innermost_value(copy["deep"], deep_depth) == 0); + CHECK(deep_depth == depth); + } + + SECTION("the copy is independent of the original") + { + const json j = json::parse(std::string(depth, '[') + '0' + std::string(depth, ']')); + + json copy(j); + + // reach the innermost value without recursing and replace it + json* current = © + while (current->is_array() && !current->empty()) + { + current = ¤t->front(); + } + *current = 42; + + std::size_t unused = 0; + CHECK(*innermost_value(copy, unused) == 42); + CHECK(*innermost_value(j, unused) == 0); + } + } +} + diff --git a/tests/src/unit-ordered_json.cpp b/tests/src/unit-ordered_json.cpp index 62a949a7f..45fbf5493 100644 --- a/tests/src/unit-ordered_json.cpp +++ b/tests/src/unit-ordered_json.cpp @@ -82,6 +82,40 @@ TEST_CASE("regression test for issue #3732 - iteration_proxy_value(fn); } +TEST_CASE("copying an ordered_json with nested values") +{ + // ordered_map is backed by a vector, so copying an object that has + // structured values takes a different route than copying a std::map-backed + // one; see https://github.com/nlohmann/json/issues/5387 + ordered_json oj; + oj["z"] = 1; + oj["a"]["y"] = 2; + oj["a"]["b"]["x"] = 3; + oj["m"] = {1, 2, {{"w", 4}}}; + + const ordered_json copy(oj); + + SECTION("the copy is equal to the original") + { + CHECK(copy == oj); + CHECK(copy.dump() == oj.dump()); + } + + SECTION("the key order is preserved at every level") + { + CHECK(copy.dump() == R"({"z":1,"a":{"y":2,"b":{"x":3}},"m":[1,2,{"w":4}]})"); + } + + SECTION("the copy is independent of the original") + { + ordered_json mutated(oj); + mutated["a"]["b"]["x"] = 99; + + CHECK(oj["a"]["b"]["x"] == 3); + CHECK(mutated["a"]["b"]["x"] == 99); + } +} + TEST_CASE("regression test - diff() must account for ordered_json member order") { SECTION("pure reorder, no value changes")