Address review comments on nested indefinite-length CBOR strings

Rename is_chunk to inside_indefinite, update the stale test section
names, and use lowercase comments like the surrounding code.

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

docs/mkdocs/docs/features/binary_formats/cbor.md

@@ -131,6 +131,7 @@ The library maps CBOR types to JSON value types as follows:
 | Byte string            | binary          | 0x59       |
 | Byte string            | binary          | 0x5A       |
 | Byte string            | binary          | 0x5B       |
+| Byte string            | binary          | 0x5F       |
 | UTF-8 string           | string          | 0x60..0x77 |
 | UTF-8 string           | string          | 0x78       |
 | UTF-8 string           | string          | 0x79       |
@@ -156,6 +157,9 @@ The library maps CBOR types to JSON value types as follows:
 | Single-Precision Float | number_float    | 0xFA       |
 | Double-Precision Float | number_float    | 0xFB       |
 
+Indefinite-length UTF-8 strings (0x7F) and byte strings (0x5F) are supported. Each chunk must be a definite-length
+string of the same major type, as required by [RFC 8949, Section 3.2.3](https://www.rfc-editor.org/rfc/rfc8949.html#section-3.2.3).
+
 !!! warning "Incomplete mapping"
 
     The mapping is **incomplete** in the sense that not all CBOR types can be converted to a JSON value. The following CBOR types are not supported and will yield parse errors:

include/nlohmann/detail/input/binary_reader.hpp

@@ -1072,6 +1072,20 @@ class binary_reader
         }
     }
 
+    /*!
+    @brief reports a nested indefinite-length CBOR string or byte array
+    @param[in] type_name  name of the rejected string type
+    @param[in] context  parsing context for the error message
+    @return whether the SAX consumer accepts the parse error
+    */
+    bool cbor_indefinite_string_error(const char* type_name, const char* context)
+    {
+        auto last_token = get_token_string();
+        return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
+                                exception_message(concat("indefinite-length ", type_name,
+                                        " is not allowed inside indefinite-length ", type_name, "; last byte: 0x", last_token), context), nullptr));
+    }
+
     /*!
     @brief reads a definite-length CBOR string
 
@@ -1081,12 +1095,13 @@ class binary_reader
     into the same string.
 
     @param[out] result  string the bytes are appended to
+    @param[in] inside_indefinite  whether the bytes belong to an indefinite-length string
 
     @return whether string creation completed
 
     @pre @a current is not EOF
     */
-    bool get_cbor_string_chunk(string_t& result)
+    bool get_cbor_string_chunk(string_t& result, const bool inside_indefinite)
     {
         switch (current)
         {
@@ -1147,7 +1162,7 @@ class binary_reader
             {
                 auto last_token = get_token_string();
                 return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
-                                        exception_message(concat("expected length specification (0x60-0x7B) or indefinite string type (0x7F); last byte: 0x", last_token), "string"), nullptr));
+                                        exception_message(concat("expected length specification (0x60-0x7B)", inside_indefinite ? "" : " or indefinite string type (0x7F)", "; last byte: 0x", last_token), "string"), nullptr));
             }
         }
     }
@@ -1165,13 +1180,9 @@ class binary_reader
     */
     bool get_cbor_string(string_t& result, const char* context = "string")
     {
-        // number of indefinite-length strings that have been opened and not
+        // read chunks iteratively, but reject a second indefinite-length
-        // closed yet. RFC 8949, Section 3.2.3 does not permit nesting them,
+        // level as required by RFC 8949, Section 3.2.3
-        // but this reader has always accepted it, so the open levels are
+        bool indefinite = false;
-        // counted instead of recursed through, which overflowed the stack for
-        // an input of repeated 0x7F bytes (see #5104). Every chunk is appended
-        // to the same result, so no per-level state is needed.
-        std::size_t open = 0;
 
         while (true)
         {
@@ -1182,29 +1193,28 @@ class binary_reader
 
             if (current == 0x7F) // UTF-8 string (indefinite length)
             {
-                ++open;
+                if (JSON_HEDLEY_UNLIKELY(indefinite))
-                get();
-                continue;
-            }
-
-            // a break marker closes the innermost indefinite-length string;
-            // outside of one it is not a string and falls through to the error
-            if (open != 0 && current == 0xFF)
-            {
-                if (--open == 0)
                 {
-                    return check_string_utf8(result, context);
+                    return cbor_indefinite_string_error("string", "string");
                 }
+                indefinite = true;
                 get();
                 continue;
             }
 
-            if (JSON_HEDLEY_UNLIKELY(!get_cbor_string_chunk(result)))
+            // a break marker closes the indefinite-length string; outside
+            // of one it falls through to the error below
+            if (indefinite && current == 0xFF)
+            {
+                return check_string_utf8(result, context);
+            }
+
+            if (JSON_HEDLEY_UNLIKELY(!get_cbor_string_chunk(result, indefinite)))
             {
                 return false;
             }
 
-            if (open == 0)
+            if (!indefinite)
             {
                 return check_string_utf8(result, context);
             }
@@ -1296,12 +1306,13 @@ class binary_reader
     read into the same byte array.
 
     @param[out] result  byte array the bytes are appended to
+    @param[in] inside_indefinite  whether the bytes belong to an indefinite-length string
 
     @return whether byte array creation completed
 
     @pre @a current is not EOF
     */
-    bool get_cbor_binary_chunk(binary_t& result)
+    bool get_cbor_binary_chunk(binary_t& result, const bool inside_indefinite)
     {
         switch (current)
         {
@@ -1366,7 +1377,7 @@ class binary_reader
             {
                 auto last_token = get_token_string();
                 return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
-                                        exception_message(concat("expected length specification (0x40-0x5B) or indefinite binary array type (0x5F); last byte: 0x", last_token), "binary"), nullptr));
+                                        exception_message(concat("expected length specification (0x40-0x5B)", inside_indefinite ? "" : " or indefinite binary array type (0x5F)", "; last byte: 0x", last_token), "binary"), nullptr));
             }
         }
     }
@@ -1384,9 +1395,9 @@ class binary_reader
     */
     bool get_cbor_binary(binary_t& result)
     {
-        // the open indefinite-length byte arrays are counted rather than
+        // read chunks iteratively, but reject a second indefinite-length
-        // recursed through, for the reason given in @ref get_cbor_string
+        // level as required by RFC 8949, Section 3.2.3
-        std::size_t open = 0;
+        bool indefinite = false;
 
         while (true)
         {
@@ -1397,29 +1408,28 @@ class binary_reader
 
             if (current == 0x5F) // Binary data (indefinite length)
             {
-                ++open;
+                if (JSON_HEDLEY_UNLIKELY(indefinite))
-                get();
-                continue;
-            }
-
-            // a break marker closes the innermost indefinite-length byte
-            // array; outside of one it falls through to the error below
-            if (open != 0 && current == 0xFF)
-            {
-                if (--open == 0)
                 {
-                    return true;
+                    return cbor_indefinite_string_error("binary array", "binary");
                 }
+                indefinite = true;
                 get();
                 continue;
             }
 
-            if (JSON_HEDLEY_UNLIKELY(!get_cbor_binary_chunk(result)))
+            // a break marker closes the indefinite-length string; outside
+            // of one it falls through to the error below
+            if (indefinite && current == 0xFF)
+            {
+                return true;
+            }
+
+            if (JSON_HEDLEY_UNLIKELY(!get_cbor_binary_chunk(result, indefinite)))
             {
                 return false;
             }
 
-            if (open == 0)
+            if (!indefinite)
             {
                 return true;
             }

single_include/nlohmann/json.hpp

@@ -14789,6 +14789,20 @@ class binary_reader
         }
     }
 
+    /*!
+    @brief reports a nested indefinite-length CBOR string or byte array
+    @param[in] type_name  name of the rejected string type
+    @param[in] context  parsing context for the error message
+    @return whether the SAX consumer accepts the parse error
+    */
+    bool cbor_indefinite_string_error(const char* type_name, const char* context)
+    {
+        auto last_token = get_token_string();
+        return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
+                                exception_message(concat("indefinite-length ", type_name,
+                                        " is not allowed inside indefinite-length ", type_name, "; last byte: 0x", last_token), context), nullptr));
+    }
+
     /*!
     @brief reads a definite-length CBOR string
 
@@ -14798,12 +14812,13 @@ class binary_reader
     into the same string.
 
     @param[out] result  string the bytes are appended to
+    @param[in] inside_indefinite  whether the bytes belong to an indefinite-length string
 
     @return whether string creation completed
 
     @pre @a current is not EOF
     */
-    bool get_cbor_string_chunk(string_t& result)
+    bool get_cbor_string_chunk(string_t& result, const bool inside_indefinite)
     {
         switch (current)
         {
@@ -14864,7 +14879,7 @@ class binary_reader
             {
                 auto last_token = get_token_string();
                 return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
-                                        exception_message(concat("expected length specification (0x60-0x7B) or indefinite string type (0x7F); last byte: 0x", last_token), "string"), nullptr));
+                                        exception_message(concat("expected length specification (0x60-0x7B)", inside_indefinite ? "" : " or indefinite string type (0x7F)", "; last byte: 0x", last_token), "string"), nullptr));
             }
         }
     }
@@ -14882,13 +14897,9 @@ class binary_reader
     */
     bool get_cbor_string(string_t& result, const char* context = "string")
     {
-        // number of indefinite-length strings that have been opened and not
+        // read chunks iteratively, but reject a second indefinite-length
-        // closed yet. RFC 8949, Section 3.2.3 does not permit nesting them,
+        // level as required by RFC 8949, Section 3.2.3
-        // but this reader has always accepted it, so the open levels are
+        bool indefinite = false;
-        // counted instead of recursed through, which overflowed the stack for
-        // an input of repeated 0x7F bytes (see #5104). Every chunk is appended
-        // to the same result, so no per-level state is needed.
-        std::size_t open = 0;
 
         while (true)
         {
@@ -14899,29 +14910,28 @@ class binary_reader
 
             if (current == 0x7F) // UTF-8 string (indefinite length)
             {
-                ++open;
+                if (JSON_HEDLEY_UNLIKELY(indefinite))
-                get();
-                continue;
-            }
-
-            // a break marker closes the innermost indefinite-length string;
-            // outside of one it is not a string and falls through to the error
-            if (open != 0 && current == 0xFF)
-            {
-                if (--open == 0)
                 {
-                    return check_string_utf8(result, context);
+                    return cbor_indefinite_string_error("string", "string");
                 }
+                indefinite = true;
                 get();
                 continue;
             }
 
-            if (JSON_HEDLEY_UNLIKELY(!get_cbor_string_chunk(result)))
+            // a break marker closes the indefinite-length string; outside
+            // of one it falls through to the error below
+            if (indefinite && current == 0xFF)
+            {
+                return check_string_utf8(result, context);
+            }
+
+            if (JSON_HEDLEY_UNLIKELY(!get_cbor_string_chunk(result, indefinite)))
             {
                 return false;
             }
 
-            if (open == 0)
+            if (!indefinite)
             {
                 return check_string_utf8(result, context);
             }
@@ -15013,12 +15023,13 @@ class binary_reader
     read into the same byte array.
 
     @param[out] result  byte array the bytes are appended to
+    @param[in] inside_indefinite  whether the bytes belong to an indefinite-length string
 
     @return whether byte array creation completed
 
     @pre @a current is not EOF
     */
-    bool get_cbor_binary_chunk(binary_t& result)
+    bool get_cbor_binary_chunk(binary_t& result, const bool inside_indefinite)
     {
         switch (current)
         {
@@ -15083,7 +15094,7 @@ class binary_reader
             {
                 auto last_token = get_token_string();
                 return sax->parse_error(chars_read, last_token, parse_error::create(113, chars_read,
-                                        exception_message(concat("expected length specification (0x40-0x5B) or indefinite binary array type (0x5F); last byte: 0x", last_token), "binary"), nullptr));
+                                        exception_message(concat("expected length specification (0x40-0x5B)", inside_indefinite ? "" : " or indefinite binary array type (0x5F)", "; last byte: 0x", last_token), "binary"), nullptr));
             }
         }
     }
@@ -15101,9 +15112,9 @@ class binary_reader
     */
     bool get_cbor_binary(binary_t& result)
     {
-        // the open indefinite-length byte arrays are counted rather than
+        // read chunks iteratively, but reject a second indefinite-length
-        // recursed through, for the reason given in @ref get_cbor_string
+        // level as required by RFC 8949, Section 3.2.3
-        std::size_t open = 0;
+        bool indefinite = false;
 
         while (true)
         {
@@ -15114,29 +15125,28 @@ class binary_reader
 
             if (current == 0x5F) // Binary data (indefinite length)
             {
-                ++open;
+                if (JSON_HEDLEY_UNLIKELY(indefinite))
-                get();
-                continue;
-            }
-
-            // a break marker closes the innermost indefinite-length byte
-            // array; outside of one it falls through to the error below
-            if (open != 0 && current == 0xFF)
-            {
-                if (--open == 0)
                 {
-                    return true;
+                    return cbor_indefinite_string_error("binary array", "binary");
                 }
+                indefinite = true;
                 get();
                 continue;
             }
 
-            if (JSON_HEDLEY_UNLIKELY(!get_cbor_binary_chunk(result)))
+            // a break marker closes the indefinite-length string; outside
+            // of one it falls through to the error below
+            if (indefinite && current == 0xFF)
+            {
+                return true;
+            }
+
+            if (JSON_HEDLEY_UNLIKELY(!get_cbor_binary_chunk(result, indefinite)))
             {
                 return false;
             }
 
-            if (open == 0)
+            if (!indefinite)
             {
                 return true;
             }

tests/src/unit-cbor.cpp

@@ -1699,7 +1699,7 @@ TEST_CASE("CBOR")
             CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0xA1, 0x61, 0X61})), "[json.exception.parse_error.110] parse error at byte 4: syntax error while parsing CBOR value: unexpected end of input", json::parse_error&);
             CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0xBF, 0x61, 0X61})), "[json.exception.parse_error.110] parse error at byte 4: syntax error while parsing CBOR value: unexpected end of input", json::parse_error&);
             CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x5F})), "[json.exception.parse_error.110] parse error at byte 2: syntax error while parsing CBOR binary: unexpected end of input", json::parse_error&);
-            CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x5F, 0x00})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR binary: expected length specification (0x40-0x5B) or indefinite binary array type (0x5F); last byte: 0x00", json::parse_error&);
+            CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x5F, 0x00})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR binary: expected length specification (0x40-0x5B); last byte: 0x00", json::parse_error&);
             CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x41})), "[json.exception.parse_error.110] parse error at byte 2: syntax error while parsing CBOR binary: unexpected end of input", json::parse_error&);
 
             CHECK(json::from_cbor(std::vector<uint8_t>({0x18}), true, false).is_discarded());
@@ -2305,22 +2305,21 @@ TEST_CASE("CBOR indefinite-length strings do not recurse per chunk")
 {
     // Reading an indefinite-length string or byte array used to call itself
     // once per chunk, so a payload of repeated 0x7F (or 0x5F) bytes exhausted
-    // the call stack before any of the input was rejected. The open levels are
+    // the call stack before any of the input was rejected. Nested indefinite
-    // counted now, and the levels below prove the reader still reads the same
+    // chunks are now rejected at the second byte, without recursing.
-    // values and reports the same errors at the same byte offsets.
     json _;
 
-    SECTION("many open levels are reported, not crashed on")
+    SECTION("nested levels are rejected, not crashed on")
     {
         const std::vector<uint8_t> input(200000, 0x7F);
-        CHECK_THROWS_WITH_AS(_ = json::from_cbor(input), "[json.exception.parse_error.110] parse error at byte 200001: syntax error while parsing CBOR string: unexpected end of input", json::parse_error&);
+        CHECK_THROWS_WITH_AS(_ = json::from_cbor(input), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR string: indefinite-length string is not allowed inside indefinite-length string; last byte: 0x7F", json::parse_error&);
         CHECK(json::from_cbor(input, true, false).is_discarded());
     }
 
-    SECTION("many open levels are reported, not crashed on (binary)")
+    SECTION("nested levels are rejected, not crashed on (binary)")
     {
         const std::vector<uint8_t> input(200000, 0x5F);
-        CHECK_THROWS_WITH_AS(_ = json::from_cbor(input), "[json.exception.parse_error.110] parse error at byte 200001: syntax error while parsing CBOR binary: unexpected end of input", json::parse_error&);
+        CHECK_THROWS_WITH_AS(_ = json::from_cbor(input), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR binary: indefinite-length binary array is not allowed inside indefinite-length binary array; last byte: 0x5F", json::parse_error&);
         CHECK(json::from_cbor(input, true, false).is_discarded());
     }
 
@@ -2328,22 +2327,22 @@ TEST_CASE("CBOR indefinite-length strings do not recurse per chunk")
     {
         CHECK(json::from_cbor(std::vector<uint8_t>({0x7F, 0xFF})) == json(""));
         CHECK(json::from_cbor(std::vector<uint8_t>({0x7F, 0x61, 0x61, 0xFF})) == json("a"));
-        // nested indefinite-length strings are concatenated across levels
+        // empty and nonempty definite-length chunks concatenate in order
-        CHECK(json::from_cbor(std::vector<uint8_t>({0x7F, 0x7F, 0x61, 0x61, 0xFF, 0x61, 0x62, 0xFF})) == json("ab"));
+        CHECK(json::from_cbor(std::vector<uint8_t>({0x7F, 0x61, 'a', 0x60, 0x61, 'b', 0x61, 'c', 0xFF})) == json("abc"));
-        CHECK(json::from_cbor(std::vector<uint8_t>({0x7F, 0x7F, 0x7F, 0x61, 0x7A, 0xFF, 0xFF, 0xFF})) == json("z"));
         CHECK(json::from_cbor(std::vector<uint8_t>({0xA1, 0x7F, 0x61, 0x61, 0xFF, 0x01})) == json({{"a", 1}}));
     }
 
     SECTION("chunks are still concatenated (binary)")
     {
         CHECK(json::from_cbor(std::vector<uint8_t>({0x5F, 0x41, 0x61, 0xFF})) == json::binary({0x61}));
-        CHECK(json::from_cbor(std::vector<uint8_t>({0x5F, 0x5F, 0x41, 0x61, 0xFF, 0x41, 0x62, 0xFF})) == json::binary({0x61, 0x62}));
+        CHECK(json::from_cbor(std::vector<uint8_t>({0x5F, 0xFF})) == json::binary({}));
+        CHECK(json::from_cbor(std::vector<uint8_t>({0x5F, 0x41, 0x61, 0x40, 0x41, 0x62, 0x41, 0x63, 0xFF})) == json::binary({0x61, 0x62, 0x63}));
     }
 
     SECTION("a chunk that is not a string is still rejected")
     {
-        CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x7F, 0x7F, 0x00})), "[json.exception.parse_error.113] parse error at byte 3: syntax error while parsing CBOR string: expected length specification (0x60-0x7B) or indefinite string type (0x7F); last byte: 0x00", json::parse_error&);
+        CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x7F, 0x00})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR string: expected length specification (0x60-0x7B); last byte: 0x00", json::parse_error&);
-        CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x5F, 0x5F, 0x00})), "[json.exception.parse_error.113] parse error at byte 3: syntax error while parsing CBOR binary: expected length specification (0x40-0x5B) or indefinite binary array type (0x5F); last byte: 0x00", json::parse_error&);
+        CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<uint8_t>({0x5F, 0x00})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR binary: expected length specification (0x40-0x5B); last byte: 0x00", json::parse_error&);
     }
 
     SECTION("a break marker outside an indefinite-length string is not a string")
@@ -2896,9 +2895,17 @@ TEST_CASE("examples from RFC 8949 Appendix A")
     {
         const auto packed = utils::read_binary_file(TEST_DATA_DIRECTORY "/binary_data/cbor_binary.cbor");
         json j;
-        CHECK_NOTHROW(j = json::from_cbor(packed));
+        // the fixture's tail contains nested indefinite-length byte strings.
+        CHECK_THROWS_WITH_AS(j = json::from_cbor(packed), "[json.exception.parse_error.113] parse error at byte 513: syntax error while parsing CBOR binary: indefinite-length binary array is not allowed inside indefinite-length binary array; last byte: 0x5F", json::parse_error&);
 
-        const auto expected = utils::read_binary_file(TEST_DATA_DIRECTORY "/binary_data/cbor_binary.out");
+        // keep the byte-for-byte decoding check for its valid prefix: the first
+        // 512 encoded bytes contain 468 payload bytes in definite-length chunks.
+        auto valid_prefix = packed;
+        valid_prefix.resize(512);
+        valid_prefix.push_back(0xFF);
+        auto expected = utils::read_binary_file(TEST_DATA_DIRECTORY "/binary_data/cbor_binary.out");
+        expected.resize(468);
+        CHECK_NOTHROW(j = json::from_cbor(valid_prefix));
         CHECK(j == json::binary(expected));
 
         // 0xd8

tests/src/unit-regression3.cpp

@@ -920,4 +920,12 @@ TEST_CASE("regression test #5476 - array type without reserve()")
     }
 }
 
+TEST_CASE("issue #5317 - nested indefinite-length CBOR string chunks are rejected")
+{
+    json _;
+    CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<std::uint8_t>({0x7F, 0x7F, 0x61, 0x61, 0xFF, 0xFF})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR string: indefinite-length string is not allowed inside indefinite-length string; last byte: 0x7F", json::parse_error&);
+    CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<std::uint8_t>({0x5F, 0x5F, 0x41, 0x61, 0xFF, 0xFF})), "[json.exception.parse_error.113] parse error at byte 2: syntax error while parsing CBOR binary: indefinite-length binary array is not allowed inside indefinite-length binary array; last byte: 0x5F", json::parse_error&);
+    CHECK_THROWS_WITH_AS(_ = json::from_cbor(std::vector<std::uint8_t>({0xA1, 0x7F, 0x7F, 0xFF, 0xFF, 0x01})), "[json.exception.parse_error.113] parse error at byte 3: syntax error while parsing CBOR string: indefinite-length string is not allowed inside indefinite-length string; last byte: 0x7F", json::parse_error&);
+}
+
 DOCTEST_CLANG_SUPPRESS_WARNING_POP