Document that read() must not be given a view of the document itself (#6086)

* Document that read() must not be given a view of the document itself

basic_json_document::read() releases the previous text, the decoded
strings and the edits before it parses its input. An input that points
into them (source(), get_string(), number_token(), or a string of an
edited document) is read after its memory was cleared or freed.

A guard that copies such input costs about 1 ns per read() (5 % of a
read of a 7-byte input), which this API exists to avoid, so state the
precondition in read.md, lifetime.md and the pages of the views that
hand out such strings instead, and show how to copy the string first.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Assert in debug builds that read() is not given its own text

read() releases the document's text, decoded strings, owned image and
edits before it parses, so an input that points into them is read after
release. Without NDEBUG, read() now asserts that a borrowed input, a
copied contiguous input and (before C++17) a string_ref input are not
held by the document: its owned text, its decoded strings, an owned
image, and the current buffer of edited strings. Release builds keep
the documented precondition without a check, which would cost 2-4.5 %
per read() of a small message.

JSON_ASSERT is undefined at the end of json.hpp, so the view uses
assert() through a new internal NLOHMANN_VIEW_ASSERT.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

docs/mkdocs/docs/api/basic_json_document/index.md

@@ -111,8 +111,9 @@ points apply to every edit:
   the end, and, with [`number_format::source`](../basic_json_view/number_format.md), keeps the spelling of every
   number that was not itself edited — see [Editing a document](../../features/json_view/editing.md) for
   why this matters.
-- [`read()`](read.md) discards all edits, [`shrink_to_fit()`](shrink_to_fit.md) does not move the node index once
-  there are edits, and [`memory_usage()`](memory_usage.md) includes the memory edits use.
+- [`read()`](read.md) discards all edits (so its `input` must not be a string obtained from this document),
+  [`shrink_to_fit()`](shrink_to_fit.md) does not move the node index once there are edits, and
+  [`memory_usage()`](memory_usage.md) includes the memory edits use.
   [`source_offset()`](../basic_json_view/source_offset.md) of a value introduced by an edit is
   `#!cpp static_cast<std::size_t>(-1)`, the same value it reports for a decoded string.
 

docs/mkdocs/docs/api/basic_json_document/read.md

@@ -46,6 +46,34 @@ Linear in the length of the input.
 Every view taken from `#!cpp *this` before the call — including the previous [`root()`](root.md) — is invalidated,
 whether or not the new parse succeeds; take fresh views from [`root()`](root.md) afterward.
 
+!!! warning "`input` must not point into the document itself"
+
+    `read()` releases the document's previous text, its decoded strings, and (for an editable document) its edits
+    *before* it parses `input`. An `input` that is, or points into, any of them violates a precondition: it may seem to
+    work, parse empty or garbled text, be reported by a sanitizer, or read freed memory (undefined behavior).
+
+    This applies to every `string_view` obtained from the document whose characters live in memory that `read()`
+    releases:
+
+    - [`source()`](source.md) of a document that owns its text;
+    - [`get_string()`](../basic_json_view/get_string.md) (also as `get<string_view_t>()` or a `string_view` member of a
+      decoded struct) of a string with escapes, of a string of a document that owns its text, and of an edited string;
+    - [`number_token()`](../basic_json_view/number_token.md) of a number of a document that owns its text, and of a
+      number written by an edit;
+    - the key of an object member (`key()`) if it has escapes, was written by an edit, or the document owns its text.
+
+    It does not apply to a text that the caller owns and the document borrows, nor to the strings, keys, and tokens that
+    are read from such a text unchanged, nor to the decoded strings of a borrowed [image](load.md), which live in the
+    caller's image: `read()` never touches them. To parse JSON that a document holds in a string of its own (for
+    example, JSON embedded in a string value), copy the string first, for example with
+    `#!cpp std::string inner(doc.root()["payload"].get_string()); doc.read(inner);`.
+
+    A debug build (one without `NDEBUG`) checks this with `assert()`: an `input` in the document's text, decoded
+    strings, or image, or in the most recent buffer of strings written by edits, ends the program. (Strings written
+    by earlier edits are not covered.) A release build does not check, to keep the cost of reusing a document at a
+    minimum. The check uses `assert()` directly, not [`JSON_ASSERT`](../macros/json_assert.md), which `json.hpp`
+    undefines at its end.
+
 `input` is borrowed or owned by the same rules as [`parse()`](parse.md#notes); a document can borrow on one call and
 own on the next, since ownership is decided freshly each time. An input that is copied (see the table in
 [`parse()`](parse.md#notes)) is written into the document's previous owned copy, reusing its memory; a moved-in

docs/mkdocs/docs/api/basic_json_document/source.md

@@ -24,6 +24,9 @@ Constant.
 For a borrowed document, `source()` points directly into the caller's buffer, so it is only valid while that buffer
 is; see [`owns_source`](owns_source.md).
 
+The text of a document that owns it must not be passed to [`read()`](read.md) of the same document: `read()` releases
+an owned text before it parses its input. Copy it first. (A borrowed text belongs to the caller and is not affected.)
+
 ## Examples
 
 ??? example

docs/mkdocs/docs/api/basic_json_view/get_string.md

@@ -46,6 +46,10 @@ The returned [`string_view_t`](index.md#member-types) is valid exactly as long a
 source text, which for a borrowed document must remain valid **and unmodified** for as long as the document exists,
 since floating-point values and unescaped strings are read from the source lazily.
 
+A string with escapes, a string of a document that owns its text, and an edited string must not be passed to
+[`read()`](../basic_json_document/read.md) of the document it came from: `read()` releases them before it parses its
+input. Copy the string first. (A string read unchanged from a borrowed text is not affected.)
+
 ## Examples
 
 ??? example

docs/mkdocs/docs/api/basic_json_view/number_token.md

@@ -38,9 +38,14 @@ by the grammar; trailing zeros in the fraction; the case and sign of the exponen
 useful precisely where that form must survive — a price or an identifier that must be reproduced exactly, or a
 number too large for any of `BasicJsonType`'s number types to hold without loss.
 
-The returned [`string_view_t`](index.md#member-types) always points into the document's
-[`source()`](../basic_json_document/source.md) text — numbers are never decoded into the document's separate string
-buffer — and is valid exactly as long as that text is, see the [validity rules](index.md) of `basic_json_view`.
+The returned [`string_view_t`](index.md#member-types) points into the document's
+[`source()`](../basic_json_document/source.md) text or, for a number written by an
+[edit](../basic_json_document/index.md#edits), into storage owned by the document (the token of an edited `NaN` is
+`nan`). It is valid exactly as long as that text or storage is, see the [validity rules](index.md) of
+`basic_json_view`. Numbers are never decoded into the document's separate string buffer.
+
+For the same reason, the token must not be passed to [`read()`](../basic_json_document/read.md) of the document it came
+from if that document owns its text or the number was written by an edit: `read()` releases both before it parses.
 
 ## Examples
 

docs/mkdocs/docs/features/json_view/lifetime.md

@@ -50,6 +50,13 @@ The same holds for what a view hands out without copying: a string from
 [`number_token()`](../../api/basic_json_view/number_token.md), and [`string_view` members](reading.md#string_view-members)
 of decoded structs are valid only as long as the document and, if it borrows, its source text.
 
+This includes [`read()`](../../api/basic_json_document/read.md): it releases the document's text, decoded strings, and
+edits before it parses its input, so the input must not be a `string_view` obtained from the document itself: its
+[`source()`](../../api/basic_json_document/source.md), a string from `get_string()`, a token from `number_token()`, or a
+key from an iterator (those of a borrowed text that the caller owns, read unchanged, are not affected). To re-parse a
+string that the document holds, for example JSON embedded in a string value, copy the string into a `#!cpp std::string`
+first and pass the copy; see [`read()`](../../api/basic_json_document/read.md#notes) for the exact list.
+
 ??? example "Example: borrowed and owned documents, and when views become invalid"
 
     ```cpp

include/nlohmann/detail/view/document_data.hpp

@@ -12,6 +12,7 @@
 #include <cstddef> // size_t
 #include <cstdint> // uint8_t, uint32_t
 #include <cstring> // memcmp, memcpy
+#include <functional> // less
 #include <limits> // numeric_limits
 #include <map> // map
 #include <memory> // unique_ptr
@@ -149,6 +150,23 @@ struct document_data
         node_capacity = inline_capacity;
     }
 
+    /// whether the n bytes at p overlap memory that the next parse releases or
+    /// reuses: the owned text, the decoded strings, an owned image, and the
+    /// current buffer of strings written by edits (earlier edit buffers are not
+    /// covered, as their sizes are not kept); for the debug check in read()
+    bool holds(const char* p, std::size_t n) const noexcept
+    {
+        const auto overlaps = [p, n](const char* b, std::size_t m) noexcept
+        {
+            // (std::less: a total order also for pointers into different objects)
+            return n != 0 && m != 0 && std::less<const char*>()(p, b + m) && std::less<const char*>()(b, p + n);
+        };
+        return overlaps(owned.data(), owned.capacity())
+               || overlaps(arena.data(), arena.capacity())
+               || overlaps(reinterpret_cast<const char*>(owned_image.data()), owned_image.capacity()) // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
+               || (edits && !edits->texts.empty() && overlaps(edits->texts.back().get(), edits->text_cap));
+    }
+
     /// the largest node count whose size in bytes fits a std::size_t
     static constexpr std::size_t max_nodes() noexcept
     {

include/nlohmann/detail/view/macro_scope.hpp

@@ -71,6 +71,11 @@
     #define NLOHMANN_VIEW_THROW JSON_THROW_USER
 #endif
 
+// debug checks of preconditions (assert(), off with NDEBUG): JSON_ASSERT is
+// one of the macros that json.hpp undefines at its end
+#include <cassert> // assert
+#define NLOHMANN_VIEW_ASSERT(x) assert(x)
+
 // the parser stores a node's first word at once where the layout of `node` is
 // known to be little-endian (MSVC targets are); elsewhere field by field
 #if (defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) || defined(_MSC_VER)

include/nlohmann/detail/view/macro_unscope.hpp

@@ -18,6 +18,7 @@
 #undef NLOHMANN_VIEW_NOCLONE
 #undef NLOHMANN_VIEW_NODISCARD
 #undef NLOHMANN_VIEW_THROW
+#undef NLOHMANN_VIEW_ASSERT
 #undef NLOHMANN_VIEW_LITTLE_ENDIAN
 #undef NLOHMANN_VIEW_REPEAT16
 #undef NLOHMANN_VIEW_REPEAT_SECOND_8

include/nlohmann/json_view.hpp

@@ -1531,6 +1531,10 @@ class basic_json_document
     /// sentinel: src[size] is readable and 0 (std::string, C strings)
     void build(const char* src, std::size_t size, on_error allow_exceptions, bool comments, bool trailing_commas, bool owned, bool sentinel)
     {
+        // precondition of read(): a borrowed input is not held by this document,
+        // which releases what it holds below (checked in debug builds only, as
+        // it would cost every read() of a small message a few percent)
+        NLOHMANN_VIEW_ASSERT(owned || !m_data || !m_data->holds(src, size));
         ensure_data(src, size);
         document_data& d = *m_data;
         if (!owned)
@@ -1620,6 +1624,10 @@ class basic_json_document
     template<typename T>
     void read_kind(const T& s, on_error ae, bool c, bool tc, std::integral_constant<input_kind, input_kind::copy_range> /*unused*/)
     {
+        // (the precondition of read() as for a borrowed input, although only an
+        // input in the owned text breaks a copy: the same call borrows, for
+        // example a std::string_view in C++17, and must not depend on that)
+        NLOHMANN_VIEW_ASSERT(!m_data || !m_data->holds(reinterpret_cast<const char*>(s.data()), static_cast<std::size_t>(s.size()))); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
         build_copy(s, ae, c, tc);
     }
 
@@ -1638,9 +1646,24 @@ class basic_json_document
     template<typename T>
     void read_kind(T&& input, on_error ae, bool c, bool tc, std::integral_constant<input_kind, input_kind::adapter> /*unused*/)
     {
+        NLOHMANN_VIEW_ASSERT(!held(input)); // (see the copy_range overload)
         build_copy(std::forward<T>(input), ae, c, tc);
     }
 
+    /// whether an input read through the input adapter is held by this document:
+    /// string_view_t before C++17 (a string handed out by a view), for the
+    /// debug check of read(); other inputs of this kind do not point into it
+    bool held(const detail::view::string_ref& s) const noexcept
+    {
+        return m_data && m_data->holds(s.data(), s.size());
+    }
+
+    template<typename T>
+    bool held(const T& /*input*/) const noexcept
+    {
+        return false;
+    }
+
     template<typename IteratorType>
     void read_range(IteratorType first, IteratorType last, on_error ae, bool c, bool tc)
     {

single_include/nlohmann/json_view.hpp

@@ -106,6 +106,7 @@
 #include <cstddef> // size_t
 #include <cstdint> // uint8_t, uint32_t
 #include <cstring> // memcmp, memcpy
+#include <functional> // less
 #include <limits> // numeric_limits
 #include <map> // map
 #include <memory> // unique_ptr
@@ -188,6 +189,11 @@
     #define NLOHMANN_VIEW_THROW JSON_THROW_USER
 #endif
 
+// debug checks of preconditions (assert(), off with NDEBUG): JSON_ASSERT is
+// one of the macros that json.hpp undefines at its end
+#include <cassert> // assert
+#define NLOHMANN_VIEW_ASSERT(x) assert(x)
+
 // the parser stores a node's first word at once where the layout of `node` is
 // known to be little-endian (MSVC targets are); elsewhere field by field
 #if (defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) || defined(_MSC_VER)
@@ -488,6 +494,23 @@ struct document_data
         node_capacity = inline_capacity;
     }
 
+    /// whether the n bytes at p overlap memory that the next parse releases or
+    /// reuses: the owned text, the decoded strings, an owned image, and the
+    /// current buffer of strings written by edits (earlier edit buffers are not
+    /// covered, as their sizes are not kept); for the debug check in read()
+    bool holds(const char* p, std::size_t n) const noexcept
+    {
+        const auto overlaps = [p, n](const char* b, std::size_t m) noexcept
+        {
+            // (std::less: a total order also for pointers into different objects)
+            return n != 0 && m != 0 && std::less<const char*>()(p, b + m) && std::less<const char*>()(b, p + n);
+        };
+        return overlaps(owned.data(), owned.capacity())
+               || overlaps(arena.data(), arena.capacity())
+               || overlaps(reinterpret_cast<const char*>(owned_image.data()), owned_image.capacity()) // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
+               || (edits && !edits->texts.empty() && overlaps(edits->texts.back().get(), edits->text_cap));
+    }
+
     /// the largest node count whose size in bytes fits a std::size_t
     static constexpr std::size_t max_nodes() noexcept
     {
@@ -11630,6 +11653,10 @@ class basic_json_document
     /// sentinel: src[size] is readable and 0 (std::string, C strings)
     void build(const char* src, std::size_t size, on_error allow_exceptions, bool comments, bool trailing_commas, bool owned, bool sentinel)
     {
+        // precondition of read(): a borrowed input is not held by this document,
+        // which releases what it holds below (checked in debug builds only, as
+        // it would cost every read() of a small message a few percent)
+        NLOHMANN_VIEW_ASSERT(owned || !m_data || !m_data->holds(src, size));
         ensure_data(src, size);
         document_data& d = *m_data;
         if (!owned)
@@ -11719,6 +11746,10 @@ class basic_json_document
     template<typename T>
     void read_kind(const T& s, on_error ae, bool c, bool tc, std::integral_constant<input_kind, input_kind::copy_range> /*unused*/)
     {
+        // (the precondition of read() as for a borrowed input, although only an
+        // input in the owned text breaks a copy: the same call borrows, for
+        // example a std::string_view in C++17, and must not depend on that)
+        NLOHMANN_VIEW_ASSERT(!m_data || !m_data->holds(reinterpret_cast<const char*>(s.data()), static_cast<std::size_t>(s.size()))); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
         build_copy(s, ae, c, tc);
     }
 
@@ -11737,9 +11768,24 @@ class basic_json_document
     template<typename T>
     void read_kind(T&& input, on_error ae, bool c, bool tc, std::integral_constant<input_kind, input_kind::adapter> /*unused*/)
     {
+        NLOHMANN_VIEW_ASSERT(!held(input)); // (see the copy_range overload)
         build_copy(std::forward<T>(input), ae, c, tc);
     }
 
+    /// whether an input read through the input adapter is held by this document:
+    /// string_view_t before C++17 (a string handed out by a view), for the
+    /// debug check of read(); other inputs of this kind do not point into it
+    bool held(const detail::view::string_ref& s) const noexcept
+    {
+        return m_data && m_data->holds(s.data(), s.size());
+    }
+
+    template<typename T>
+    bool held(const T& /*input*/) const noexcept
+    {
+        return false;
+    }
+
     template<typename IteratorType>
     void read_range(IteratorType first, IteratorType last, on_error ae, bool c, bool tc)
     {
@@ -11895,6 +11941,7 @@ class tuple_element<N, ::nlohmann::detail::view::view_item<View>> // NOLINT(cert
 #undef NLOHMANN_VIEW_NOCLONE
 #undef NLOHMANN_VIEW_NODISCARD
 #undef NLOHMANN_VIEW_THROW
+#undef NLOHMANN_VIEW_ASSERT
 #undef NLOHMANN_VIEW_LITTLE_ENDIAN
 #undef NLOHMANN_VIEW_REPEAT16
 #undef NLOHMANN_VIEW_REPEAT_SECOND_8

tests/src/unit-json_view_builder.cpp

@@ -906,3 +906,71 @@ TEST_CASE("json_view node array size limit")
     d->reserve(cap);
     CHECK(d->nodes == nodes);
 }
+
+TEST_CASE("json_view document_data::holds() (the debug check of read())")
+{
+    std::unique_ptr<document_data, document_data::deleter> d(document_data::create(0));
+    const std::string other = "[1,2,3]";
+    // an empty document holds nothing
+    CHECK(!d->holds(other.data(), other.size()));
+
+    SECTION("owned text")
+    {
+        d->owned = "{\"a\":\"some text that does not fit a short string\"}";
+        const char* const b = d->owned.data();
+        CHECK(d->holds(b, 1));
+        CHECK(d->holds(b + 5, 3));
+        CHECK(d->holds(b + d->owned.size() - 1, 1));
+        // also after clear(), which keeps the buffer that the next parse writes
+        d->owned.clear();
+        CHECK(d->holds(b, 4));
+        CHECK(!d->holds(other.data(), other.size()));
+        CHECK(!d->holds(b, 0));
+    }
+
+    SECTION("decoded strings")
+    {
+        d->arena.assign(32, 'x');
+        CHECK(d->holds(d->arena.data() + 31, 1));
+        CHECK(d->holds(d->arena.data() + 31, 8)); // (overlapping the end)
+        CHECK(!d->holds(other.data(), other.size()));
+    }
+
+    SECTION("owned image")
+    {
+        d->owned_image.assign(16, 0);
+        CHECK(d->holds(reinterpret_cast<const char*>(d->owned_image.data()) + 8, 2)); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
+    }
+
+    SECTION("current edit buffer")
+    {
+        d->edits.reset(new document_data::edit_state()); // NOLINT(cppcoreguidelines-owning-memory)
+        CHECK(!d->holds(other.data(), other.size()));
+        d->edits->texts.emplace_back(new char[16]); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays)
+        d->edits->text_cap = 16;
+        CHECK(d->holds(d->edits->texts.back().get() + 3, 3));
+        CHECK(!d->holds(other.data(), other.size()));
+    }
+}
+
+TEST_CASE("json_view read() of inputs the check must accept")
+{
+    // in a debug build each read() below runs the check; none of the inputs
+    // is held by the document
+    const std::string text = R"({"a":"x\ny","b":[1,2]})";
+    nlohmann::json_document d;
+    d.read(text); // borrowed
+    d.read(text); // the same borrowed text again
+    CHECK(d.root()["b"][1].get<int>() == 2);
+    const std::string copy(d.root()["a"].get_string()); // a decoded string, copied first
+    d.read(std::string(text)); // owned
+    d.read(R"(["x"])");
+    d.read(text);
+    CHECK(copy == "x\ny");
+    nlohmann::json_editable_document e;
+    e.read(text);
+    e.set(e.root(), "a", "[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]");
+    const std::string edited(e.root()["a"].get_string()); // an edited string, copied first
+    e.read(edited);
+    CHECK(e.root().size() == 16);
+}