Download raw body.
TCP window shifts unsinged long
Sure, that's the history but back in those days, long was entirely 32-bit.
64-bit machines arrived later.
So the mistake perhaps is that when 64-bit machines arrived, this long was
not changed an int (or some typedef of 32-bit int)
> Getting consistent u_long makes reasoning about integer conversions
> easier. I am planning to convert the remaining long window sizes
> to u_long in a separate diff.
On 64-bit machines, yes. On 32-bit machines, that reasoning fails.
If you wanted the advantage of catching the 32-bit edges correctly using
a 64-bit type, it would need to a gauranteed 64-bit type "long long",
otherwise bugs can sneak in. On 64-bit machines, this "long long" would
be the same cost as "long". On 32-bit machines, the cost is higher.
I understand the point that 32-bit + rolling around checks can get complicated,
but the use of 32-bit without rolling around checks, or rolling-around tests which
will eventually becomed tuned to the expectation that long is 64-bit on most
machines -- on 32-bit machines, this seems dangerous.
Alexander Bluhm <bluhm@openbsd.org> wrote:
> On Sat, Aug 09, 2025 at 03:01:36PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 5 Aug 2025 14:15:32 +0200
> > > From: Alexander Bluhm <bluhm@openbsd.org>
> > >
> > > Hi,
> > >
> > > Coverity complains about signed integer shift in
> > > tiwin = th->th_win << tp->snd_scale
> > > I think we are safe as TCP_MAX_WINSHIFT 14 prevents us from using
> > > potential sign bits.
> > >
> > > However, the type of the TCP window is inconsistent. I want to
> > > convert it to long. Maybe u_long would be even better, but let's
> > > start with the integer size. Converting to unsigned everywhere can
> > > be done later.
> > >
> > > ok?
> >
> > Sorry, not that isn't right. It makes no sense to use long for this
> > type, since on 32-bit systems that is the same as int. Where did this
> > go wrong? This doesn't seem to be an area where we want different
> > beheviour between 32-bit and 64-bit architectures.
>
> In tcp_input long tiwin was introduced here:
> commit 2d8be1d57460e127f5505122950d3cb8919e6263
> Author: Andrew Cherenson <andrew>
> Date: Fri Jan 8 18:42:28 1993 -0800
>
> (By Sklower) checkpoint the current state of Cherenson's work.
>
> SCCS-vsn: 7.30
>
> and tcp_output has long win since:
> commit 1ba008c794aee88f6e22ac3474b056a69e00f641
> Author: Mike Karels <karels>
> Date: Mon Feb 29 17:17:18 1988 -0800
>
> use just one mbuf when sending small data segments;
> truncate sbspace to our max window
>
> SCCS-vsn: 7.14
>
> Note that this diff is not about making the window size 64 bit in
> general. There are some type conversion that are inconsistent.
> When done in u_long they look less dangerous. My diff also removes
> some ugly casts.
>
> Wondow size calculations are more or less u_long or long for 35
> years. I will not switch back to int, now that we have 64 bit
> architectures.
>
> Getting consistent u_long makes reasoning about integer conversions
> easier. I am planning to convert the remaining long window sizes
> to u_long in a separate diff.
>
> bluhm
>
> > > Index: netinet/tcp.h
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp.h,v
> > > diff -u -p -r1.24 tcp.h
> > > --- netinet/tcp.h 19 May 2023 01:04:39 -0000 1.24
> > > +++ netinet/tcp.h 2 Aug 2025 22:25:50 -0000
> > > @@ -113,7 +113,7 @@ struct tcphdr {
> > > */
> > > #define TCP_MSS 512
> > >
> > > -#define TCP_MAXWIN 65535 /* largest value for (unscaled) window */
> > > +#define TCP_MAXWIN 65535UL /* largest value for (unscaled) window */
> > >
> > > #define TCP_MAX_WINSHIFT 14 /* maximum window shift */
> > >
> > > Index: netinet/tcp_input.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
> > > diff -u -p -r1.457 tcp_input.c
> > > --- netinet/tcp_input.c 24 Jul 2025 21:34:07 -0000 1.457
> > > +++ netinet/tcp_input.c 2 Aug 2025 22:28:22 -0000
> > > @@ -690,7 +690,7 @@ findpcb:
> > >
> > > /* Unscale the window into a 32-bit value. */
> > > if ((tiflags & TH_SYN) == 0)
> > > - tiwin = th->th_win << tp->snd_scale;
> > > + tiwin = (u_long)th->th_win << tp->snd_scale;
> > > else
> > > tiwin = th->th_win;
> > >
> > > @@ -1156,12 +1156,12 @@ findpcb:
> > > * but not less than advertised window.
> > > */
> > > {
> > > - int win;
> > > + long win;
> > >
> > > win = sbspace(&so->so_rcv);
> > > if (win < 0)
> > > win = 0;
> > > - tp->rcv_wnd = imax(win, (int)(tp->rcv_adv - tp->rcv_nxt));
> > > + tp->rcv_wnd = lmax(win, (int)(tp->rcv_adv - tp->rcv_nxt));
> > > }
> > >
> > > switch (tp->t_state) {
> > > @@ -1551,7 +1551,7 @@ trimthenstep6:
> > > (TF_RCVD_SCALE|TF_REQ_SCALE)) {
> > > tp->snd_scale = tp->requested_s_scale;
> > > tp->rcv_scale = tp->request_r_scale;
> > > - tiwin = th->th_win << tp->snd_scale;
> > > + tiwin = (u_long)th->th_win << tp->snd_scale;
> > > }
> > > tcp_flush_queue(tp);
> > > tp->snd_wl1 = th->th_seq - 1;
> > > @@ -1582,11 +1582,13 @@ trimthenstep6:
> > > if (do_ecn && (tiflags & TH_ECE)) {
> > > if ((tp->t_flags & TF_ECN_PERMIT) &&
> > > SEQ_GEQ(tp->snd_una, tp->snd_last)) {
> > > - u_int win;
> > > + u_long win;
> > >
> > > - win = min(tp->snd_wnd, tp->snd_cwnd) / tp->t_maxseg;
> > > + win = ulmin(tp->snd_wnd, tp->snd_cwnd) /
> > > + tp->t_maxseg;
> > > if (win > 1) {
> > > - tp->snd_ssthresh = win / 2 * tp->t_maxseg;
> > > + tp->snd_ssthresh = win / 2 *
> > > + tp->t_maxseg;
> > > tp->snd_cwnd = tp->snd_ssthresh;
> > > tp->snd_last = tp->snd_max;
> > > tp->t_flags |= TF_SEND_CWR;
> > > Index: netinet/tcp_output.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_output.c,v
> > > diff -u -p -r1.156 tcp_output.c
> > > --- netinet/tcp_output.c 8 Jul 2025 00:47:41 -0000 1.156
> > > +++ netinet/tcp_output.c 2 Aug 2025 22:23:04 -0000
> > > @@ -418,7 +418,7 @@ again:
> > > * taking into account that we are limited by
> > > * TCP_MAXWIN << tp->rcv_scale.
> > > */
> > > - long adv = lmin(win, (long)TCP_MAXWIN << tp->rcv_scale) -
> > > + long adv = lmin(win, TCP_MAXWIN << tp->rcv_scale) -
> > > (tp->rcv_adv - tp->rcv_nxt);
> > >
> > > if (adv >= (long) (2 * tp->t_maxseg))
> > > @@ -859,13 +859,13 @@ send:
> > > */
> > > if (win < (rcv_hiwat / 4) && win < (long)tp->t_maxseg)
> > > win = 0;
> > > - if (win > (long)TCP_MAXWIN << tp->rcv_scale)
> > > - win = (long)TCP_MAXWIN << tp->rcv_scale;
> > > - if (win < (long)(int32_t)(tp->rcv_adv - tp->rcv_nxt))
> > > - win = (long)(int32_t)(tp->rcv_adv - tp->rcv_nxt);
> > > + if (win > TCP_MAXWIN << tp->rcv_scale)
> > > + win = TCP_MAXWIN << tp->rcv_scale;
> > > + if (win < (int32_t)(tp->rcv_adv - tp->rcv_nxt))
> > > + win = (int32_t)(tp->rcv_adv - tp->rcv_nxt);
> > > if (flags & TH_RST)
> > > win = 0;
> > > - th->th_win = htons((u_int16_t) (win>>tp->rcv_scale));
> > > + th->th_win = htons((u_int16_t)(win >> tp->rcv_scale));
> > > if (th->th_win == 0)
> > > tp->t_sndzerowin++;
> > > if (SEQ_GT(tp->snd_up, tp->snd_nxt)) {
> > > @@ -1164,7 +1164,7 @@ out:
> > > * then remember the size of the advertised window.
> > > * Any pending ACK has now been sent.
> > > */
> > > - if (win > 0 && SEQ_GT(tp->rcv_nxt+win, tp->rcv_adv))
> > > + if (win > 0 && SEQ_GT(tp->rcv_nxt + win, tp->rcv_adv))
> > > tp->rcv_adv = tp->rcv_nxt + win;
> > > tp->last_ack_sent = tp->rcv_nxt;
> > > tp->t_sndacktime = now;
> > > Index: netinet/tcp_subr.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
> > > diff -u -p -r1.216 tcp_subr.c
> > > --- netinet/tcp_subr.c 18 Jul 2025 08:39:14 -0000 1.216
> > > +++ netinet/tcp_subr.c 2 Aug 2025 22:11:34 -0000
> > > @@ -294,7 +294,7 @@ tcp_respond(struct tcpcb *tp, caddr_t te
> > > tcp_seq ack, tcp_seq seq, int flags, u_int rtableid, uint64_t now)
> > > {
> > > int tlen;
> > > - int win = 0;
> > > + long win = 0;
> > > struct mbuf *m = NULL;
> > > struct tcphdr *th;
> > > struct ip *ip;
> > >
> > >
>
TCP window shifts unsinged long