Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertions with cbor_assert(cond) #166

Open
TSonono opened this issue Oct 9, 2019 · 6 comments
Open

Assertions with cbor_assert(cond) #166

TSonono opened this issue Oct 9, 2019 · 6 comments

Comments

@TSonono
Copy link

TSonono commented Oct 9, 2019

Hi,

I was wondering if there is a particular reason that the macro cbor_assert(cond) defined on line 64 in compilersupport_p.h is used instead of using a macro such as:

#  define MACRO do { if (!(cond)) return SomeCborError; } while (0)

The drawback of having an assertion from my point-of-view as someone who uses the library, is the fact that when the assertion fails, the program crashes. This is not ideal when I am fuzz testing my program since it will prevent me from knowing if it is my program that is causing the crash or the assertion from tinycbor.

@thiagomacieira
Copy link
Member

The way TinyCBOR was designed, the idea was that you shouldn't pay for extra checking of state that you had already checked. So I don't want to return an error condition: you should check your inputs before calling the API. Calling for example cbor_value_advance_fixed on a non-fixed-size type is API abuse and your code is at fault.

So crashing is the right consequence. You get a core dump and you can fix your code.

I could accept a patch to add a third mode where it does return. Actually, we may want to separate internal self-checks from external state validation.

@TSonono
Copy link
Author

TSonono commented Oct 10, 2019

Ok, I see. It is even stated in your documentation that error checks should be done before calling the API. Sorry about that.

Hopefully can I rephrase my question? In the scenario below, am I doing my error check wrong here? Because in VERIFY_CBOR(cbor_value_map_find_value(&p, "c", &result) == CborNoError); I am getting an assertion error (thus a crash) from cbor_value_map_find_value when using the buffer defined below (starts with a map). This is an erroneous message and I would ideally in my program be able to return something rather than have it crash due to an assertion. Am I missing an error check which I should do before calling cbor_value_map_find_value in this case?

#define VERIFY_CBOR(x) do { if (!(x)) return 0; } while(0)

uint8_t buffer [128] = {0xbf,0x64,0x0a,0xe0,0xb5,0xbf,0x04,0x00,0xff,0x1e,0xff,0xff,0x80,0xbf,0xa4,0xbf,0xff,0x14,0x00,0x14,0x00,0xbf,0xbf,0xbf,0xe4,0xbf,0xbf,0x43,0xbf,0xbf,0xa0,0x13,0xaa,0xaa,0xaa,0xaa,0x70,0xaa,0xaa,0xaa,0xaa,0xaa,0xea,0xaa,0xae,0x05,0x11,0x62,0x78,0x7c,0x05,0x03,0x18,0x00,0x10,0x40,0x20,0x80,0x61,0x00,0xd8,0x00,0x00,0x00,0xde,0x00,0x00,0xcf,0x35,0x10,0x1e,0x00,0x00,0x00,0x00,0x00,0x80,0x00,0xaa,0xff,0xaa,0xaa,0x00,0x01,0x07,0x00,0xaa,0xaa,0xac,0x05,0x03,0x1a,0x82,0xa4,0x40,0x0d,0x80,0x35,0x78,0xac,0x05,0x00,0x00,0x00,0x7f,0xff,0xff,0xff,0xa8,0xf2,0x61,0x00,0xd8,0x00,0x00,0x00,0xcf,0x62,0x78,0x78,0xac,0x05,0x00,0xbd,0x11,0xb6,0x03,0x66};

size_t len;
CborParser parser;
CborValue  it;
CborValue result;

VERIFY_CBOR(cbor_parser_init(buffer, sizeof(buffer), 0, &parser, &it) == CborNoError);
VERIFY_CBOR(cbor_value_is_map(&it));

VERIFY_CBOR(cbor_value_map_find_value(&it, "c", &result) == CborNoError);
VERIFY_CBOR(result.type != CborInvalidType);      //Ensure "c" exists
VERIFY_CBOR(cbor_value_is_text_string(&result));  //Ensure c's value is a text string
VERIFY_CBOR(cbor_value_get_string_length(&result, &len)  == CborNoError);

@thiagomacieira
Copy link
Member

Decoding your stream using the Qt example wrapper, I'm getting:

$ xxd -ps -r | ./cbordump --annotated
bf                                                 # Map (indeterminate length)
  64                                               # Text string length 4
    0a e0 b5 bf                                    # "...."
  04                                               # Unsigned integer 4
  00                                               # Unsigned integer 0
cbordump: decoding failed at 8: Invalid CBOR stream: unexpected 'break' byte
 bytes at 8: ff 1e ff ff 80 bf a4 bf ff

Your stream is corrupt (the tool is right, I manually verified your bytes). Was that intended?

I don't think I have a test of cbor_value_map_find_value against invalid streams, but looking at the code shows it should not crash or make mistakes. It compared the first string of "\n\xe0\xb5\xbf", found that it was not the right string, then skipped over the value (integer 4). Then it got to the next key (integer 0), and tried to skip over it. When it did, the next element failed to parse and returned CborErrorUnexpectedBreak.

@TSonono
Copy link
Author

TSonono commented Oct 11, 2019

Sorry, I should have been clearer in my previous post. Indeed, the stream is intentionally corrupt.

Your result was what I was hoping for. If I remember correctly, when I ran this program, it crashed due to an assertion in ’cbor_value_map_find_value’.

I will check again when I have time. Thanks for taking the time to reply.

@thiagomacieira
Copy link
Member

I mentally ran this. The decoder whose output we see above is not using cbor_value_map_find_value. I'll add this to the unit test and see if there's a bug that needs fixing.

@thiagomacieira
Copy link
Member

Created #167 to track that particular issue.

We still have value in this task: separate the macro used for the internal state checker assertions from the API verification checkers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants