Skip to content

Commit 56ea5cd

Browse files
maribumaribu
committed
sys/net/gnrc_sock: fix race in gnrc_sock_recv()
Implement the timeout using ztimer_mbox_get_timeout() to fix a race condition.
1 parent 8230a27 commit 56ea5cd

1 file changed

Lines changed: 36 additions & 76 deletions

File tree

‎sys/net/gnrc/sock/gnrc_sock.c‎

Lines changed: 36 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -28,38 +28,18 @@
2828
#include "net/ipv6/hdr.h"
2929
#include "net/udp.h"
3030
#include "utlist.h"
31-
#if IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
32-
#include "ztimer.h"
33-
#endif
34-
#if IS_USED(MODULE_XTIMER)
35-
#include "xtimer.h"
36-
#endif
37-
3831
#include "sock_types.h"
3932
#include "gnrc_sock_internal.h"
4033

34+
#if IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
35+
# include "ztimer.h"
36+
#endif
37+
4138
#ifdef MODULE_FUZZING
4239
extern gnrc_pktsnip_t *gnrc_pktbuf_fuzzptr;
4340
gnrc_pktsnip_t *gnrc_sock_prevpkt = NULL;
4441
#endif
4542

46-
#if IS_USED(MODULE_XTIMER) || IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
47-
#define _TIMEOUT_MAGIC (0xF38A0B63U)
48-
#define _TIMEOUT_MSG_TYPE (0x8474)
49-
50-
static void _callback_put(void *arg)
51-
{
52-
msg_t timeout_msg = { .sender_pid = KERNEL_PID_UNDEF,
53-
.type = _TIMEOUT_MSG_TYPE,
54-
.content = { .value = _TIMEOUT_MAGIC } };
55-
gnrc_sock_reg_t *reg = arg;
56-
57-
/* should be safe, because otherwise if mbox were filled this callback is
58-
* senseless */
59-
mbox_try_put(&reg->mbox, &timeout_msg);
60-
}
61-
#endif
62-
6343
#ifdef SOCK_HAS_ASYNC
6444
static void _netapi_cb(uint16_t cmd, gnrc_pktsnip_t *pkt, void *ctx)
6545
{
@@ -121,71 +101,51 @@ ssize_t gnrc_sock_recv(gnrc_sock_reg_t *reg, gnrc_pktsnip_t **pkt_out,
121101
if (mbox_size(&reg->mbox) != GNRC_SOCK_MBOX_SIZE) {
122102
return -EINVAL;
123103
}
124-
#if IS_USED(MODULE_ZTIMER_USEC)
125-
ztimer_t timeout_timer = { .base = { .next = NULL } };
126-
if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
127-
timeout_timer.callback = _callback_put;
128-
timeout_timer.arg = reg;
129-
ztimer_set(ZTIMER_USEC, &timeout_timer, timeout);
130-
}
131-
#elif IS_USED(MODULE_ZTIMER_MSEC)
132-
ztimer_t timeout_timer = { .base = { .next = NULL } };
133-
if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
134-
timeout_timer.callback = _callback_put;
135-
timeout_timer.arg = reg;
136-
ztimer_set(ZTIMER_MSEC, &timeout_timer, DIV_ROUND_INF(timeout, US_PER_MS));
137-
}
138-
#elif IS_USED(MODULE_XTIMER)
139-
xtimer_t timeout_timer = { .callback = NULL };
140104

141-
/* xtimer_spin would make this never receive anything.
142-
* Avoid that by setting the minimal not spinning timeout. */
143-
if (timeout < XTIMER_BACKOFF && timeout > 0) {
144-
timeout = XTIMER_BACKOFF;
145-
}
146-
147-
if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
148-
timeout_timer.callback = _callback_put;
149-
timeout_timer.arg = reg;
150-
xtimer_set(&timeout_timer, timeout);
151-
}
152-
#else
153-
assume((timeout == SOCK_NO_TIMEOUT) || (timeout == 0));
154-
#endif
155-
if (timeout != 0) {
156105
#if defined(DEVELHELP) && IS_ACTIVE(SOCK_HAS_ASYNC)
157-
if (reg->async_cb.generic) {
158-
/* this warning is a false positive when sock_*_recv() was not called from
159-
* the asynchronous handler */
160-
LOG_WARNING("gnrc_sock: timeout != 0 within the asynchronous callback lead "
161-
"to unexpected delays within the asynchronous handler.\n");
162-
}
106+
if ((timeout != 0) && (reg->async_cb.generic)) {
107+
/* this warning is a false positive when sock_*_recv() was not called from
108+
* the asynchronous handler */
109+
LOG_WARNING("gnrc_sock: timeout != 0 within the asynchronous callback lead "
110+
"to unexpected delays within the asynchronous handler.\n");
111+
}
163112
#endif
113+
114+
if (timeout == SOCK_NO_TIMEOUT) {
164115
mbox_get(&reg->mbox, &msg);
165116
}
166-
else {
117+
else if (timeout == 0) {
167118
if (!mbox_try_get(&reg->mbox, &msg)) {
168119
return -EAGAIN;
169120
}
170121
}
171-
#if IS_USED(MODULE_ZTIMER_USEC)
172-
ztimer_remove(ZTIMER_USEC, &timeout_timer);
173-
#elif IS_USED(MODULE_ZTIMER_MSEC)
174-
ztimer_remove(ZTIMER_MSEC, &timeout_timer);
175-
#elif IS_USED(MODULE_XTIMER)
176-
xtimer_remove(&timeout_timer);
177-
#endif
122+
else {
123+
/* Preferring low power over us precision here if both options are
124+
* possible. This is typically the better trade-off, as even on fast
125+
* networks round-trip-times are typically measured in ms rather than
126+
* in us */
127+
if (IS_USED(MODULE_ZTIMER_MSEC)) {
128+
/* rounding up seems more sensible here */
129+
uint32_t timeout_ms = (timeout + US_PER_MS - 1) / US_PER_MS;
130+
if (ztimer_mbox_get_timeout(ZTIMER_MSEC, &reg->mbox, &msg, timeout_ms)) {
131+
return -ETIMEDOUT;
132+
}
133+
}
134+
else if (IS_USED(MODULE_ZTIMER_USEC)) {
135+
if (ztimer_mbox_get_timeout(ZTIMER_USEC, &reg->mbox, &msg, timeout)) {
136+
return -ETIMEDOUT;
137+
}
138+
}
139+
else {
140+
/* cannot do timeout without a timer */
141+
assert(0);
142+
return -ENOTSUP;
143+
}
144+
}
178145
switch (msg.type) {
179146
case GNRC_NETAPI_MSG_TYPE_RCV:
180147
pkt = msg.content.ptr;
181148
break;
182-
#if IS_USED(MODULE_XTIMER) || IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
183-
case _TIMEOUT_MSG_TYPE:
184-
if (msg.content.value == _TIMEOUT_MAGIC) {
185-
return -ETIMEDOUT;
186-
}
187-
#endif
188-
/* Falls Through. */
189149
default:
190150
return -EINVAL;
191151
}

0 commit comments

Comments
 (0)