kern/154006: tcp "window probe" bug on 64bit

Bruce Evans brde at optusnet.com.au
Sat Jan 15 06:10:10 UTC 2011


The following reply was made to PR kern/154006; it has been noted by GNATS.

From: Bruce Evans <brde at optusnet.com.au>
To: Bruce Evans <brde at optusnet.com.au>
Cc: Stefan `Sec` Zehl <sec at 42.org>, freebsd-bugs at freebsd.org,
        FreeBSD-gnats-submit at freebsd.org
Subject: Re: kern/154006: tcp "window probe" bug on 64bit
Date: Sat, 15 Jan 2011 17:05:02 +1100 (EST)

 On Sat, 15 Jan 2011, Bruce Evans wrote:
 
 > ...
 > Later code in tcp_output uses bogus casts to long and larger code instead:
 >
 > % 	if (recwin < (long)(tp->rcv_adv - tp->rcv_nxt))
 > % 		recwin = (long)(tp->rcv_adv - tp->rcv_nxt);
 > % 	if (recwin > (long)TCP_MAXWIN << tp->rcv_scale)
 > % 		recwin = (long)TCP_MAXWIN << tp->rcv_scale;
 > % 	...
 > % 	if (recwin > 0 && SEQ_GT(tp->rcv_nxt + recwin, tp->rcv_adv))
 > % 		tp->rcv_adv = tp->rcv_nxt + recwin;
 >
 > Note that the first statement avoids using the technically incorrect
 > SEQ_FOO() although its internals are better (cast to int instead of
 > long).  It uses cases essentially like yours.  Then further analysis
                    ^^^^^ a cast
 > is simpler because everything is converted to long.  The second starement
 > is similar to the first half of the broken expression.  Large code using
 > if's and else's and tests (x >= y) before subtracting y from x is much
 > easier to get right than 1 complicated 1-statement expression like the
 > broken one.  It takes these (x >= y) tests to make code with mixed types
 > obviously correct.  But I prefer small fast code with ints for everything,
 > since type analyis is too hard.
 
 But the casts to long are not good.  Here they have no effect except
 to break the warning about the bad type of `recwin'.  recwin has type
 long, so assignment to it does the same conversion as the cast, possibly
 with a warning about the implicit conversion if it might overflow (can
 overflow only on 32-bit arches).  The code depends on rcv_adv being
 sequentially >= rcv_next with or without the cast.  Otherwise, the
 difference is huge unsigned int, and the cast only changes this (by
 benign overflow) on 32-bit arches.
 
 This can be fixed by casting to int instead of long (now the cast may have
 an effect, and breaking the warning may be intential), or my proposed
 SEQ_DIFF() macros work well here:
 
  	int recwin, sd;
  	...
  	sd = SEQ_NONNEG_NONLARGE_DIFF(tp->rcv_adv, tp->rcv_nxt);
  	/*
  	 * SEQ_DIFF() supports negative differences;
  	 * SEQ_NONNEG_NONGLARGE_DIFF() KASSERT()s that they don't happen and
  	 * are not too large.  This name is too long.
  	 */
  	if (recwin < sd)
  		recwin = sd;
  	...
 
 Bruce


More information about the freebsd-bugs mailing list