From 687f41f10bd5f493e82395e127f7d4d52b11d308 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Tue, 1 Jun 2021 00:19:03 +0200 Subject: [PATCH] udhcpc[6]: fix "untangle timeout and remaining lease" fallout As reported in bug 13776, before this fix the renew never times out. function old new delta udhcpc_main 2541 2585 +44 udhcpc6_main 2567 2558 -9 ------------------------------------------------------------------------------ (add/remove: 0/0 grow/shrink: 1/1 up/down: 44/-9) Total: 35 bytes Signed-off-by: Denys Vlasenko --- networking/udhcp/d6_dhcpc.c | 34 ++++++++++++++++++----------- networking/udhcp/dhcpc.c | 43 ++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/networking/udhcp/d6_dhcpc.c b/networking/udhcp/d6_dhcpc.c index 0a5cae310..5bca4a824 100644 --- a/networking/udhcp/d6_dhcpc.c +++ b/networking/udhcp/d6_dhcpc.c @@ -1356,14 +1356,17 @@ int udhcpc6_main(int argc UNUSED_PARAM, char **argv) log1("waiting %u seconds", timeout); diff = (unsigned)monotonic_sec(); retval = poll(pfds, 2, timeout * 1000); + diff = (unsigned)monotonic_sec() - diff; + lease_remaining -= diff; + if (lease_remaining < 0) + lease_remaining = 0; + timeout -= diff; + if (timeout < 0) + timeout = 0; + if (retval < 0) { /* EINTR? A signal was caught, don't panic */ if (errno == EINTR) { - diff = (unsigned)monotonic_sec() - diff; - lease_remaining -= diff; - if (lease_remaining < 0) - lease_remaining = 0; - timeout -= diff; continue; } /* Else: an error occured, panic! */ @@ -1455,7 +1458,6 @@ int udhcpc6_main(int argc UNUSED_PARAM, char **argv) case_RENEW_REQUESTED: case RENEWING: if (packet_num < 3) { - packet_num++; /* send an unicast renew request */ /* Sometimes observed to fail (EADDRNOTAVAIL) to bind * a new UDP socket for sending inside send_renew. @@ -1471,23 +1473,26 @@ int udhcpc6_main(int argc UNUSED_PARAM, char **argv) send_d6_renew(xid, &srv6_buf, requested_ipv6); timeout = discover_timeout; /* ^^^ used to be = lease_remaining / 2 - WAY too long */ + packet_num++; continue; } /* Timed out, enter rebinding state */ log1s("entering rebinding state"); client_data.state = REBINDING; + packet_num = 0; /* fall right through */ case REBINDING: /* Switch to bcast receive */ change_listen_mode(LISTEN_RAW); /* Lease is *really* about to run out, * try to find DHCP server using broadcast */ - if (lease_remaining > 0) { + if (lease_remaining > 0 && packet_num < 3) { if (opt & OPT_l) send_d6_info_request(xid); else /* send a broadcast renew request */ send_d6_renew(xid, /*server_ipv6:*/ NULL, requested_ipv6); timeout = discover_timeout; + packet_num++; continue; } /* Timed out, enter init state */ @@ -1809,12 +1814,9 @@ int udhcpc6_main(int argc UNUSED_PARAM, char **argv) start = monotonic_sec(); d6_run_script(packet.d6_options, packet_end, (client_data.state == REQUESTING ? "bound" : "renew")); - timeout = (unsigned)lease_remaining / 2; - timeout -= (unsigned)monotonic_sec() - start; - packet_num = 0; - - client_data.state = BOUND; - change_listen_mode(LISTEN_NONE); + lease_remaining -= (unsigned)monotonic_sec() - start; + if (lease_remaining < 0) + lease_remaining = 0; if (opt & OPT_q) { /* quit after lease */ goto ret0; } @@ -1827,6 +1829,12 @@ int udhcpc6_main(int argc UNUSED_PARAM, char **argv) opt = ((opt & ~OPT_b) | OPT_f); } #endif + +// BOUND_for_half_lease: + timeout = (unsigned)lease_remaining / 2; + client_data.state = BOUND; + change_listen_mode(LISTEN_NONE); + packet_num = 0; continue; /* back to main loop */ } continue; diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c index 6666cbce6..ea06405ba 100644 --- a/networking/udhcp/dhcpc.c +++ b/networking/udhcp/dhcpc.c @@ -1417,14 +1417,17 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) log1("waiting %u seconds", timeout); diff = (unsigned)monotonic_sec(); retval = poll(pfds, 2, timeout * 1000); + diff = (unsigned)monotonic_sec() - diff; + lease_remaining -= diff; + if (lease_remaining < 0) + lease_remaining = 0; + timeout -= diff; + if (timeout < 0) + timeout = 0; + if (retval < 0) { /* EINTR? A signal was caught, don't panic */ if (errno == EINTR) { - diff = (unsigned)monotonic_sec() - diff; - lease_remaining -= diff; - if (lease_remaining < 0) - lease_remaining = 0; - timeout -= diff; continue; } /* Else: an error occurred, panic! */ @@ -1513,7 +1516,6 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) case_RENEW_REQUESTED: case RENEWING: if (packet_num < 3) { - packet_num++; /* send an unicast renew request */ /* Sometimes observed to fail (EADDRNOTAVAIL) to bind * a new UDP socket for sending inside send_renew. @@ -1526,6 +1528,7 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) if (send_renew(xid, server_addr, requested_ip) >= 0) { timeout = discover_timeout; /* ^^^ used to be = lease_remaining / 2 - WAY too long */ + packet_num++; continue; } /* else: error sending. @@ -1534,21 +1537,26 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) * which wasn't reachable (and probably did not exist). */ } -//TODO: if 3 renew's failed (no reply) but remaining lease is large, -//it might make sense to make a large pause (~1 hour?) and try later? +//TODO: if 3 renew's failed (no reply) but remaining lease is large enough, +//it might make sense to go back to BOUND and try later? a-la +// if (lease_remaining > 30) goto BOUND_for_half_lease; +//If we do that, "packet_num < 3" test below might be superfluous +//(lease_remaining will run out anyway) /* Timed out or error, enter rebinding state */ log1s("entering rebinding state"); client_data.state = REBINDING; + packet_num = 0; /* fall right through */ case REBINDING: /* Switch to bcast receive */ change_listen_mode(LISTEN_RAW); /* Lease is *really* about to run out, * try to find DHCP server using broadcast */ - if (lease_remaining > 0) { + if (lease_remaining > 0 && packet_num < 3) { /* send a broadcast renew request */ send_renew(xid, 0 /*INADDR_ANY*/, requested_ip); timeout = discover_timeout; + packet_num++; continue; } /* Timed out, enter init state */ @@ -1773,13 +1781,9 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) /* enter bound state */ start = monotonic_sec(); udhcp_run_script(&packet, client_data.state == REQUESTING ? "bound" : "renew"); - timeout = (unsigned)lease_remaining / 2; -//TODO: why / 2? - timeout -= (unsigned)monotonic_sec() - start; - packet_num = 0; - - client_data.state = BOUND; - change_listen_mode(LISTEN_NONE); + lease_remaining -= (unsigned)monotonic_sec() - start; + if (lease_remaining < 0) + lease_remaining = 0; if (opt & OPT_q) { /* quit after lease */ goto ret0; } @@ -1792,9 +1796,14 @@ int udhcpc_main(int argc UNUSED_PARAM, char **argv) opt = ((opt & ~OPT_b) | OPT_f); } #endif + +// BOUND_for_half_lease: + timeout = (unsigned)lease_remaining / 2; + client_data.state = BOUND; + change_listen_mode(LISTEN_NONE); /* make future renew packets use different xid */ /* xid = random_xid(); ...but why bother? */ - + packet_num = 0; continue; /* back to main loop */ } if (*message == DHCPNAK) {