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

diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c
index bc8404bc5ae859ba2172ede6bc573431381a2353..815d2c65c8e9d2a3c9e751186449bde8758be225 100644 (file)
--- 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;
                }