Abstract reader/writer issue with non-contigous input data

[CrustyAuklet]

So I think this may be related to and/or covered by #208 , but I wanted to open an issue to report the specifics of what i am seeing.

I think the root of the issue is that in the structure CborParserOperations the function pointer transfer_string should advance the cursor but when calling CborError cbor_value_copy_text_string(const CborValue *value, char *buffer, size_t *buflen, CborValue *next) the cursor should be advanced for the output parameter next but not advanced in the in-out parameter value. I think the user data token* is shared between copies of CborValue.

I am working in an embedded system and transitioning some existing code to use a struct netbuff like structure instead of just copying around worst-case sized buffers. To do this i need to move all my code to work on iterators instead of just taking a std::span<uint8_t> (we are on C++20). The iterators to this new structure are bidirectional_iterator, so no random access.

A simple (exposition only) example of the code that fails:

detail::ParserState<InItr, S> cbor_parse;
CborValue it;
cbor_parse.init(data.begin(), data.end(), it);
CborValue it2;
char func_name_buff[10]{};
size_t name_len = 10;
if (!cbor_value_is_text_string(&it)) {
    return BadValue;
}
if (auto err = cbor_value_copy_text_string(&it, func_name_buff, &name_len, &it2) != CborNoError) {
    return err;
}
auto errr = cbor_value_advance(&it);    // ERROR: CborErrorIllegalType
auto type = cbor_value_get_type(&it)    // CborTextStringType, should be NullType after the advance
auto type2 = cbor_value_get_type(&it2)  // CborNullType

I will work on getting a minimal reproducible example into compiler explorer, especially if this is a new case.

[CrustyAuklet]

Getting it into CE looked like a lot of effort, so I just added a test case to my project. Here is the test case code. Only requires tinycbor and catch2. And I guess you could always replace all the catch2 test cases with asserts :)

EDIT: just FYI, I changed cbor_value_copy_text_string to a simple cbor_value_text_string_equals and the result is the same.

#include <catch2/catch_test_macros.hpp>
#include <cbor.h>
#include <cassert>
#include <iterator>
#include <array>
#include <vector>
#include <algorithm>

namespace {

    template <std::input_iterator InItr, std::sentinel_for<InItr> S>
    struct ParserState {
        InItr itr;
        S end;
        CborParser parser_;
        std::array<uint8_t, 16> str_buff_;

        static inline CborParserOperations ops_{
            .can_read_bytes=[](void* token, size_t len) ->bool {
                auto state = static_cast<ParserState *>(token);
                return state->itr != state->end;
            },
            .read_bytes=[](void* token, void* dst, size_t offset, size_t len) ->void* {
                auto* state = static_cast<ParserState *>(token);
                auto* dest = static_cast<uint8_t*>(dst);
                auto local_itr = state->itr;
                std::advance(local_itr, offset);
                std::copy_n(local_itr, len, dest);
                return dst;
            },
            .advance_bytes=[](void *token, size_t len) ->void {
                auto state = static_cast<ParserState *>(token);
                std::advance(state->itr, len);
            },
            .transfer_string=[](void *token, const void **userptr, size_t offset, size_t len) ->CborError {
                auto state = static_cast<ParserState *>(token);
                assert(len <= state->str_buff_.size());
                *userptr = state->str_buff_.data();
                std::advance(state->itr, offset);
                for (int i = 0; i < len; ++i) {
                    if (state->itr == state->end) {
                        return CborErrorUnexpectedEOF;
                    }
                    state->str_buff_[i] = *state->itr++;
                }
                return CborNoError;
            }
        };

        CborError init(InItr b, S e, CborValue& val) {
            itr = b;
            end = e;
            return cbor_parser_init_reader(&ops_, &parser_, &val, this);
        }
    };

} // namespace

TEST_CASE("CBOR stream reader") {
    // [90, "inf", null]
    std::vector<uint8_t> data{0x83,0x18,0x5a,0x63,0x69,0x6e,0x66,0xf6};
    using I = decltype(data.begin());
    using S = decltype(data.end());

    ParserState<I,S> cbor_parse;
    CborValue it;
    REQUIRE(cbor_parse.init(data.begin(), data.end(), it) == CborNoError);

    // make sure we have an array of 3 items as expected, and then enter it
    REQUIRE(cbor_value_is_array(&it));
    size_t sz = 0;
    cbor_value_get_array_length(&it, &sz);
    REQUIRE(sz == 3);
    REQUIRE(cbor_value_enter_container(&it, &it) == CborNoError);

    // extract the first item: integer that should be 90
    REQUIRE(cbor_value_is_integer(&it));
    uint64_t number = 0;
    REQUIRE(cbor_value_get_uint64(&it, &number) == CborNoError);
    REQUIRE(number == 90);

    // Second item should be a string that reads 'inf'
    REQUIRE(cbor_value_advance(&it) == CborNoError);
    REQUIRE(cbor_value_is_text_string(&it));

    char func_name_buff[10]{};
    size_t name_len = 10;
    CborValue it2;
    REQUIRE(cbor_value_copy_text_string(&it, func_name_buff, &name_len, &it2) == CborNoError);
    REQUIRE(name_len == 3);
    REQUIRE(strncmp(func_name_buff, "inf", 3) == 0);

    // after that operation 'it' should still be the string,
    // and 'it2' should point to the next item which is NULL
    REQUIRE(cbor_value_is_text_string(&it));
    REQUIRE(cbor_value_is_null(&it2));

    // Advance the value to the last item, which is a NULL value
    // this is where it all falls apart!
    CHECK(cbor_value_advance(&it) == CborNoError);  // ERROR: CborErrorIllegalType
    CHECK(cbor_value_is_null(&it));  // CborTextStringType, should be NullType!
}

[matusnovak]

I have hit this issue as well.

One dirty way to partially solve it, if you do not wish to modify the library code, is the following:

(1) Before calling cbor_value_advance to get to the next value, remember that you are advancing. Some kind of flag. You could use the unused upper bits space in the parser flags. Like this:

reinterpret_cast<uint32_t&>(const_cast<CborParser*>(value.parser)->flags) |= 0x800000;
const auto res = cbor_value_advance(&value);

(2) In the ops function transfer_string, do not move the current iterator to the next position. Move the iterator only if such flag is set.

In my code I do:

// "consumed" tracks how many bytes have been consumed already
*userptr = buffer.data() + consumed + offset;

auto& flags = reinterpret_cast<uint32_t&>(parser.flags);
if (flags & 0x800000) {
    consumed += len + offset;
    flags &= ~0x800000;
}

(3) When ops function advance_bytes is called, you must clear the flag:

auto& flags = reinterpret_cast<uint32_t&>(parser.flags);
flags &= ~0x800000;

Why this works?

When you are reading bytes or string, you should not advance past the string's position. You should stay where you are, so that the parser remembers the string's length. Calling advance must re-read the string's length and transfer the string.

When this won't work?

If you are advancing across a nested structure, like a map, then you are out of luck, the advance_recursive is used internally that calls transfer string multiple times. One way to solve this would be to modify the source code of the cbor_value_advance function, so that it sets the flag each time it needs to advance. Of course, chunked strings are also problematic.

[matusnovak]

Continuing my previous comment...

Here is my C++ wrapper around the cbor_parser_init_reader. It assumes that the Reader template type has the following two member functions:

size_t read(void* dst, size_t length);
bool eof() const;

I don't know if this is 100% correct, and assumes the flag is set (see my previous comment), and has the limitations as I have mentioned.

template <typename T>
concept ReaderConcept = requires(T reader, void* dst, size_t length) {
    { reader.read(dst, length) } -> std::same_as<size_t>;
    { reader.eof() } -> std::same_as<bool>;
    { std::as_const(reader).eof() } -> std::same_as<bool>;
};

template <ReaderConcept Reader> class CborChunkReader : public Cbor::Visitor {
public:
    explicit CborChunkReader(Reader& reader, CborValue& value) : reader{reader} {
        total = reader.read(chunk.data(), chunk.size());

        CborError err = cbor_parser_init_reader(&kOps, &parser, &value, this);
        if (err != CborNoError) {
            throw std::runtime_error("Failed to init cbor parser");
        }
    }

private:
    CborError truncateAndReadMore() {
        if (reader.eof()) {
            return CborErrorUnexpectedEOF;
        }

        // Move bytes to left
        const auto bytesLeft = total - consumed;
        std::memmove(chunk.data(), chunk.data() + consumed, bytesLeft);

        // Reset counters
        consumed = 0;
        total = bytesLeft;

        // Read more bytes to fill the empty space in the buffer
        total += reader.read(chunk.data() + bytesLeft, chunk.size() - bytesLeft);

        return CborNoError;
    }

    bool canReadBytes(const size_t len) {
        if (consumed < total) {
            return true;
        }

        return !!reader.eof();
    }

    void* readBytes(void* dst, const size_t offset, const size_t len) {
        // Do we need to read more?
        if (total - consumed < offset + len) {
            if (const auto res = truncateAndReadMore(); res != CborNoError) {
                // EOF
                return nullptr;
            }
        }

        assert(len + offset <= chunk.size() - consumed);
        return std::memcpy(dst, chunk.data() + consumed + offset, len);
    }

    void advanceBytes(size_t len) {
        consumed += len;

        auto& flags = reinterpret_cast<uint32_t&>(parser.flags);
        flags &= ~0x800000;
    }

    CborError transferString(const void** userptr, size_t offset, size_t len) {
        if (total - consumed < offset + len) {
            if (const auto res = truncateAndReadMore(); res != CborNoError) {
                return res;
            }

            // Are we still out of range?
            if (len + offset > total) {
                return CborErrorDataTooLarge;
            }
        }

        *userptr = chunk.data() + consumed + offset;

        auto& flags = reinterpret_cast<uint32_t&>(parser.flags);
        if (flags & 0x800000) {
            consumed += len + offset;
            flags &= ~0x800000;
        }

        return CborNoError;
    }

    static constexpr CborParserOperations kOps = {
        .can_read_bytes =
            +[](void* token, const size_t len) {
                auto& self = *static_cast<CborChunkReader*>(token);
                return self.canReadBytes(len);
            },
        .read_bytes =
            +[](void* token, void* dst, const size_t offset, const size_t len) {
                auto& self = *static_cast<CborChunkReader*>(token);
                return self.readBytes(dst, offset, len);
            },
        .advance_bytes =
            +[](void* token, const size_t len) {
                auto& self = *static_cast<CborChunkReader*>(token);
                self.advanceBytes(len);
            },
        .transfer_string =
            +[](void* token, const void** userptr, size_t offset, size_t len) {
                auto& self = *static_cast<CborChunkReader*>(token);
                return self.transferString(userptr, offset, len);
            },
    };

    Reader& reader;
    CborParser parser;
    std::array<uint8_t, 16 * 1024> chunk;
    size_t consumed{0};
    size_t total{0};
};

Usage:

MyCustomFileReader reader{"data.cbor"};
CborValue value{};
CborChunkReader<MyCustomFileReader> parser{reader, value};

No dynamic memory allocations :)