On Fri, 2021-04-09 at 09:47 +0200, Florian Obser wrote:
> On Fri, Apr 09, 2021 at 09:41:24AM +0200, Florian Obser wrote:
> > I think it would be better (and less ugly) to treat invalid values as
> > if they had not been set. Could you pull the two checks up before
> > if(renewal_time == 0) and do something like this:
> >
> > if (renewal_time >= rebinding_time) {
> > log_warn(...)
> > renewal_time = 0;
> > }
> > if (rebinding_time >= lease_time) {
> > log_warn(...)
> > rebinding_time = 0;
> > }
> >
>
> It might even be better to ignore both values if at least one is
> invalid.
>
> if (renewal_time >= rebinding_time || rebinding_time >= lease_time) {
> ...
>
> Since you are in there and have a device that causes trouble could you
> give this a bit of thought? I'm currently swamped with other things
> and won't have time until the evening.
>
Using your latter suggestion would loose information on what grounds the
times are reset. If someone wants to do some proper work on their server
side this is useful information. My diff below still works for me and
shows full information while still resetting both. I also changed the
printf statement a little, since this reads a little better to me, but
don´t mind changing it back if you feel strong about it.
Since the lease time here is 24 hours I haven´t tested this yet with
a renewal.
Index: engine.c
===================================================================
RCS file: /cvs/src/sbin/dhcpleased/engine.c,v
retrieving revision 1.11
diff -u -p -r1.11 engine.c
--- engine.c 22 Mar 2021 15:34:07 -0000 1.11
+++ engine.c 9 Apr 2021 08:26:37 -0000
@@ -972,23 +972,24 @@ parse_dhcp(struct dhcpleased_iface *ifac
}
/* RFC 2131 4.4.5 */
+ /* Ignore invalid T1/T2 options */
+ if (renewal_time >= rebinding_time) {
+ log_warnx("%s: renewal_time(%u) >= rebinding_time(%u) "
+ "from %s: using defaults",
+ __func__, renewal_time, rebinding_time, from);
+ renewal_time = rebinding_time = 0;
+ } else if (rebinding_time >= lease_time) {
+ log_warnx("%s: rebinding_time(%u) >= lease_time(%u) "
+ "from %s: using defaults",
+ __func__, rebinding_time, lease_time, from);
+ renewal_time = rebinding_time = 0;
+ }
+
if(renewal_time == 0)
renewal_time = lease_time / 2;
if (rebinding_time == 0)
rebinding_time = lease_time - (lease_time / 8);
- if (renewal_time >= rebinding_time) {
- log_warnx("%s: renewal_time >= rebinding_time "
- "(%u >= %u) from %s", __func__, renewal_time,
- rebinding_time, from);
- return;
- }
- if (rebinding_time >= lease_time) {
- log_warnx("%s: rebinding_time >= lease_time"
- "(%u >= %u) from %s", __func__, rebinding_time,
- lease_time, from);
- return;
- }
clock_gettime(CLOCK_MONOTONIC, &iface->request_time);
iface->server_identifier.s_addr = server_identifier.s_addr;
iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
