On Mon, Jan 26, 2004 at 12:42:31PM +0100, Martin Blapp wrote:
>  	/* Now introduce some randomness to the renewal time: */
> +	if (ds.data)
>  	client -> new -> renewal = (((client -> new -> renewal + 3) * 3 / 4) +
>  				    (random () % /* XXX NUMS */
>  				     ((client -> new -> renewal + 3) / 4)));
ds.data...this really amazed me when i looked at it.

so i looked at the freebsd ticket, and it made a little more sense
when i saw that it was an FPE.  but i still don't like the idea of
keying on wether or not the renewal was supplied to us by a server,
to decide wether or not to add random jitter.

i would rather just not enter the arithmatic if the renewal time is
invalid for the modulus (or would produce undesireable jitter, such
as if the renewal value becomes negative), regardless of where that
invalidity came from.

so i revived a patch someone sent me some time ago that i had earmarked
for 3.1 (did not fix a bug (at least not one that people realistically
were running up against except in this guy's lab)), which deals with
servers sending an expiry time of -1 and all the related tomfoolery.

i've attached what i'm comitting.

> Would be good if you can integrate into 3.0.1 final.

this makes 2 code changes, and i don't think i can qualify this one as
being small enough to slip through.  it changes enough to be frightening.
i think we're going to have an rc13 in a week, and hopefully that will
have a short lifetime as an rc.

which means if any of you have other patches or bugs on dhcp-bugs queued,
i'll be comitting them as well.

-- 
David W. Hankins		"If you don't do it right the first time,
Operations Engineer			you'll just have to do it again."
Internet Systems Consortium, Inc.		-- Jack T. Hankins


-- Attached file included as plaintext by Ecartis --

? work.freebsd
Index: client/dhclient.c
===================================================================
RCS file: /proj/cvs/prod/DHCP/client/dhclient.c,v
retrieving revision 1.129.2.16
diff -u -r1.129.2.16 dhclient.c
--- client/dhclient.c	26 Apr 2003 21:51:39 -0000	1.129.2.16
+++ client/dhclient.c	3 Feb 2004 19:39:14 -0000
@@ -793,11 +793,15 @@
 
 	/* If it wasn't specified by the server, calculate it. */
 	if (!client -> new -> renewal)
-		client -> new -> renewal =
-			client -> new -> expiry / 2;
+		client -> new -> renewal = client -> new -> expiry / 2 + 1;
+
+	if (client -> new -> renewal <= 0)
+		client -> new -> renewal = TIME_MAX;
 
 	/* Now introduce some randomness to the renewal time: */
-	client -> new -> renewal = (((client -> new -> renewal + 3) * 3 / 4) +
+	if (client -> new -> renewal <= TIME_MAX / 3 - 3)
+		client -> new -> renewal =
+				(((client -> new -> renewal + 3) * 3 / 4) +
 				    (random () % /* XXX NUMS */
 				     ((client -> new -> renewal + 3) / 4)));
 
@@ -816,14 +820,25 @@
 	} else
 			client -> new -> rebind = 0;
 
-	if (!client -> new -> rebind)
-		client -> new -> rebind =
-			(client -> new -> expiry * 7) / 8; /* XXX NUMS */
+	if (client -> new -> rebind <= 0) {
+		if (client -> new -> expiry <= TIME_MAX / 7)
+			client -> new -> rebind =
+					client -> new -> expiry * 7 / 8;
+		else
+			client -> new -> rebind =
+					client -> new -> expiry / 8 * 7;
+	}
 
 	/* Make sure our randomness didn't run the renewal time past the
 	   rebind time. */
-	if (client -> new -> renewal > client -> new -> rebind)
-		client -> new -> renewal = (client -> new -> rebind * 3) / 4;
+	if (client -> new -> renewal > client -> new -> rebind) {
+		if (client -> new -> rebind <= TIME_MAX / 3)
+			client -> new -> renewal =
+					client -> new -> rebind * 3 / 4;
+		else
+			client -> new -> renewal =
+					client -> new -> rebind / 4 * 3;
+	}
 
 	client -> new -> expiry += cur_time;
 	/* Lease lengths can never be negative. */