From d5662e207d3d5ce32931dd70517aa1d18fdc24d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20R=C3=B8nningstad?= Date: Wed, 5 Jun 2024 08:04:30 +0200 Subject: [PATCH] zcbor_encode.c: Fix a length bug in zcbor_bstr_end_decode() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bug caused CBOR-encoded bstrs to lose a byte. When calculating the maximum length of the bstr, there are certain payload lengths which cannot be filled by a bstr (incl. header and payload). One of these lengths is 258, since a 255 byte payload gets a 2-byte header (total 257) and a 256 byte payload gets a 3-byte header (total 259), i.e. there is no combination that gives 258 bytes. The same applies to total lengths of 25, 65539-65540, and 4294967300-4294967304 The previous code tried to find the start of the string by extrapolating from the end of the payload, but this gave the wrong answer for the cases described above. But also, the remaining_str_len was suboptimally implemented, so a few more cases were also wrong. Fix zcbor_bstr_end_encode(), remaining_str_len(), and add tests that test both functions Signed-off-by: Øyvind Rønningstad --- include/zcbor_common.h | 12 +++ src/zcbor_common.c | 16 ++++ src/zcbor_encode.c | 13 +--- tests/unit/test1_unit_tests/src/main.c | 101 +++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 11 deletions(-) diff --git a/include/zcbor_common.h b/include/zcbor_common.h index 3cd4e7b9..ccdcb97f 100644 --- a/include/zcbor_common.h +++ b/include/zcbor_common.h @@ -475,6 +475,18 @@ size_t zcbor_header_len(uint64_t value); /** Like @ref zcbor_header_len but for integer of any size <= 8. */ size_t zcbor_header_len_ptr(const void *const value, size_t value_len); +/** If a string (header + payload) is encoded into the rest of the payload, how long would it be? + * + * Note that a string with this length doesn't necessarily fill the rest of the + * payload. For some payload lengths, e.g. 25, it's impossible to encode a + * string of that total length. + * + * @param[in] state The current state. + * + * @return The length of the string (payload, not including header) in bytes. + */ +size_t zcbor_remaining_str_len(zcbor_state_t *state); + /** Convert a float16 value to float32. * * @param[in] input The float16 value stored in a uint16_t. diff --git a/src/zcbor_common.c b/src/zcbor_common.c index cd89db41..5bf959e6 100644 --- a/src/zcbor_common.c +++ b/src/zcbor_common.c @@ -283,6 +283,22 @@ size_t zcbor_header_len_ptr(const void *const value, size_t value_len) } +size_t zcbor_remaining_str_len(zcbor_state_t *state) +{ + size_t max_len = (size_t)state->payload_end - (size_t)state->payload; + + if (max_len == 0) { + return 0; + } + + size_t max_header_len = zcbor_header_len(max_len); + size_t min_header_len = zcbor_header_len(max_len - max_header_len); + size_t header_len = zcbor_header_len(max_len - min_header_len); + + return max_len - header_len; +} + + int zcbor_entry_function(const uint8_t *payload, size_t payload_len, void *result, size_t *payload_len_out, zcbor_state_t *state, zcbor_decoder_t func, size_t n_states, size_t elem_count) diff --git a/src/zcbor_encode.c b/src/zcbor_encode.c index 584dad97..18207f56 100644 --- a/src/zcbor_encode.c +++ b/src/zcbor_encode.c @@ -224,22 +224,13 @@ static bool str_start_encode(zcbor_state_t *state, } -static size_t remaining_str_len(zcbor_state_t *state) -{ - size_t max_len = (size_t)state->payload_end - (size_t)state->payload; - size_t result_len = zcbor_header_len_ptr(&max_len, sizeof(max_len)) - 1; - - return max_len - result_len - 1; -} - - bool zcbor_bstr_start_encode(zcbor_state_t *state) { if (!zcbor_new_backup(state, 0)) { ZCBOR_FAIL(); } - uint64_t max_len = remaining_str_len(state); + uint64_t max_len = zcbor_remaining_str_len(state); /* Encode a dummy header */ if (!value_encode(state, ZCBOR_MAJOR_TYPE_BSTR, &max_len, sizeof(max_len))) { @@ -265,7 +256,7 @@ bool zcbor_bstr_end_encode(zcbor_state_t *state, struct zcbor_string *result) ZCBOR_FAIL(); } - result->value = state->payload_end - remaining_str_len(state); + result->value = state->payload + zcbor_header_len(zcbor_remaining_str_len(state)); result->len = (size_t)payload - (size_t)result->value; /* Reencode header of list now that we know the number of elements. */ diff --git a/tests/unit/test1_unit_tests/src/main.c b/tests/unit/test1_unit_tests/src/main.c index 0dcb0ed4..c1b16425 100644 --- a/tests/unit/test1_unit_tests/src/main.c +++ b/tests/unit/test1_unit_tests/src/main.c @@ -1440,4 +1440,105 @@ ZTEST(zcbor_unit_tests, test_zcbor_version) } +/* Test that CBOR-encoded bstrs are encoded with the correct length. */ +ZTEST(zcbor_unit_tests, test_cbor_encoded_bstr_len) +{ + uint8_t payload[50]; + +#ifdef ZCBOR_VERBOSE + for (size_t len = 10; len < 0x108; len++) +#else + for (size_t len = 10; len < 0x10010; len++) +#endif /* ZCBOR_VERBOSE */ + { + ZCBOR_STATE_E(state_e, 1, payload, len, 0); + ZCBOR_STATE_D(state_d, 1, payload, len, 1, 0); + + zassert_true(zcbor_bstr_start_encode(state_e), "len: %d\n", len); + zassert_true(zcbor_size_put(state_e, len), "len: %d\n", len); + zassert_true(zcbor_bstr_end_encode(state_e, NULL), "len: %d\n", len); + + zassert_true(zcbor_bstr_start_decode(state_d, NULL), "len: %d\n", len); + zassert_true(zcbor_size_expect(state_d, len), "len: %d\n", len); + zassert_true(zcbor_bstr_end_decode(state_d), "len: %d\n", len); + } + +#if SIZE_MAX == UINT64_MAX + for (size_t len = 0xFFFFFF00; len <= 0x100000100; len++) { + ZCBOR_STATE_E(state_e, 1, payload, len, 0); + ZCBOR_STATE_D(state_d, 1, payload, len, 1, 0); + + zassert_true(zcbor_bstr_start_encode(state_e), "len: %d\n", len); + zassert_true(zcbor_size_put(state_e, len), "len: %d\n", len); + zassert_true(zcbor_bstr_end_encode(state_e, NULL), "len: %d\n", len); + + zassert_true(zcbor_bstr_start_decode(state_d, NULL), "len: %d\n", len); + zassert_true(zcbor_size_expect(state_d, len), "len: %d\n", len); + zassert_true(zcbor_bstr_end_decode(state_d), "len: %d\n", len); + } +#endif /* SIZE_MAX == UINT64_MAX */ +} + + +/* Test zcbor_remaining_str_len(). + * Some payload lengths are impossible to fill with a properly encoded string, + * these have special cases. */ +ZTEST(zcbor_unit_tests, test_remaining_str_len) +{ + uint8_t payload[8]; + ZCBOR_STATE_E(state_e, 1, payload, 0, 0); + + zassert_equal(zcbor_remaining_str_len(state_e), 0, "i: 0\n"); + + for (uint64_t i = 1; i <= 0x20000; i++) { + size_t offset; + + state_e->payload_end = payload + i; + + switch(i) { + case 25: + case 0x102: + offset = 1; + break; + case 0x10003: + case 0x10004: + offset = 2; + break; + default: + offset = 0; + break; + } + + size_t total_len = zcbor_remaining_str_len(state_e) + + zcbor_header_len(zcbor_remaining_str_len(state_e)); + zassert_equal(i - offset, total_len, "i: %ld, len: %zu\n", i, total_len); + } + +#if SIZE_MAX == UINT64_MAX + for (uint64_t i = 0xFFFFFF00; i <= 0x100000100; i++) { + size_t offset; + + state_e->payload_end = payload + i; + + switch(i) { + case 0x100000005: + case 0x100000006: + case 0x100000007: + case 0x100000008: + offset = 4; + break; + default: + offset = 0; + break; + } + + size_t total_len = zcbor_remaining_str_len(state_e) + + zcbor_header_len(zcbor_remaining_str_len(state_e)); + zassert_equal(i - offset, total_len, "i: %lx, len: %zx\n", i, total_len); + } +#endif +} + + + ZTEST_SUITE(zcbor_unit_tests, NULL, NULL, NULL, NULL, NULL);