From 3cdf43607c6fbdef1d943ff87da499879d63ee26 Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Fri, 1 Feb 2019 17:35:54 -0500 Subject: [PATCH 1/5] net/nanocoap: Return error from coap_opt_add_xxx() if no space --- sys/include/net/nanocoap.h | 9 +++++--- sys/net/application_layer/gcoap/gcoap.c | 22 +++++++------------ sys/net/application_layer/nanocoap/nanocoap.c | 15 ++++++++----- 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index c39e982d28..5772cb9b86 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -620,7 +620,8 @@ size_t coap_put_block1_ok(uint8_t *pkt_pos, coap_block1_t *block1, uint16_t last * @param[in] separator character used in @p string to separate parts * * @return number of bytes written to buffer - * @return -ENOSPC if no available options + * @return <0 on error + * @return -ENOSPC if no available options or insufficient buffer space */ ssize_t coap_opt_add_string(coap_pkt_t *pkt, uint16_t optnum, const char *string, char separator); @@ -635,7 +636,8 @@ ssize_t coap_opt_add_string(coap_pkt_t *pkt, uint16_t optnum, const char *string * @param[in] value uint to encode * * @return number of bytes written to buffer - * @return <0 reserved for error but not implemented yet + * @return <0 on error + * @return -ENOSPC if no available options or insufficient buffer space */ ssize_t coap_opt_add_uint(coap_pkt_t *pkt, uint16_t optnum, uint32_t value); @@ -649,7 +651,8 @@ ssize_t coap_opt_add_uint(coap_pkt_t *pkt, uint16_t optnum, uint32_t value); * @param[in] format COAP_FORMAT_xxx to use * * @return number of bytes written to buffer - * @return <0 reserved for error but not implemented yet + * @return <0 on error + * @return -ENOSPC if no available options or insufficient buffer space */ static inline ssize_t coap_opt_add_format(coap_pkt_t *pkt, uint16_t format) { diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c index c3fa012167..335c187583 100644 --- a/sys/net/application_layer/gcoap/gcoap.c +++ b/sys/net/application_layer/gcoap/gcoap.c @@ -653,25 +653,19 @@ int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len, (GCOAP_TOKENLEN - i >= 4) ? 4 : GCOAP_TOKENLEN - i); } uint16_t msgid = (uint16_t)atomic_fetch_add(&_coap_state.next_message_id, 1); - ssize_t hdrlen = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, &token[0], GCOAP_TOKENLEN, - code, msgid); + ssize_t res = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, &token[0], GCOAP_TOKENLEN, + code, msgid); #else uint16_t msgid = (uint16_t)atomic_fetch_add(&_coap_state.next_message_id, 1); - ssize_t hdrlen = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, NULL, GCOAP_TOKENLEN, - code, msgid); + ssize_t res = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, NULL, GCOAP_TOKENLEN, + code, msgid); #endif - if (hdrlen > 0) { - coap_pkt_init(pdu, buf, len - GCOAP_REQ_OPTIONS_BUF, hdrlen); - if (path != NULL) { - coap_opt_add_string(pdu, COAP_OPT_URI_PATH, path, '/'); - } - return 0; - } - else { - /* reason for negative hdrlen is not defined, so we also are vague */ - return -1; + coap_pkt_init(pdu, buf, len - GCOAP_REQ_OPTIONS_BUF, res); + if (path != NULL) { + res = coap_opt_add_string(pdu, COAP_OPT_URI_PATH, path, '/'); } + return (res > 0) ? 0 : res; } /* diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index b4a8167de6..d25ce6269e 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -716,7 +716,9 @@ size_t coap_opt_put_string(uint8_t *buf, uint16_t lastonum, uint16_t optnum, static ssize_t _add_opt_pkt(coap_pkt_t *pkt, uint16_t optnum, uint8_t *val, size_t val_len) { - assert(pkt->options_len < NANOCOAP_NOPTS_MAX); + if (pkt->options_len >= NANOCOAP_NOPTS_MAX) { + return -ENOSPC; + } uint16_t lastonum = (pkt->options_len) ? pkt->options[pkt->options_len - 1].opt_num : 0; @@ -727,7 +729,9 @@ static ssize_t _add_opt_pkt(coap_pkt_t *pkt, uint16_t optnum, uint8_t *val, size_t optlen = _put_delta_optlen(dummy, 1, 4, optnum - lastonum); optlen += _put_delta_optlen(dummy, 0, 0, val_len); optlen += val_len; - assert(pkt->payload_len >= optlen); + if (pkt->payload_len < optlen) { + return -ENOSPC; + } coap_put_option(pkt->payload, lastonum, optnum, val, val_len); @@ -772,10 +776,11 @@ ssize_t coap_opt_add_string(coap_pkt_t *pkt, uint16_t optnum, const char *string /* Creates empty option if part for Uri-Path or Uri-Location contains * only a trailing slash, except for root path ("/"). */ if (part_len || ((separator == '/') && write_len)) { - if (pkt->options_len == NANOCOAP_NOPTS_MAX) { - return -ENOSPC; + ssize_t optlen = _add_opt_pkt(pkt, optnum, part_start, part_len); + if (optlen < 0) { + return optlen; } - write_len += _add_opt_pkt(pkt, optnum, part_start, part_len); + write_len += optlen; } } From 5bf2fc622763284ff1a46b4a2cf0253051db88be Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Fri, 1 Feb 2019 19:19:52 -0500 Subject: [PATCH 2/5] net/nanocoap: return error from coap_opt_finish if no space --- sys/include/net/nanocoap.h | 1 + sys/net/application_layer/nanocoap/nanocoap.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index 5772cb9b86..90f506f0c6 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -669,6 +669,7 @@ static inline ssize_t coap_opt_add_format(coap_pkt_t *pkt, uint16_t format) * @param[in] flags see COAP_OPT_FINISH... macros * * @return total number of bytes written to buffer + * @return -ENOSPC if no buffer space for payload marker */ ssize_t coap_opt_finish(coap_pkt_t *pkt, uint16_t flags); diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index d25ce6269e..2b01ed6e9b 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -797,7 +797,9 @@ ssize_t coap_opt_add_uint(coap_pkt_t *pkt, uint16_t optnum, uint32_t value) ssize_t coap_opt_finish(coap_pkt_t *pkt, uint16_t flags) { if (flags & COAP_OPT_FINISH_PAYLOAD) { - assert(pkt->payload_len > 1); + if (!pkt->payload_len) { + return -ENOSPC; + } *pkt->payload++ = 0xFF; pkt->payload_len--; From 23e11f8a7d60ad472cd393e2abbcddca02c31808 Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Sat, 2 Feb 2019 06:03:18 -0500 Subject: [PATCH 3/5] net/nanocoap: clarify API buffer space doc --- sys/include/net/nanocoap.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index 90f506f0c6..5a9a8dfb5d 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -69,10 +69,13 @@ * * - **minimal API** requires only a reference to the buffer for the message. * However, the caller must provide the last option number written as well as - * the buffer position. + * the buffer position. The caller is primarily responsible for tracking and + * managing the space remaining in the buffer. * * - **struct-based API** uses a coap_pkt_t struct to conveniently track each - * option as it is written and prepare for any payload. + * option as it is written and prepare for any payload. The caller must monitor + * space remaining in the buffer; however, the API *will not* write past the + * end of the buffer, and returns -ENOSPC when it is full. * * You must use one API exclusively for a given message. For either API, the * caller must write options in order by option number (see "CoAP option @@ -89,6 +92,9 @@ * option. These functions require the position in the buffer to start writing, * and return the number of bytes written. * + * @note You must ensure the buffer has enough space remaining to write each + * option. The API does not verify the safety of writing an option. + * * If there is a payload, append a payload marker (0xFF). Then write the * payload to within the maximum length remaining in the buffer. * @@ -102,6 +108,10 @@ * coap_opt_add_uint(). When all options have been added, call * coap_opt_finish(). * + * @note You must ensure the buffer has enough space remaining to write each + * option. You can monitor `coap_pkt_t.payload_len` for remaining space, or + * watch for a -ENOSPC return value from the API. + * * Finally, write any message payload at the coap_pkt_t _payload_ pointer * attribute. The _payload_len_ attribute provides the available length in the * buffer. The option functions keep these values current as they are used. From 58faa3515647ca4b2f0742cc39c156f45b0bc14f Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Sat, 2 Feb 2019 06:37:14 -0500 Subject: [PATCH 4/5] tests/nanocoap: verify error when overfill buffer --- tests/unittests/tests-nanocoap/tests-nanocoap.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/unittests/tests-nanocoap/tests-nanocoap.c b/tests/unittests/tests-nanocoap/tests-nanocoap.c index b742fadd97..e6dab02936 100644 --- a/tests/unittests/tests-nanocoap/tests-nanocoap.c +++ b/tests/unittests/tests-nanocoap/tests-nanocoap.c @@ -332,7 +332,7 @@ static void test_nanocoap__get_multi_query(void) /* * Builds on get_req test, to test building a PDU that completely fills the - * buffer. + * buffer, and one that tries to overfill the buffer. */ static void test_nanocoap__option_add_buffer_max(void) { @@ -344,13 +344,19 @@ static void test_nanocoap__option_add_buffer_max(void) size_t uri_opt_len = 64; /* option hdr 2, option value 62 */ - size_t len = coap_build_hdr((coap_hdr_t *)&buf[0], COAP_TYPE_NON, - &token[0], 2, COAP_METHOD_GET, msgid); + ssize_t len = coap_build_hdr((coap_hdr_t *)&buf[0], COAP_TYPE_NON, + &token[0], 2, COAP_METHOD_GET, msgid); coap_pkt_init(&pkt, &buf[0], sizeof(buf), len); len = coap_opt_add_string(&pkt, COAP_OPT_URI_PATH, &path[0], '/'); TEST_ASSERT_EQUAL_INT(uri_opt_len, len); + + /* shrink buffer to attempt overfill */ + coap_pkt_init(&pkt, &buf[0], sizeof(buf) - 1, len); + + len = coap_opt_add_string(&pkt, COAP_OPT_URI_PATH, &path[0], '/'); + TEST_ASSERT_EQUAL_INT(-ENOSPC, len); } /* From 7d259cc2682607fe5352efea6477c750197996a6 Mon Sep 17 00:00:00 2001 From: Ken Bannister Date: Sat, 2 Feb 2019 07:45:18 -0500 Subject: [PATCH 5/5] net/gcoap: verify error when overfill on coap_opt_finish --- tests/unittests/tests-gcoap/tests-gcoap.c | 24 ++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/unittests/tests-gcoap/tests-gcoap.c b/tests/unittests/tests-gcoap/tests-gcoap.c index b67273ac5c..981186aecc 100644 --- a/tests/unittests/tests-gcoap/tests-gcoap.c +++ b/tests/unittests/tests-gcoap/tests-gcoap.c @@ -133,7 +133,7 @@ static void test_gcoap__client_get_resp(void) */ static void test_gcoap__client_put_req(void) { - uint8_t buf[GCOAP_PDU_BUF_SIZE]; + uint8_t buf[GCOAP_PDU_BUF_SIZE]; /* header 4, token 2, path 11 */ coap_pkt_t pdu; size_t len; char path[] = "/riot/value"; @@ -151,6 +151,27 @@ static void test_gcoap__client_put_req(void) TEST_ASSERT_EQUAL_INT('1', (char)*pdu.payload); } +/* + * Builds on client_put_req to test overfill on coap_opt_finish(). + */ +static void test_gcoap__client_put_req_overfill(void) +{ + /* header 4, token 2, path 11, format 1, marker 1 = 19 */ + uint8_t buf[18+GCOAP_REQ_OPTIONS_BUF]; + coap_pkt_t pdu; + ssize_t len; + char path[] = "/riot/value"; + + gcoap_req_init(&pdu, buf, sizeof(buf), COAP_METHOD_PUT, path); + TEST_ASSERT_EQUAL_INT(1, pdu.payload_len); + + coap_opt_add_format(&pdu, COAP_FORMAT_TEXT); + TEST_ASSERT_EQUAL_INT(0, pdu.payload_len); + + len = coap_opt_finish(&pdu, COAP_OPT_FINISH_PAYLOAD); + TEST_ASSERT_EQUAL_INT(-ENOSPC, len); +} + /* * Builds on get_req test, to test gcoap_add_qstring() to add Uri-Query * options. @@ -375,6 +396,7 @@ Test *tests_gcoap_tests(void) new_TestFixture(test_gcoap__client_get_req), new_TestFixture(test_gcoap__client_get_resp), new_TestFixture(test_gcoap__client_put_req), + new_TestFixture(test_gcoap__client_put_req_overfill), new_TestFixture(test_gcoap__client_get_query), new_TestFixture(test_gcoap__client_get_path_defer), new_TestFixture(test_gcoap__server_get_req),