Merge pull request #7121 from kb2ma/gcoap/thread_safety

net/gcoap: Protect internal state for multithreaded use

sys/include/net/gcoap.h

@@ -209,7 +209,10 @@
 #ifndef NET_GCOAP_H
 #define NET_GCOAP_H
 
+#include <stdint.h>
+#include <stdatomic.h>
 #include "net/sock/udp.h"
+#include "mutex.h"
 #include "nanocoap.h"
 #include "xtimer.h"
 
@@ -437,12 +440,13 @@ typedef struct {
  * @brief   Container for the state of gcoap itself
  */
 typedef struct {
+    mutex_t lock;                       /**< Shares state attributes safely */
     gcoap_listener_t *listeners;        /**< List of registered listeners */
     gcoap_request_memo_t open_reqs[GCOAP_REQ_WAITING_MAX];
                                         /**< Storage for open requests; if first
                                              byte of an entry is zero, the entry
                                              is available */
-    uint16_t last_message_id;           /**< Last message ID used */
+    atomic_uint next_message_id;        /**< Next message ID to use */
     sock_udp_ep_t observers[GCOAP_OBS_CLIENTS_MAX];
                                         /**< Observe clients; allows reuse for
                                              observe memos */

sys/net/application_layer/coap/gcoap.c

@@ -585,12 +585,13 @@ kernel_pid_t gcoap_init(void)
     _pid = thread_create(_msg_stack, sizeof(_msg_stack), THREAD_PRIORITY_MAIN - 1,
                             THREAD_CREATE_STACKTEST, _event_loop, NULL, "coap");
 
+    mutex_init(&_coap_state.lock);
     /* Blank lists so we know if an entry is available. */
     memset(&_coap_state.open_reqs[0], 0, sizeof(_coap_state.open_reqs));
     memset(&_coap_state.observers[0], 0, sizeof(_coap_state.observers));
     memset(&_coap_state.observe_memos[0], 0, sizeof(_coap_state.observe_memos));
     /* randomize initial value */
-    _coap_state.last_message_id = random_uint32() & 0xFFFF;
+    atomic_init(&_coap_state.next_message_id, (unsigned)random_uint32());
 
     return _pid;
 }
@@ -609,7 +610,6 @@ void gcoap_register_listener(gcoap_listener_t *listener)
 
 int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len, unsigned code,
                                                               char *path) {
-    ssize_t hdrlen;
     (void)len;
 
     pdu->hdr = (coap_hdr_t *)buf;
@@ -624,11 +624,13 @@ int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len, unsigned code,
                &rand,
                (GCOAP_TOKENLEN - i >= 4) ? 4 : GCOAP_TOKENLEN - i);
     }
-    hdrlen = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, &token[0], GCOAP_TOKENLEN,
-                            code, ++_coap_state.last_message_id);
+    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);
 #else
-    hdrlen = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, NULL, GCOAP_TOKENLEN,
-                            code, ++_coap_state.last_message_id);
+    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);
 #endif
 
     if (hdrlen > 0) {
@@ -682,6 +684,7 @@ size_t gcoap_req_send2(const uint8_t *buf, size_t len,
     assert(resp_handler != NULL);
 
     /* Find empty slot in list of open requests. */
+    mutex_lock(&_coap_state.lock);
     for (int i = 0; i < GCOAP_REQ_WAITING_MAX; i++) {
         if (_coap_state.open_reqs[i].state == GCOAP_MEMO_UNUSED) {
             memo = &_coap_state.open_reqs[i];
@@ -689,6 +692,8 @@ size_t gcoap_req_send2(const uint8_t *buf, size_t len,
             break;
         }
     }
+    mutex_unlock(&_coap_state.lock);
+
     if (memo) {
         memcpy(&memo->hdr_buf[0], buf, GCOAP_HEADER_MAXLEN);
         memo->resp_handler = resp_handler;
@@ -742,7 +747,6 @@ int gcoap_resp_init(coap_pkt_t *pdu, uint8_t *buf, size_t len, unsigned code)
 int gcoap_obs_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
                                                   const coap_resource_t *resource)
 {
-    ssize_t hdrlen;
     gcoap_observe_memo_t *memo = NULL;
 
     _find_obs_memo_resource(&memo, resource);
@@ -751,10 +755,11 @@ int gcoap_obs_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
         return GCOAP_OBS_INIT_UNUSED;
     }
 
-    pdu->hdr = (coap_hdr_t *)buf;
-    hdrlen   = coap_build_hdr(pdu->hdr, COAP_TYPE_NON, &memo->token[0],
-                              memo->token_len, COAP_CODE_CONTENT,
-                              ++_coap_state.last_message_id);
+    pdu->hdr       = (coap_hdr_t *)buf;
+    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, &memo->token[0],
+                                    memo->token_len, COAP_CODE_CONTENT, msgid);
+
     if (hdrlen > 0) {
         uint32_t now       = xtimer_now_usec();
         pdu->observe_value = (now >> GCOAP_OBS_TICK_EXPONENT) & 0xFFFFFF;