From 5fd2f89ef068214130e5d60b7087ef48711fa615 Mon Sep 17 00:00:00 2001 From: Jon Shallow Date: Sat, 30 Sep 2023 13:19:54 +0100 Subject: [PATCH] pdu.c: Fix issue with 269 delta in internal coap_remove_option() Update unit tests to check for a delta of 13 / 269. --- src/coap_pdu.c | 12 +-- tests/test_pdu.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 191 insertions(+), 10 deletions(-) diff --git a/src/coap_pdu.c b/src/coap_pdu.c index c3f68a3350..afe445c802 100644 --- a/src/coap_pdu.c +++ b/src/coap_pdu.c @@ -453,18 +453,18 @@ coap_remove_option(coap_pdu_t *pdu, coap_option_num_t number) { &decode_next)) return 0; opt_delta = decode_this.delta + decode_next.delta; - if (opt_delta <= 12) { + if (opt_delta < 13) { /* can simply update the delta of next option */ next_option[0] = (next_option[0] & 0x0f) + (coap_opt_t)(opt_delta << 4); - } else if (opt_delta <= 269 && decode_next.delta <= 12) { + } else if (opt_delta < 269 && decode_next.delta < 13) { /* next option delta size increase */ next_option -= 1; next_option[0] = (next_option[1] & 0x0f) + (13 << 4); next_option[1] = (coap_opt_t)(opt_delta - 13); - } else if (opt_delta <= 269) { + } else if (opt_delta < 269) { /* can simply update the delta of next option */ next_option[1] = (coap_opt_t)(opt_delta - 13); - } else if (decode_next.delta <= 12) { + } else if (decode_next.delta < 13) { /* opt_delta >= 269 */ /* next option delta size increase */ if (next_option - option < 2) { /* Need to shuffle everything up by 1 before decrement */ @@ -493,13 +493,13 @@ coap_remove_option(coap_pdu_t *pdu, coap_option_num_t number) { next_option[0] = (next_option[2] & 0x0f) + (14 << 4); next_option[1] = (coap_opt_t)((opt_delta - 269) >> 8); next_option[2] = (opt_delta - 269) & 0xff; - } else if (decode_next.delta <= 269) { + } else if (decode_next.delta < 269) { /* opt_delta >= 269 */ /* next option delta size increase */ next_option -= 1; next_option[0] = (next_option[1] & 0x0f) + (14 << 4); next_option[1] = (coap_opt_t)((opt_delta - 269) >> 8); next_option[2] = (opt_delta - 269) & 0xff; - } else { + } else { /* decode_next.delta >= 269 && opt_delta >= 269 */ next_option[1] = (coap_opt_t)((opt_delta - 269) >> 8); next_option[2] = (opt_delta - 269) & 0xff; } diff --git a/tests/test_pdu.c b/tests/test_pdu.c index 70753c2032..d9246655c9 100644 --- a/tests/test_pdu.c +++ b/tests/test_pdu.c @@ -1067,6 +1067,11 @@ t_encode_pdu18(void) { } /* Remove an option (no data) */ +/* + * Next Delta New Delta + * 4 18 + * 18 25 + */ static void t_encode_pdu19(void) { size_t n; @@ -1120,9 +1125,94 @@ t_encode_pdu19(void) { CU_ASSERT(memcmp(pdu->token, data6, pdu->used_size) == 0); } -/* Remove an option (with data) */ +/* Remove an option (no data), but exercise delta boundaries (13 and 269) */ +/* + * Next Delta New Delta + * 11 12 if (opt_delta < 13) { + * 1 13 } else if (opt_delta < 269 && decode_next.delta < 13) { + * 254 268 } else if (opt_delta < 269) { + * 1 269 } else if (decode_next.delta < 13) { // opt_delta >= 269 + * 71 350 } else if (decode_next.delta < 269) { // opt_delta >= 269 + * 320 670 } else { // decode_next.delta >= 269 && opt_delta >= 269 + */ static void t_encode_pdu20(void) { + size_t n; + uint8_t token[] = { 't' }; + uint16_t opt_num[] = { 1, 12, 13, 268, 269, 350, 670 }; + uint8_t opt_val[] = { 50, 51, 52, 53, 54, 55, 56 }; + uint8_t data1[] = { 0x74, 0x11, 0x32, 0xb1, 0x33, 0x11, 0x34, 0xd1, + 0xf2, 0x35, 0x11, 0x36, 0xd1, 0x44, 0x37, 0xe1, + 0x00, 0x33, 0x38 + }; + uint8_t data2[] = { 0x74, 0xc1, 0x33, 0x11, 0x34, 0xd1, 0xf2, 0x35, + 0x11, 0x36, 0xd1, 0x44, 0x37, 0xe1, 0x00, 0x33, + 0x38 + }; + uint8_t data3[] = { 0x74, 0xd1, 0x00, 0x34, 0xd1, 0xf2, 0x35, 0x11, + 0x36, 0xd1, 0x44, 0x37, 0xe1, 0x00, 0x33, 0x38 + }; + uint8_t data4[] = { 0x74, 0xd1, 0xff, 0x35, 0x11, 0x36, 0xd1, 0x44, + 0x37, 0xe1, 0x00, 0x33, 0x38 + }; + uint8_t data5[] = { 0x74, 0xe1, 0x00, 0x00, 0x36, 0xd1, 0x44, 0x37, + 0xe1, 0x00, 0x33, 0x38 + }; + uint8_t data6[] = { 0x74, 0xe1, 0x00, 0x51, 0x37, 0xe1, 0x00, 0x33, + 0x38 + }; + uint8_t data7[] = { 0x74, 0xe1, 0x01, 0x91, 0x38 }; + uint8_t data8[] = { 0x74 }; + + coap_pdu_clear(pdu, pdu->max_size); /* clear PDU */ + + CU_ASSERT(pdu->data == NULL); + + coap_add_token(pdu, sizeof(token), token); + /* Put the options in in reverse order to test that logic */ + for (n = sizeof(opt_num)/sizeof(opt_num[0]); n > 0; n--) { + coap_insert_option(pdu, opt_num[n-1], + sizeof(opt_val[n-1]), &opt_val[n-1]); + } + CU_ASSERT(memcmp(pdu->token, data1, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 1); + CU_ASSERT(memcmp(pdu->token, data2, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 12); + CU_ASSERT(memcmp(pdu->token, data3, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 13); + CU_ASSERT(memcmp(pdu->token, data4, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 268); + CU_ASSERT(memcmp(pdu->token, data5, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 269); + CU_ASSERT(memcmp(pdu->token, data6, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 350); + CU_ASSERT(memcmp(pdu->token, data7, pdu->used_size) == 0); + + /* Now remove the final option */ + coap_remove_option(pdu, 670); + CU_ASSERT(memcmp(pdu->token, data8, pdu->used_size) == 0); +} + +/* Remove an option (with data) */ +/* + * Next Delta New Delta + * 4 18 + * 18 25 + */ +static void +t_encode_pdu21(void) { size_t n; uint8_t token[] = { 't' }; uint16_t opt_num[] = { 300, 7, 21, 25 }; @@ -1183,9 +1273,99 @@ t_encode_pdu20(void) { CU_ASSERT(memcmp(pdu->token, data6, pdu->used_size) == 0); } +/* Remove an option (with data), but exercise delta boundaries (13 and 269) */ +/* + * Next Delta New Delta + * 11 12 if (opt_delta < 13) { + * 1 13 } else if (opt_delta < 269 && decode_next.delta < 13) { + * 254 268 } else if (opt_delta < 269) { + * 1 269 } else if (decode_next.delta < 13) { // opt_delta >= 269 + * 71 350 } else if (decode_next.delta < 269) { // opt_delta >= 269 + * 320 670 } else { // decode_next.delta >= 269 && opt_delta >= 269 + */ +static void +t_encode_pdu22(void) { + size_t n; + uint8_t token[] = { 't' }; + uint16_t opt_num[] = { 1, 12, 13, 268, 269, 350, 670 }; + uint8_t opt_val[] = { 50, 51, 52, 53, 54, 55, 56 }; + uint8_t data[] = { 'd', 'a', 't', 'a' }; + uint8_t data1[] = { 0x74, 0x11, 0x32, 0xb1, 0x33, 0x11, 0x34, 0xd1, + 0xf2, 0x35, 0x11, 0x36, 0xd1, 0x44, 0x37, 0xe1, + 0x00, 0x33, 0x38, 0xff, 0x64, 0x61, 0x74, 0x61 + }; + uint8_t data2[] = { 0x74, 0xc1, 0x33, 0x11, 0x34, 0xd1, 0xf2, 0x35, + 0x11, 0x36, 0xd1, 0x44, 0x37, 0xe1, 0x00, 0x33, + 0x38, 0xff, 0x64, 0x61, 0x74, 0x61 + }; + uint8_t data3[] = { 0x74, 0xd1, 0x00, 0x34, 0xd1, 0xf2, 0x35, 0x11, + 0x36, 0xd1, 0x44, 0x37, 0xe1, 0x00, 0x33, 0x38, + 0xff, 0x64, 0x61, 0x74, 0x61 + }; + uint8_t data4[] = { 0x74, 0xd1, 0xff, 0x35, 0x11, 0x36, 0xd1, 0x44, + 0x37, 0xe1, 0x00, 0x33, 0x38, 0xff, 0x64, 0x61, + 0x74, 0x61 + }; + uint8_t data5[] = { 0x74, 0xe1, 0x00, 0x00, 0x36, 0xd1, 0x44, 0x37, + 0xe1, 0x00, 0x33, 0x38, 0xff, 0x64, 0x61, 0x74, + 0x61 + }; + uint8_t data6[] = { 0x74, 0xe1, 0x00, 0x51, 0x37, 0xe1, 0x00, 0x33, + 0x38, 0xff, 0x64, 0x61, 0x74, 0x61 + }; + uint8_t data7[] = { 0x74, 0xe1, 0x01, 0x91, 0x38, 0xff, 0x64, 0x61, + 0x74, 0x61 + }; + uint8_t data8[] = { 0x74, 0xff, 0x64, 0x61, 0x74, 0x61 }; + + coap_pdu_clear(pdu, pdu->max_size); /* clear PDU */ + + CU_ASSERT(pdu->data == NULL); + + coap_add_token(pdu, sizeof(token), token); + + coap_add_data(pdu, sizeof(data), data); + + /* Put the options in in reverse order to test that logic */ + for (n = sizeof(opt_num)/sizeof(opt_num[0]); n > 0; n--) { + coap_insert_option(pdu, opt_num[n-1], + sizeof(opt_val[n-1]), &opt_val[n-1]); + } + CU_ASSERT(memcmp(pdu->token, data1, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 1); + CU_ASSERT(memcmp(pdu->token, data2, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 12); + CU_ASSERT(memcmp(pdu->token, data3, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 13); + CU_ASSERT(memcmp(pdu->token, data4, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 268); + CU_ASSERT(memcmp(pdu->token, data5, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 269); + CU_ASSERT(memcmp(pdu->token, data6, pdu->used_size) == 0); + + /* Now remove an option from the start */ + coap_remove_option(pdu, 350); + CU_ASSERT(memcmp(pdu->token, data7, pdu->used_size) == 0); + + /* Now remove the final option */ + coap_remove_option(pdu, 670); + CU_ASSERT(memcmp(pdu->token, data8, pdu->used_size) == 0); +} + + /* Update token */ static void -t_encode_pdu21(void) { +t_encode_pdu23(void) { size_t n; uint8_t token[] = { 't' }; uint8_t new_token[] = { 't', 'o', 'k', 'e', 'n' }; @@ -1232,7 +1412,7 @@ t_encode_pdu21(void) { /* insert option before (large) final one */ static void -t_encode_pdu22(void) { +t_encode_pdu24(void) { size_t n; uint8_t token[] = { 't' }; uint8_t buf[4]; @@ -1277,7 +1457,6 @@ t_encode_pdu22(void) { } } - static int t_pdu_tests_create(void) { pdu = coap_pdu_init(0, 0, 0, COAP_DEFAULT_MTU); @@ -1356,6 +1535,8 @@ t_init_pdu_tests(void) { PDU_ENCODER_TEST(suite[1], t_encode_pdu20); PDU_ENCODER_TEST(suite[1], t_encode_pdu21); PDU_ENCODER_TEST(suite[1], t_encode_pdu22); + PDU_ENCODER_TEST(suite[1], t_encode_pdu23); + PDU_ENCODER_TEST(suite[1], t_encode_pdu24); } else /* signal error */ fprintf(stderr, "W: cannot add pdu parser test suite (%s)\n",