From: Jan Klemkow Subject: Re: TCP window shifts unsinged long To: Alexander Bluhm Cc: tech@openbsd.org Date: Wed, 6 Aug 2025 14:26:34 +0200 On Tue, Aug 05, 2025 at 02:15:32PM +0200, Alexander Bluhm wrote: > 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? ok jan@ > 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; >