Incorrect parsing of indefinite length CBOR strings.

[johnfb]

I found this library and thought it was very interesting and like a lot about it and decided to brows the source code a little bit. I am thinking I will try using this in my next project. In my browsing I found the following problem:

json/include/nlohmann/detail/input/binary_reader.hpp

Line 946 in 737cffe

case 0x7F: // UTF-8 string (indefinite length)

According to section 2.2.2 of the CBOR RFC 7049 an indefinite length string is a list of definite length strings terminated by the terminal byte 0xFF. As such the following code snippet should print the same string twice.

    using json = nlohmann::json;
    std::vector<uint8_t> v_cbor = {
        0x7F,
            0x64,
                'a', 'b', 'c', 'd',
            0x63,
                '1', '2', '3',
            0xFF
    };
    json j = json::from_cbor(v_cbor);
    std::cout << "\"abcd123\"\n";
    std::cout <<  j << std::endl;

But as of version 3.1.0 it prints:

"abcd123"
"dabcdc123"

Changing that case to something like:

            case 0x7F: // UTF-8 string (indefinite length)
            {
                std::vector<string_t> result_list;
                std::size_t size = 0;
                while (get() != 0xFF)
                {
                    unexpect_eof();
                    result_list.push_back(get_cbor_string());
                    size += result_list.back().size();
                }
                string_t result;
                result.reserve(size);
                for (auto r: result_list) { result.append(r); }
                return result;
            }

Fixes it. I'm not sure if this is the way you want to do it though.

[nlohmann]

I can confirm your observation, and I also checked with the CBOR playground:

I honestly must have missed that part and implemented indefinite length strings just like maps and arrays. Thanks for checking the code so thoroughly!

I think your fix is valid - I just have the feeling there is an STL algorithm (accumulate?) to merge the strings. Alternatively, one could just add the parsed strings directly to the result string.

[johnfb]

Simply appending to the string directly certainly is simpler, and only a one line change:

            case 0x7F: // UTF-8 string (indefinite length)
            {
                string_t result;
                while (get() != 0xFF)
                {
                    unexpect_eof();
                    result.append(get_cbor_string());
                }
                return result;
            }

Indefinite length strings are probably such a rare case that doing things like reserving capacity to save allocation as the first one does... premature optimization and therefore evil. 😜

[gregmarr]

With the secondary array, you're going to end up with guaranteed two string copies, one allocation, and one deallocation per string in the indefinite string (unless those strings fit in the small string optimization). If you do it as.... as @johnfb just wrote as I was writing this... you will have the same count as upper bound, and better performance in general.

[nlohmann]

Thanks! And after all, you would also need allocations to copy the strings into the vector.

[nlohmann]

Kudos for @gregmarr to always be so fast ;)

[nlohmann]

Alright - I'll add a fix later today.

[nlohmann]

Thanks everybody!