[riot-notifications] [RIOT-OS/RIOT] nanocoap: fix server-side option_count overflow (#10754)

Ken Bannister notifications at github.com
Sat Jan 12 13:53:36 CET 2019


kb2ma requested changes on this pull request.

I agree the verification in coap_parse() is in the right place now. See my suggested alternative unit test in [kapar030/#41](https://github.com/kaspar030/RIOT/pull/41). We're getting closer...

> @@ -106,6 +106,11 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
             DEBUG("option count=%u nr=%u len=%i\n", option_count, option_nr, option_len);
 
             if (option_delta) {
+                if (option_count >= NANOCOAP_NOPTS_MAX) {
+                    DEBUG("nanocoap: max nr of options exceeded");

I agree debug statement is helpful. Needs newline.

> @@ -433,6 +434,40 @@ static void test_nanocoap__server_reply_simple_con(void)
     TEST_ASSERT_EQUAL_INT(COAP_TYPE_ACK, coap_get_type(&pkt));
 }
 
+static void test_nanocoap__server_option_count_overflow_check(void)

I kind of understand what you are doing to test the overflow. Why are there 42 options in guard data? How does the unit test work? I think it's important for others to understand this without a lot of thought/investigation.

-- 
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/10754#pullrequestreview-191937213
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190112/36823979/attachment.html>


More information about the notifications mailing list