[riot-notifications] [RIOT-OS/RIOT] Bugfix/nanocoap token overflow (#14075)

Ken Bannister notifications at github.com
Fri May 15 06:54:40 CEST 2020


@kb2ma requested changes on this pull request.

Thanks for the contribution, @mjurczak! If you address the inline comment, I'd be happy to approve this PR. If you want to make a couple of more additions, read on.

In #14074 you mention the maximum length of the token is 8 bytes, but there is no validation in the code here. You could add that in a separate commit. It would be best to define a constant in `coap.h`, like COAP_TOKEN_LENGTH_MAX.

A unit test in `tests-nanocoap.c` would be awesome but not essential. You could craft a bogus packet and try to parse it. There are some similar examples there.

> @@ -87,12 +87,17 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
         pkt->token = NULL;
     }
 
+    if (pkt_pos > pkt_end) {

Let's combine checks and save a few bytes. With your change to the while loop condition, we can move this check below the while loop, and remove the check currently at the end of the while loop. I'd change the debug message to something like "bad pkt len" to cover both cases. 

It also would be good to add a comment above, where testing the token length, that the validation is below the while loop.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/14075#pullrequestreview-412341933
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200514/f2cf88af/attachment.htm>


More information about the notifications mailing list