Merge pull request #10931 from kb2ma/nanocoap/pktapi_opt_enospc

net/nanocoap: Packet API return error if buffer full

sys/include/net/nanocoap.h

@@ -88,10 +88,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
@@ -108,6 +111,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.
  *
@@ -121,6 +127,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.
@@ -641,7 +651,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);
 
@@ -656,7 +667,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);
 
@@ -670,7 +682,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)
 {
@@ -687,6 +700,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);
 

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;
 }
 
 /*

sys/net/application_layer/nanocoap/nanocoap.c

@@ -731,7 +731,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;
@@ -742,7 +744,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);
 
@@ -787,10 +791,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;
         }
     }
 
@@ -807,7 +812,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--;

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),

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);
 }
 
 /*