From 499b0785d88b7a354ba01c07a055e62be018cc9a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Sep 2019 17:16:03 +0200 Subject: [PATCH 1/4] probe: fix leaking message during client probe --- shared/n-dhcp4/src/n-dhcp4-c-probe.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/shared/n-dhcp4/src/n-dhcp4-c-probe.c b/shared/n-dhcp4/src/n-dhcp4-c-probe.c index 107c18bbad..65d6ef8373 100644 --- a/shared/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/shared/n-dhcp4/src/n-dhcp4-c-probe.c @@ -836,7 +836,8 @@ static int n_dhcp4_client_probe_transition_offer(NDhcp4ClientProbe *probe, NDhcp case N_DHCP4_CLIENT_PROBE_STATE_REBINDING: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore */ + /* ignore, but consume message. */ + n_dhcp4_incoming_free(message); break; } @@ -885,7 +886,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I if (r) return r; - /* message consumed, don to fail */ + /* message consumed, do not fail */ n_dhcp4_client_lease_link(lease, probe); @@ -903,7 +904,8 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I case N_DHCP4_CLIENT_PROBE_STATE_GRANTED: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore */ + /* ignore, but consume message. */ + n_dhcp4_incoming_free(message); break; } From ce5c8db1753357a0b853357ee1e4cd74ba5ad50d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Sep 2019 17:22:17 +0200 Subject: [PATCH 2/4] probe: unconditionally pass ownership of message in n_dhcp4_client_probe_dispatch_io() It is error prone when a function consumes an input only in certain cases (and telling the caller via the return code). At least in these cases, the message is never used afterwards, and we can always pass it on. --- shared/n-dhcp4/src/n-dhcp4-c-probe.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/shared/n-dhcp4/src/n-dhcp4-c-probe.c b/shared/n-dhcp4/src/n-dhcp4-c-probe.c index 65d6ef8373..88af36afea 100644 --- a/shared/n-dhcp4/src/n-dhcp4-c-probe.c +++ b/shared/n-dhcp4/src/n-dhcp4-c-probe.c @@ -800,7 +800,8 @@ static int n_dhcp4_client_probe_transition_lifetime(NDhcp4ClientProbe *probe) { return 0; } -static int n_dhcp4_client_probe_transition_offer(NDhcp4ClientProbe *probe, NDhcp4Incoming *message) { +static int n_dhcp4_client_probe_transition_offer(NDhcp4ClientProbe *probe, NDhcp4Incoming *message_take) { + _c_cleanup_(n_dhcp4_incoming_freep) NDhcp4Incoming *message = message_take; _c_cleanup_(n_dhcp4_client_lease_unrefp) NDhcp4ClientLease *lease = NULL; NDhcp4CEventNode *node; int r; @@ -818,7 +819,7 @@ static int n_dhcp4_client_probe_transition_offer(NDhcp4ClientProbe *probe, NDhcp if (r) return r; - /* message consumed, do not fail */ + message = NULL; /* consumed */ n_dhcp4_client_lease_link(lease, probe); @@ -836,15 +837,15 @@ static int n_dhcp4_client_probe_transition_offer(NDhcp4ClientProbe *probe, NDhcp case N_DHCP4_CLIENT_PROBE_STATE_REBINDING: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore, but consume message. */ - n_dhcp4_incoming_free(message); + /* ignore */ break; } return 0; } -static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4Incoming *message) { +static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4Incoming *message_take) { + _c_cleanup_(n_dhcp4_incoming_freep) NDhcp4Incoming *message = message_take; _c_cleanup_(n_dhcp4_client_lease_unrefp) NDhcp4ClientLease *lease = NULL; NDhcp4CEventNode *node; int r; @@ -863,7 +864,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I if (r) return r; - /* message consumed, do not fail */ + message = NULL; /* consumed */ n_dhcp4_client_lease_link(lease, probe); @@ -886,7 +887,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I if (r) return r; - /* message consumed, do not fail */ + message = NULL; /* consumed */ n_dhcp4_client_lease_link(lease, probe); @@ -904,8 +905,7 @@ static int n_dhcp4_client_probe_transition_ack(NDhcp4ClientProbe *probe, NDhcp4I case N_DHCP4_CLIENT_PROBE_STATE_GRANTED: case N_DHCP4_CLIENT_PROBE_STATE_EXPIRED: default: - /* ignore, but consume message. */ - n_dhcp4_incoming_free(message); + /* ignore */ break; } @@ -1169,17 +1169,15 @@ int n_dhcp4_client_probe_dispatch_io(NDhcp4ClientProbe *probe, uint32_t events) switch (type) { case N_DHCP4_MESSAGE_OFFER: r = n_dhcp4_client_probe_transition_offer(probe, message); + message = NULL; /* consumed */ if (r) return r; - else - message = NULL; /* consumed */ break; case N_DHCP4_MESSAGE_ACK: r = n_dhcp4_client_probe_transition_ack(probe, message); + message = NULL; /* consumed */ if (r) return r; - else - message = NULL; /* consumed */ break; case N_DHCP4_MESSAGE_NAK: r = n_dhcp4_client_probe_transition_nak(probe); From d29c8b615a69438f01e58c329b8e34747e008685 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Sep 2019 16:41:12 +0200 Subject: [PATCH 3/4] incoming: don't handle 0xFFFFFFFF timestamps special in n_dhcp4_incoming_query_u32() First of all, from the naming of n_dhcp4_incoming_query_u32() it is confusing to coerce 0xFFFFFFFF to zero. It should just return the plain value. Also note that n_dhcp4_incoming_query_u32() only has three callers: n_dhcp4_incoming_query_lifetime(), n_dhcp4_incoming_query_t1() and n_dhcp4_incoming_query_t2(). Looking further, those three functions only have one caller: n_dhcp4_incoming_get_timeouts(). Note how the code there already tries to handle UINT32_MAX and interprets it as infinity (UINT64_MAX). But as it was, UINT32_MAX never actually was returned. It seems that RFC [1] does not specially define the meanings of 0xFFFFFFFF and 0. It sounds reasonable to assume that 0 just means 0 lifetime, and 0xFFFFFFFF means infinity. On the other hand, compare this to systemd's code [2], which coerces 0 to 1. This does not seem right to me though. Note how systemd returns 0xFFFFFFFF as-is. Drop the special handling of 0xFFFFFFFF from n_dhcp4_incoming_query_u32(). It now just returns the plain value and it's up to n_dhcp4_incoming_get_timeouts() to make sense of that. This will fix behavior, so that 0xFFFFFFFF will be reported as infinity, and not as zero. [1] https://tools.ietf.org/html/rfc2132#section-9.2 [2] https://github.com/systemd/systemd/blob/68c2b5ddb1881c40201c1d86a7852dd5c5c06a76/src/libsystemd-network/sd-dhcp-lease.c#L553 --- shared/n-dhcp4/src/n-dhcp4-incoming.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/shared/n-dhcp4/src/n-dhcp4-incoming.c b/shared/n-dhcp4/src/n-dhcp4-incoming.c index 255da45845..e7234c0a52 100644 --- a/shared/n-dhcp4/src/n-dhcp4-incoming.c +++ b/shared/n-dhcp4/src/n-dhcp4-incoming.c @@ -365,10 +365,7 @@ static int n_dhcp4_incoming_query_u32(NDhcp4Incoming *message, uint8_t option, u memcpy(&be32, data, sizeof(be32)); - if (be32 == (uint32_t)-1) - *u32p = 0; - else - *u32p = ntohl(be32); + *u32p = ntohl(be32); return 0; } From d688019bf8494dc8a141abbb8b8e9a3f1a89a394 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Sep 2019 18:06:00 +0200 Subject: [PATCH 4/4] lease: add n_dhcp4_client_lease_get_basetime() The API already had n_dhcp4_client_lease_get_lifetime(), which is the CLOCK_BOOTTIME when the lease expires (or ((uint64_t)-1)). But it might be interesting to know the actual lease duration and when the lease was received (and the time started to count). Expose an API for that. With this, one can also calculate the original, exact lease lifetime, by subtracting n_dhcp4_client_lease_get_basetime() from n_dhcp4_client_lease_get_lifetime(), while taking care of ((uint64_t)-1). --- shared/n-dhcp4/src/n-dhcp4-c-lease.c | 12 ++++++++++++ shared/n-dhcp4/src/n-dhcp4.h | 1 + 2 files changed, 13 insertions(+) diff --git a/shared/n-dhcp4/src/n-dhcp4-c-lease.c b/shared/n-dhcp4/src/n-dhcp4-c-lease.c index c14a9daf0a..695a112ab7 100644 --- a/shared/n-dhcp4/src/n-dhcp4-c-lease.c +++ b/shared/n-dhcp4/src/n-dhcp4-c-lease.c @@ -217,6 +217,18 @@ _c_public_ void n_dhcp4_client_lease_get_siaddr(NDhcp4ClientLease *lease, struct siaddr->s_addr = header->siaddr; } +/** + * n_dhcp4_client_lease_get_basetime() - get the timestamp when the lease was received. + * @lease: the lease to operate on + * @ns_basetimep: return argument for the base time in nano seconds + * + * Gets the timestamp when the lease was received in CLOCK_BOOTTIME. This + * is also the base timestamp for the expiration of the lifetime and t1/t2. + */ +_c_public_ void n_dhcp4_client_lease_get_basetime(NDhcp4ClientLease *lease, uint64_t *ns_basetimep) { + *ns_basetimep = lease->message->userdata.base_time; +} + /** * n_dhcp4_client_lease_get_lifetime() - get the lifetime * @lease: the lease to operate on diff --git a/shared/n-dhcp4/src/n-dhcp4.h b/shared/n-dhcp4/src/n-dhcp4.h index ef75eca7c8..a800352b50 100644 --- a/shared/n-dhcp4/src/n-dhcp4.h +++ b/shared/n-dhcp4/src/n-dhcp4.h @@ -157,6 +157,7 @@ NDhcp4ClientLease *n_dhcp4_client_lease_unref(NDhcp4ClientLease *lease); void n_dhcp4_client_lease_get_yiaddr(NDhcp4ClientLease *lease, struct in_addr *yiaddr); void n_dhcp4_client_lease_get_siaddr(NDhcp4ClientLease *lease, struct in_addr *siaddr); +void n_dhcp4_client_lease_get_basetime(NDhcp4ClientLease *lease, uint64_t *ns_basetimep); void n_dhcp4_client_lease_get_lifetime(NDhcp4ClientLease *lease, uint64_t *ns_lifetimep); int n_dhcp4_client_lease_query(NDhcp4ClientLease *lease, uint8_t option, uint8_t **datap, size_t *n_datap);