Write and read the inner length of BSON binary subtype 0x02

The BSON specification defines the payload of the "binary (old)" subtype
0x02 as an int32 followed by that many bytes. to_bson wrote the plain
payload instead, so decoders such as pymongo and libbson-based readers
rejected the bytes, and from_bson returned the inner length as part of the
payload (documented as lenient behavior in gh-5333).

The writer now emits the outer length (payload + 4), the subtype, the inner
int32 holding the payload size, and the payload. The reader requires an
outer length of at least 4 and an inner length equal to the outer length
minus 4 (the cases the bson-corpus lists as decode errors), reports parse
error 112 otherwise, and returns only the payload. Both directions change
together so that the round trip keeps working.

The single-reservation fast path for scalar entries declines subtype 0x02
because it has no room for the inner length.

This is a deliberate exception to the opt-in rule for behavior changes:
the previous output was invalid BSON.

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

include/nlohmann/detail/input/binary_reader.hpp

@@ -584,9 +584,46 @@ class binary_reader
         }
         result.set_subtype(subtype);
 
+        // the "old binary" subtype 0x02 starts with an int32 that repeats the
+        // size of the payload following it; the payload does not include it
+        if (subtype == 0x02)
+        {
+            return get_bson_old_binary(len, result);
+        }
+
         return get_binary(len, result);
     }
 
+    /*!
+    @brief Parses the payload of a BSON binary value with subtype 0x02.
+    @param[in] len  The length of the binary value, including the inner int32.
+    @param[in,out] result  A reference to the binary variable where the payload
+                           (without the inner int32) is to be stored.
+    @pre len >= 0
+    @return `true` if the inner length was present and matches @a len
+    */
+    template<typename NumberType>
+    bool get_bson_old_binary(const NumberType len, binary_t& result)
+    {
+        if (JSON_HEDLEY_UNLIKELY(len < 4))
+        {
+            return bson_length_error("length of binary subtype 0x02 must be at least 4, is ", len, "binary");
+        }
+
+        std::int32_t inner_length{};
+        if (JSON_HEDLEY_UNLIKELY((!get_number<std::int32_t, true>(inner_length))))
+        {
+            return false;
+        }
+
+        if (JSON_HEDLEY_UNLIKELY(static_cast<std::int64_t>(inner_length) != static_cast<std::int64_t>(len) - 4))
+        {
+            return bson_length_error("inner length of binary subtype 0x02 does not match the payload size; inner length is ", inner_length, "binary");
+        }
+
+        return get_binary(inner_length, result);
+    }
+
     /*!
     @brief Read a BSON document element of the given @a element_type.
     @param[in] element_type The BSON element type, c.f. http://bsonspec.org/spec.html

include/nlohmann/detail/output/binary_writer.hpp

@@ -1684,7 +1684,15 @@ class binary_writer
             throw_subtype_out_of_range(value.subtype(), "the BSON binary subtype", j);
         }
 
-        return sizeof(std::int32_t) + value.size() + 1ul;
+        // the "old binary" subtype 0x02 repeats the payload size in an inner
+        // int32 in front of the payload
+        return sizeof(std::int32_t) + (is_bson_old_binary(value) ? sizeof(std::int32_t) : 0ul) + value.size() + 1ul;
+    }
+
+    /// @return whether @a value has the BSON subtype 0x02 ("binary (old)")
+    static bool is_bson_old_binary(const binary_t& value) noexcept
+    {
+        return value.has_subtype() && value.subtype() == 0x02;
     }
 
     /*!
@@ -1697,10 +1705,18 @@ class binary_writer
     {
         write_bson_entry_header(name, 0x05);
 
-        write_number<std::int32_t>(to_bson_length(value.size()), true);
+        const bool old_binary = is_bson_old_binary(value);
+
+        // the length field counts the inner int32 of the old binary subtype
+        write_number<std::int32_t>(to_bson_length(value.size() + (old_binary ? sizeof(std::int32_t) : 0ul)), true);
 
         write_number(value.has_subtype() ? static_cast<std::uint8_t>(value.subtype()) : static_cast<std::uint8_t>(0x00));
 
+        if (old_binary)
+        {
+            write_number<std::int32_t>(to_bson_length(value.size()), true);
+        }
+
         write_bytes(value.data(), value.size());
     }
 
@@ -1860,7 +1876,8 @@ class binary_writer
            (or the header of a string or binary value) in one reservation
     @return whether the entry was written; false for a discarded value, a
             string or binary value whose length does not fit the length
-            field, in which case nothing was written
+            field, or a binary value with subtype 0x02, in which case nothing
+            was written
     @pre    @a name is at most bson_scalar_max_name_size characters long and is
             not changed by the error handler
     */
@@ -1921,7 +1938,9 @@ class binary_writer
         if (j.is_binary())
         {
             const binary_t& value = *j.m_data.m_value.binary;
-            if (JSON_HEDLEY_UNLIKELY(!value_in_range_of<std::int32_t>(value.size())))
+            // the old binary subtype 0x02 has an inner length that the single
+            // reservation has no room for; write_bson_binary writes it
+            if (JSON_HEDLEY_UNLIKELY(!value_in_range_of<std::int32_t>(value.size()) || is_bson_old_binary(value)))
             {
                 return false;
             }

tests/src/unit-bson.cpp

@@ -802,12 +802,11 @@ TEST_CASE("BSON")
             CHECK(json::from_bson(result, true, false) == j);
         }
 
-        SECTION("binary member with subtype 0x02 (old binary) keeps its inner length prefix (lenient parsing)")
+        SECTION("binary member with subtype 0x02 (old binary) carries an inner length")
         {
-            // documented lenient behavior (see gh-5333): the payload for
-            // binary subtype 0x02 ("old binary") is returned as-is,
-            // including its own inner 4-byte length prefix; it is not
-            // stripped or reinterpreted
+            // the payload of binary subtype 0x02 ("old binary") starts with an
+            // int32 that repeats the payload size; the bytes below are what
+            // pymongo and the bson-corpus (binary.json) use
             std::vector<std::uint8_t> const input =
             {
                 0x17, 0x00, 0x00, 0x00, // size (little endian)
@@ -815,16 +814,87 @@ TEST_CASE("BSON")
 
                 0x06, 0x00, 0x00, 0x00, // size of binary (little endian)
                 0x02, // "old binary" subtype
-                0x02, 0x00, 0x00, 0x00, // inner length prefix (part of the old-binary payload)
+                0x02, 0x00, 0x00, 0x00, // inner length
                 0x68, 0x69, // payload ('h', 'i')
 
                 0x00 // end marker
             };
 
-            // the inner length prefix is part of the (unmodified) payload
-            const std::vector<std::uint8_t> expected_payload = {0x02, 0x00, 0x00, 0x00, 0x68, 0x69};
-            const json expected = { { "entry", json::binary(expected_payload, 0x02) } };
+            // the inner length is not part of the payload
+            const json expected = { { "entry", json::binary({0x68, 0x69}, 0x02) } };
             CHECK(json::from_bson(input) == expected);
+            CHECK(json::to_bson(expected) == input);
+            CHECK(json::from_bson(json::to_bson(expected)) == expected);
+        }
+
+        SECTION("binary member with subtype 0x02 (old binary) and empty payload")
+        {
+            std::vector<std::uint8_t> const input =
+            {
+                0x15, 0x00, 0x00, 0x00, // size (little endian)
+                0x05, 'e', 'n', 't', 'r', 'y', '\x00', // entry: binary
+                0x04, 0x00, 0x00, 0x00, // size of binary (little endian)
+                0x02, // "old binary" subtype
+                0x00, 0x00, 0x00, 0x00, // inner length
+                0x00 // end marker
+            };
+
+            const json expected = { { "entry", json::binary({}, 0x02) } };
+            CHECK(json::from_bson(input) == expected);
+            CHECK(json::to_bson(expected) == input);
+        }
+
+        SECTION("binary member with subtype 0x02 (old binary) with an invalid inner length")
+        {
+            std::vector<std::uint8_t> input =
+            {
+                0x17, 0x00, 0x00, 0x00, // size (little endian)
+                0x05, 'e', 'n', 't', 'r', 'y', '\x00', // entry: binary
+                0x06, 0x00, 0x00, 0x00, // size of binary (little endian)
+                0x02, // "old binary" subtype
+                0x02, 0x00, 0x00, 0x00, // inner length
+                0x68, 0x69, // payload ('h', 'i')
+                0x00 // end marker
+            };
+
+            SECTION("inner length too long")
+            {
+                input[19] = 0x03;
+            }
+
+            SECTION("inner length too short")
+            {
+                input[19] = 0x01;
+            }
+
+            SECTION("inner length negative")
+            {
+                input[19] = 0xFF;
+                input[20] = 0xFF;
+                input[21] = 0xFF;
+                input[22] = 0xFF;
+            }
+
+            CAPTURE(input)
+            json _;
+            CHECK_THROWS_AS(_ = json::from_bson(input), json::parse_error&);
+        }
+
+        SECTION("binary member with subtype 0x02 (old binary) without an inner length")
+        {
+            // the payload is shorter than the inner length field
+            std::vector<std::uint8_t> const input =
+            {
+                0x14, 0x00, 0x00, 0x00, // size (little endian)
+                0x05, 'e', 'n', 't', 'r', 'y', '\x00', // entry: binary
+                0x03, 0x00, 0x00, 0x00, // size of binary (little endian)
+                0x02, // "old binary" subtype
+                0x01, 0x02, 0x03, // payload without an inner length
+                0x00 // end marker
+            };
+
+            json _;
+            CHECK_THROWS_AS(_ = json::from_bson(input), json::parse_error&);
         }
 
         SECTION("Some more complex document")