[net.bugs.4bsd] 4.3BSD network bug

karels@MONET (Mike Karels) (10/23/86)

Index:	sys/netinet/tcp_output.c 4.3BSD FIX

This is the first of a set of three bug reports with fixes for the network
in 4.3BSD.  All 4.3 sites should install the modifications described in
these reports.  The bug described in this report is the most serious,
as it can cause unnoticed loss of data.

Description:

The final change in the send code in TCP in 4.3 was made incorrectly.
In tcp_output (/sys/netinet/tcp_output.c), the output packet flags
are chosen before the packet length is adjusted to reflect the maximum
segment size.  Under some cirsumstances, this results in sending a FIN
with a packet which is not the last data packet.  This is most often
noticed when the connection implements a one-way transfer of data
and the sender closes while the data is still draining.

Fix:

Move the lines in tcp_output that look up the flags to be sent
to a location after the final length adjustment, as follows:

*** /nbsd/sys/netinet/tcp_output.c	Thu Jun  5 00:31:36 1986
--- tcp_output.c	Wed Aug 20 09:31:34 1986
***************
*** 5,7 ****
   *
!  *	@(#)tcp_output.c	7.1 (Berkeley) 6/5/86
   */
--- 5,7 ----
   *
!  *	@(#)tcp_output.c	7.2 (Berkeley) 8/20/86
   */
***************
*** 82,85 ****
  	flags = tcp_outflags[tp->t_state];
- 	if (SEQ_LT(tp->snd_nxt + len, tp->snd_una + so->so_snd.sb_cc))
- 		flags &= ~TH_FIN;
  
--- 82,83 ----
***************
*** 118,119 ****
--- 116,119 ----
  	}
+ 	if (SEQ_LT(tp->snd_nxt + len, tp->snd_una + so->so_snd.sb_cc))
+ 		flags &= ~TH_FIN;
  	win = sbspace(&so->so_rcv);

karels@MONET (Mike Karels) (10/23/86)

Index:	sys/netinet/in_pcb.c 4.3BSD FIX

This is the second in a set of three bug reports with fixes for the
network in 4.3BSD.  All 4.3 sites should install the modifications
described in these reports.

Description:

There is a logic error in the code to assign a local address to a socket
when it is unbound and is setting a destination address.  For a datagram
socket, this may result in the use of a suboptimal return address.
In the most serious case, the return address for a datagram could be
the address used for the loopback interface (if the loopback address
is set before the other interfaces).  The problem can result in failures
of the name resolution routines that use datagrams to communicate with
the name server.

(Note: the "primary" address of a 4.3BSD host is the address set with
the first "ifconfig" to set an internet address after boot time.
The primary address is used when an address must be chosen without context;
this is supposed to happen rarely, for example when no route exists yet
at bind time.  In general, the loopback address should not be the first
address set unless there is no hardware network interface.  Certain network
interfaces do not receive their own packets, however, and either must have
their addresses set by number, or have them set after some other interface.)

The problem is in the code in in_pcbconnect (/sys/netinet/in_pcb.c)
that checks the cached route held by the socket before routing the packet,
then reroutes if necessary.  Part of the interface lookup procedure is done
only if there is no correct cached route.  That part of the procedure
should be done each time a local address is chosen.

Fix:

The fix is to move the interface lookup code outside of the conditional
section which is run only to allocate a new route.  The diffs follow:

*** /nbsd/sys/netinet/in_pcb.c	Thu Jun  5 00:26:13 1986
--- netinet/in_pcb.c	Thu Sep  4 19:33:11 1986
***************
*** 5,7 ****
   *
!  *	@(#)in_pcb.c	7.1 (Berkeley) 6/5/86
   */
--- 5,7 ----
   *
!  *	@(#)in_pcb.c	7.2 (Berkeley) 9/4/86
   */
***************
*** 154,156 ****
  		    (ro->ro_rt == (struct rtentry *)0 ||
! 		    (ifp = ro->ro_rt->rt_ifp) == (struct ifnet *)0)) {
  			/* No route yet, so try to acquire one */
--- 154,156 ----
  		    (ro->ro_rt == (struct rtentry *)0 ||
! 		    ro->ro_rt->rt_ifp == (struct ifnet *)0)) {
  			/* No route yet, so try to acquire one */
***************
*** 160,173 ****
  			rtalloc(ro);
- 			/*
- 			 * If we found a route, use the address
- 			 * corresponding to the outgoing interface
- 			 * unless it is the loopback (in case a route
- 			 * to our address on another net goes to loopback).
- 			 */
- 			if (ro->ro_rt && (ifp = ro->ro_rt->rt_ifp) &&
- 			    (ifp->if_flags & IFF_LOOPBACK) == 0)
- 				for (ia = in_ifaddr; ia; ia = ia->ia_next)
- 					if (ia->ia_ifp == ifp)
- 						break;
  		}
  		if (ia == 0) {
--- 160,173 ----
  			rtalloc(ro);
  		}
+ 		/*
+ 		 * If we found a route, use the address
+ 		 * corresponding to the outgoing interface
+ 		 * unless it is the loopback (in case a route
+ 		 * to our address on another net goes to loopback).
+ 		 */
+ 		if (ro->ro_rt && (ifp = ro->ro_rt->rt_ifp) &&
+ 		    (ifp->if_flags & IFF_LOOPBACK) == 0)
+ 			for (ia = in_ifaddr; ia; ia = ia->ia_next)
+ 				if (ia->ia_ifp == ifp)
+ 					break;
  		if (ia == 0) {

karels@MONET (Mike Karels) (10/23/86)

Index:	sys/h/mbuf.h 4.3BSD FIX

This is the last of a set of 3 bug reports with fixes for the network
in 4.3BSD.  All 4.3 sites should install the modifications described
in these reports.  Although rare, the bug described in this report
may cause unexplained crashes or random failures.

Description:

The macros for addition of page clusters to mbufs were revised in 4.3BSD.
A new macro, MCLGET, is called to add a single page cluster to an mbuf.
If there are no free clusters, the macro calls m_clalloc to attempt
to create a new cluster.  Unfortunately, the MCLGET macro does not run
at high priority, but m_clalloc may only be called from splimp to block
reentrance of the memory allocation routines.  The call to m_clalloc
should be made from MCLALLOC, which does run at high priority.

Fix:

In the file /sys/h/mbuf.h, move the test of mclfree and call to m_clalloc
from the MCLGET macro to the MCLALLOC macro, as shown by the following diffs:

*** mbuf.h.old	Thu Sep 11 06:07:29 1986
--- mbuf.h	Thu Sep 11 06:20:07 1986
***************
*** 5,7 ****
   *
!  *	@(#)mbuf.h	7.1 (Berkeley) 6/4/86
   */
--- 5,7 ----
   *
!  *	@(#)mbuf.h	7.3 (Berkeley) 9/11/86
   */
***************
*** 97,98 ****
--- 97,100 ----
  	{ int ms = splimp(); \
+ 	  if (mclfree == 0) \
+ 		(void)m_clalloc((i), MPG_CLUSTERS, M_DONTWAIT); \
  	  if ((m)=mclfree) \
***************
*** 105,108 ****
  	{ struct mbuf *p; \
- 	  if (mclfree == 0) \
- 		(void)m_clalloc(1, MPG_CLUSTERS, M_DONTWAIT); \
  	  MCLALLOC(p, 1); \
--- 107,108 ----