From 77553bfee686e64abc958b27600cbc24978ada43 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 20 Jul 2022 15:13:39 +0200 Subject: [PATCH] dhcp: Fix retransmission timeouts The previous code did not ensure that there was a delay of at least `try` seconds after each sent request. Instead, whenever the condvar was signaled, which could be due to retransmitted responses or messages for unrelated transactions (there could even be spurious wakeups), the counter was increased and a retransmit sent. So instead of actually waiting for 15 seconds for a response (and sending 4 retransmits over that timespan), it could happen that all five messages were sent within a second without enough time to actually receive a response. Using an absolute timeout that we reuse as long as there was no timeout and the condvar was signaled for something unrelated, should ensure we wait at least the intended delay after each sent message. Closes strongswan/strongswan#1154 --- src/libcharon/plugins/dhcp/dhcp_socket.c | 40 +++++++++++++++++++++--- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index bc8404bc5a..815d2c65c8 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -384,11 +384,24 @@ static bool request(private_dhcp_socket_t *this, return TRUE; } +/** + * Calculate the timeout to wait for a response for the given try + */ +static inline void get_timeout(int try, timeval_t *timeout) +{ + timeval_t delay = { .tv_sec = try }; + + time_monotonic(timeout); + timeradd(timeout, &delay, timeout); +} + METHOD(dhcp_socket_t, enroll, dhcp_transaction_t*, private_dhcp_socket_t *this, identification_t *identity) { dhcp_transaction_t *transaction; + timeval_t timeout; uint32_t id; + bool got_response; int try; if (!this->rng->get_bytes(this->rng, sizeof(id), (uint8_t*)&id)) @@ -400,11 +413,21 @@ METHOD(dhcp_socket_t, enroll, dhcp_transaction_t*, this->mutex->lock(this->mutex); this->discover->insert_last(this->discover, transaction); + try = 1; + got_response = FALSE; while (try <= DHCP_TRIES && discover(this, transaction)) { - if (!this->condvar->timed_wait(this->condvar, this->mutex, 1000 * try) && - this->request->find_first(this->request, NULL, (void**)&transaction)) + get_timeout(try, &timeout); + while (!this->condvar->timed_wait_abs(this->condvar, this->mutex, timeout)) + { + if (this->request->find_first(this->request, NULL, (void**)&transaction)) + { + got_response = TRUE; + break; + } + } + if (got_response) { break; } @@ -422,10 +445,19 @@ METHOD(dhcp_socket_t, enroll, dhcp_transaction_t*, transaction->get_server(transaction)); try = 1; + got_response = FALSE; while (try <= DHCP_TRIES && request(this, transaction)) { - if (!this->condvar->timed_wait(this->condvar, this->mutex, 1000 * try) && - this->completed->remove(this->completed, transaction, NULL)) + get_timeout(try, &timeout); + while (!this->condvar->timed_wait_abs(this->condvar, this->mutex, timeout)) + { + if (this->completed->remove(this->completed, transaction, NULL)) + { + got_response = TRUE; + break; + } + } + if (got_response) { break; } -- 2.47.2