bergzand / nanocbor Goto Github PK

The docstring on nanocbor_put_bstr states that the return value is the number of bytes written.

If you look at the source code the return value depends on the call to _fits which either returns 0 or -1. The docstring should probably be updated to something like:

• @return NANOCBOR_OK on success
• @return negative on error

Should be fixed by properly declaring the constants

"make" -C /tmp/dwq.0.7453450615585222/ad9984d6052f52acdf6a81056f4fa44d/build/pkg/nanocbor/src \
	-f /tmp/dwq.0.7453450615585222/ad9984d6052f52acdf6a81056f4fa44d/pkg/nanocbor/Makefile.nanocbor
encoder.c: In function '_single_is_zero':
encoder.c:210:34: error: left shift count >= width of type [-Werror=shift-count-overflow]
 #define FLOAT_SIGN_MASK      (1U << FLOAT_SIGN_POS)
                                  ^
encoder.c:212:35: note: in expansion of macro 'FLOAT_SIGN_MASK'
 #define FLOAT_IS_ZERO          (~(FLOAT_SIGN_MASK))
                                   ^
encoder.c:234:19: note: in expansion of macro 'FLOAT_IS_ZERO'
     return (num & FLOAT_IS_ZERO) == 0;
                   ^
encoder.c: In function 'nanocbor_fmt_double':
encoder.c:210:34: error: left shift count >= width of type [-Werror=shift-count-overflow]
 #define FLOAT_SIGN_MASK      (1U << FLOAT_SIGN_POS)
                                  ^
encoder.c:326:68: note: in expansion of macro 'FLOAT_SIGN_MASK'
         uint32_t single = (*unum >> (DOUBLE_SIZE - FLOAT_SIZE)) & (FLOAT_SIGN_MASK);
                                                                    ^
encoder.c:331:43: error: left shift count >= width of type [-Werror=shift-count-overflow]
         single |= ((exp & FLOAT_EXP_MASK) << FLOAT_EXP_POS) |
                                           ^
cc1: all warnings being treated as errors

The decoder given in the test/fuzz isn't able to decode an indefinite array encoded as: nanocbor_fmt_array_indefinite(enc); nanocbor_fmt_int(enc, 8); nanocbor_fmt_end_indefinite(enc);

According to its documentation it should:

"skip over nested structures in the CBOR stream such as (nested) arrays and maps"

However it doesn't seem to work. A simple test case such as the following fails:

nanocbor_value_t decoder;
uint8_t byteval = 0xA0; /* empty map, size 0 */

nanocbor_decoder_init(&decoder, &byteval, sizeof(byteval));
nanocbor_skip(&decoder);

CU_ASSERT_EQUAL(&byteval + 1, decoder.cur);

Investigating this I found something suspicious in _skip_limited. The part that recursively skips the content of the map/array is inside a conditional branch which is never entered. It checks if the return code from nanocbor_enter_map/array is > 0, which it never is, since those function either returns something < 0 or 0. Changing that expression to == 0, the test case works.

diff --git a/src/decoder.c b/src/decoder.c
index 6b69405..51a07cc 100644
--- a/src/decoder.c
+++ b/src/decoder.c
@@ -365,7 +365,7 @@ static int _skip_limited(nanocbor_value_t *it, uint8_t limit)
         res = (type == NANOCBOR_TYPE_MAP
                ? nanocbor_enter_map(it, &recurse)
                : nanocbor_enter_array(it, &recurse));
-        if (res > 0) {
+        if (res == 0) {
             while (!nanocbor_at_end(&recurse)) {
                 res = _skip_limited(&recurse, limit - 1);
                 if (res < 0) {

Is this actually an issue or have I misunderstood something about skip on nested structures?

Code for reproducing

#include<stdio.h>
#include<nanocbor/nanocbor.h>

int main(void) {
    char encode_buf[1024];
    nanocbor_encoder_t cbor_encoder;
    nanocbor_encoder_init(&cbor_encoder, encode_buf, 1024);

    nanocbor_fmt_map(&cbor_encoder, 2);
    nanocbor_put_tstr(&cbor_encoder, "key1");
    nanocbor_put_tstr(&cbor_encoder, "value1");
    nanocbor_put_tstr(&cbor_encoder, "key2");
    nanocbor_put_tstr(&cbor_encoder, "value2");

    nanocbor_value_t cbor_decoder;
    nanocbor_value_t cbor_encoded_val;
    nanocbor_value_t parsed_map;
    nanocbor_decoder_init(&cbor_decoder, encode_buf, nanocbor_encoded_len(&cbor_encoder));

    if(nanocbor_enter_map(&cbor_decoder, &parsed_map) == NANOCBOR_OK 
            && nanocbor_get_key_tstr(&parsed_map, "key3", &cbor_encoded_val) == NANOCBOR_OK) {
        printf("key3 found\n");
    } else {
        printf("key3 not found\n");
    }
}

This prints "key3 found". I haven't been able to figure out why this is happening.

It seems that there is a lot of duplicate code between:

% cd tests; diff automated/test_decoder.c encode/main.c

Is there a reason not to just merge it all into automated?

Only floats seem to be supported currently. A function for doubles (nanocbor_fmt_double?) would be greatly appreciated!

The documentation states

* @return NANOCBOR_OK on success

NANOCBOR_OK is defined to be 0.

However, nanocbor_enter_array() returns the result of _enter_container() which return 1 on success.

Checking the return value of nanocbor_enter_array() to match NANOCBOR_OK will therefore always fail.

The separation of buffer (with its end) and length already allows having nanoCBOR encoded into the first block of a CoAP block-wise response. Would it be reasonable to extend the encoder by a field that counts how many initially encoded bytes to discard? (Similar to how the produced length tells how many bytes were discarded after the buffer).

I don't have a full use case yet, but looked at @silkeh's RIOT-OS/RIOT#17571, and that would appear to work great with CoAP block-wise. So this is not a full feature request yet, more like a "would it make sense to add this feature?"

(And it may happen that the next request is then for a checksum field that all the input is fed into, which would then generate ETags to detect whether the blocks can even be assembled).

When using NanoCBOR with the waspmote-pro board (in RIOT), I get the following error:

$ BOARD=waspmote-pro make -C tests/pkg_nanocbor
make: Entering directory '/home/jdavid/sandboxes/UiO/RIOT/tests/pkg_nanocbor'
Building application "tests_pkg_nanocbor" for "waspmote-pro" with MCU "atmega1281".

"make" -C /home/jdavid/sandboxes/UiO/RIOT/pkg/nanocbor/ 
"make" -C /home/jdavid/sandboxes/UiO/RIOT/build/pkg/nanocbor/src -f /home/jdavid/sandboxes/UiO/RIOT/Makefile.base MODULE=nanocbor
/home/jdavid/sandboxes/UiO/RIOT/build/pkg/nanocbor/src/encoder.c: In function ‘nanocbor_fmt_double’:
/home/jdavid/sandboxes/UiO/RIOT/build/pkg/nanocbor/src/encoder.c:324:21: error: array subscript ‘uint64_t {aka long long unsigned int}[0]’ is partly outside array bounds of ‘double[1]’ [-Werror=array-bounds]
  324 |     uint16_t exp = (*unum >> DOUBLE_EXP_POS) & DOUBLE_EXP_MASK;
      |                     ^~~~~
/home/jdavid/sandboxes/UiO/RIOT/build/pkg/nanocbor/src/encoder.c:321:57: note: while referencing ‘num’
  321 | int nanocbor_fmt_double(nanocbor_encoder_t *enc, double num)
      |                                                  ~~~~~~~^~~
cc1: all warnings being treated as errors
make[2]: *** [/home/jdavid/sandboxes/UiO/RIOT/Makefile.base:146: /home/jdavid/sandboxes/UiO/RIOT/tests/pkg_nanocbor/bin/waspmote-pro/nanocbor/encoder.o] Error 1
make[1]: *** [Makefile:9: all] Error 2
make: *** [/home/jdavid/sandboxes/UiO/RIOT/tests/pkg_nanocbor/../../Makefile.include:802: pkg-build] Error 2
make: Leaving directory '/home/jdavid/sandboxes/UiO/RIOT/tests/pkg_nanocbor'

This may be related to issue #18