Date: Thu, 19 Jul 2001 13:54:23 +0300 From: Ruslan Ermilov <ru@FreeBSD.org> To: Bruce Evans <bde@zeta.org.au>, Kris Kennaway <kris@obsecurity.org> Cc: "Daniel C. Sobral" <dcs@FreeBSD.org>, Bill Fenner <fenner@FreeBSD.org>, net@FreeBSD.org Subject: Re: cvs commit: src/sys/netinet ip_output.c Message-ID: <20010719135423.G69276@sunbay.com> In-Reply-To: <Pine.BSF.4.21.0107191733360.778-100000@besplex.bde.org>; from bde@zeta.org.au on Thu, Jul 19, 2001 at 05:46:38PM %2B1000 References: <200107190710.f6J7AVl44738@freefall.freebsd.org> <20010719011806.A28830@xor.obsecurity.org> <200107190710.f6J7AVl44738@freefall.freebsd.org> <Pine.BSF.4.21.0107191733360.778-100000@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 19, 2001 at 05:46:38PM +1000, Bruce Evans wrote:
> > Modified files:
> > sys/netinet ip_output.c
> > Log:
> > Backout non-functional changes from revision 1.128.
> >
> > Not objected to by: dcs
>
> This reduces the bugs in rev.1.128 to style bugs on every line, and:
>
> ./@/netinet/ip_output.c: In function `ip_output':
> ./@/netinet/ip_output.c:127: warning: `ifp' might be used uninitialized in this function
> ./@/netinet/ip_output.c:132: warning: `ia' might be used uninitialized in this function
> ./@/netinet/ip_output.c:133: warning: `isbroadcast' might be used uninitialized in this function
>
> The warning is because the variables were initialized only in each arm of an
> if-else clause, and the commit adds an elseif which doesn't do anything.
> Are you going to fix this too? I don't really understand this.
>
You are attacking the wrong person here. Daniel did the actual commit.
In fact, warnings about `ifp' and `isbroadcast' were mostly harmless.
gcc(1) just can't realize that these variables are either unused or
really get initialized in the code path that is affected by the
following conditional (let's call it "multicast destination" block),
due to (probably) using goto's:
if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr))) {
...
}
This block then jumps to either `sendit' or `done' labels. `ifp' is
properly initialized within the block, and `isbroadcast' wasn't used
below in `sendit' and `done'. The warning for `ifp' is easily fixable.
Another story is with `ia'. `ia' is used later to update per-address
statistics counters:
: /* Record statistics for this interface address. */
: if (!(flags & IP_FORWARDING)) {
: ia->ia_ifa.if_opackets++;
: ia->ia_ifa.if_obytes += m->m_pkthdr.len;
: }
(And yet in one place below, for "fragmentation" case.)
The following patch:
1) Fixes the `ifp' warning.
2) Doesn't fix the `isbroadcast' (harmless) warning. ???
3) Tries to fix the `ia' warning, but:
What happens if the interface in question (imo->imo_multicast_ifp)
doesn't have any INET addresses bound to it? IFP_TO_IA(ifp, ia)
will set `ia' to 0, and the statistics code will later panic.
It's unclear to me at all whether this should be allowed (multicasting
over an interface with no addresses), and if yes, then the statistics
code should be modified to check whether `ia' is zero or not (I've put
this in the patch). But there's still race possible in the "multicast
destination" block, when it initializes the unspecified (INADDR_ANY)
source address of a datagram using the address of an interface.
NetBSD (see below) should panic here.
Index: ip_output.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.130
diff -u -p -r1.130 ip_output.c
--- ip_output.c 2001/07/19 07:10:30 1.130
+++ ip_output.c 2001/07/19 10:49:09
@@ -253,14 +253,13 @@ ip_output(m0, opt, ro, flags, imo)
ip->ip_ttl = 1;
isbroadcast = in_broadcast(dst->sin_addr, ifp);
} else if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) &&
- (imo != NULL) &&
- (imo->imo_multicast_ifp != NULL)) {
+ imo != NULL && imo->imo_multicast_ifp != NULL) {
/*
- * bypass the normal routing lookup for
- * multicast packets if the interface is
- * specified
+ * Bypass the normal routing lookup for multicast
+ * packets if the interface is specified.
*/
- /* No Operation */
+ ifp = imo->imo_multicast_ifp;
+ IFP_TO_IA(ifp, ia);
} else {
/*
* If this is the case, we probably don't want to allocate
@@ -303,8 +302,6 @@ ip_output(m0, opt, ro, flags, imo)
*/
if (imo != NULL) {
ip->ip_ttl = imo->imo_multicast_ttl;
- if (imo->imo_multicast_ifp != NULL)
- ifp = imo->imo_multicast_ifp;
if (imo->imo_multicast_vif != -1)
ip->ip_src.s_addr =
ip_mcast_src(imo->imo_multicast_vif);
@@ -325,13 +322,9 @@ ip_output(m0, opt, ro, flags, imo)
* of outgoing interface.
*/
if (ip->ip_src.s_addr == INADDR_ANY) {
- register struct in_ifaddr *ia1;
-
- TAILQ_FOREACH(ia1, &in_ifaddrhead, ia_link)
- if (ia1->ia_ifp == ifp) {
- ip->ip_src = IA_SIN(ia1)->sin_addr;
- break;
- }
+ /* XXX: interface may have no addresses. */
+ if (ia != NULL)
+ ip->ip_src = IA_SIN(ia)->sin_addr;
}
IN_LOOKUP_MULTI(ip->ip_dst, ifp, inm);
@@ -824,7 +817,7 @@ pass:
}
/* Record statistics for this interface address. */
- if (!(flags & IP_FORWARDING)) {
+ if (!(flags & IP_FORWARDING) && ia) {
ia->ia_ifa.if_opackets++;
ia->ia_ifa.if_obytes += m->m_pkthdr.len;
}
@@ -964,7 +957,7 @@ sendorfree:
/* clean ipsec history once it goes out of the node */
ipsec_delaux(m);
#endif
- if (error == 0) {
+ if (error == 0 && ia) {
/* Record statistics for this interface address. */
ia->ia_ifa.if_opackets++;
ia->ia_ifa.if_obytes += m->m_pkthdr.len;
Finally, I have looked at NetBSD code, and they seem to:
1) look the route to the destination, as we did it before.
2) check that `ia' is non-zero in the per-address statistics code.
3) not check that `ia' is non-zero in the "multicast destination"
block, in the following code snippet:
: /*
: * See if the caller provided any multicast options
: */
: if (imo != NULL) {
: ip->ip_ttl = imo->imo_multicast_ttl;
: if (imo->imo_multicast_ifp != NULL) {
: ifp = imo->imo_multicast_ifp;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
: mtu = ifp->if_mtu;
: }
: } else
: ip->ip_ttl = IP_DEFAULT_MULTICAST_TTL;
[...]
: /*
: * If source address not specified yet, use an address
: * of outgoing interface.
: */
: if (in_nullhost(ip->ip_src)) {
: struct in_ifaddr *ia;
: IFP_TO_IA(ifp, ia);
: ip->ip_src = ia->ia_addr.sin_addr;
: }
There's a race in the last two lines if `ifp' doesn't have addresses
bound.
Sorry, I'm not too familiar with the multicast code, so probably
some MCAST geeks like Bill Fenner could speak up? :-)
Cheers,
--
Ruslan Ermilov Oracle Developer/DBA,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010719135423.G69276>