misc/112126: netstat segfaults on unusual ICMP statistics

[Maxim Konovalov]

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

From: Maxim Konovalov <maxim at macomnet.ru>
To: Christoph Weber-Fahr <cwf-ml at arcor.de>
Cc: bug-followup at freebsd.org
Subject: Re: misc/112126: netstat segfaults on unusual ICMP statistics
Date: Fri, 27 Apr 2007 21:37:17 +0400 (MSD)

 On Fri, 27 Apr 2007, 16:35+0200, Christoph Weber-Fahr wrote:
 
 > Hello,
 >
 > Maxim Konovalov <maxim at macomnet.ru> wrote:
 > > On Fri, 27 Apr 2007, 03:50+0200, Christoph Weber-Fahr wrote:
 > > > Input histogram:
 > > > echo reply: 12
 > > > destination unreachable: 1
 > > > echo: 41
 > > > #20: 7
 > > > icmp traceroute: 16
 > > > mobile registration req: 25
 > > > #37: 31
 > > > 8
 > > > 15
 > > >
 > > > Note the last two untagged values. They
 > > > are created when the kernel, which in
 > > > the meantime has a ICM_MAXTYPE at, say, 49,
 > > > has logged 8 packets of type 44, and 15 of type 47.
 >
 > > icmpstat.icps_outhist and icps_inhist are definde this way:
 > >
 > > u_long icps_outhist[ICMP_MAXTYPE + 1];
 > > u_long icps_inhist[ICMP_MAXTYPE + 1];
 > >
 > > How do you fit types > ICMP_MAXTYPE + 1 there?
 >
 > Not at all. We are debating the case when ICMP_MAXTYPE
 > in the kernel gets raised without adapting netstat.
 >
 > To test this you would not only have had modified ping, but also
 > recompiled a kernel with a modified ICMP_MAXTYPE.
 >
 > My scenario above assumed ICMP_MAXTYPE to be 49.
 
 OK, what about this version:
 
 Index: inet.c
 ===================================================================
 RCS file: /home/ncvs/src/usr.bin/netstat/inet.c,v
 retrieving revision 1.74
 diff -u -p -r1.74 inet.c
 --- inet.c	26 Feb 2007 22:25:21 -0000	1.74
 +++ inet.c	27 Apr 2007 17:32:01 -0000
 @@ -636,26 +636,48 @@ ip_stats(u_long off __unused, const char
  #undef p1a
  }
 
 -static	const char *icmpnames[] = {
 -	"echo reply",
 +static	const char *icmpnames[ICMP_MAXTYPE + 1] = {
 +	"echo reply",			/* RFC 792 */
  	"#1",
  	"#2",
 -	"destination unreachable",
 -	"source quench",
 -	"routing redirect",
 +	"destination unreachable",	/* RFC 792 */
 +	"source quench",		/* RFC 792 */
 +	"routing redirect",		/* RFC 792 */
  	"#6",
  	"#7",
 -	"echo",
 -	"router advertisement",
 -	"router solicitation",
 -	"time exceeded",
 -	"parameter problem",
 -	"time stamp",
 -	"time stamp reply",
 -	"information request",
 -	"information request reply",
 -	"address mask request",
 -	"address mask reply",
 +	"echo",				/* RFC 792 */
 +	"router advertisement",		/* RFC 1256 */
 +	"router solicitation",		/* RFC 1256 */
 +	"time exceeded",		/* RFC 792 */
 +	"parameter problem",		/* RFC 792 */
 +	"time stamp",			/* RFC 792 */
 +	"time stamp reply",		/* RFC 792 */
 +	"information request",		/* RFC 792 */
 +	"information request reply",	/* RFC 792 */
 +	"address mask request",		/* RFC 950 */
 +	"address mask reply",		/* RFC 950 */
 +	"#19",
 +	"#20",
 +	"#21",
 +	"#22",
 +	"#23",
 +	"#24",
 +	"#25",
 +	"#26",
 +	"#27",
 +	"#28",
 +	"#29",
 +	"icmp traceroute",		/* RFC 1393 */
 +	"datagram conversion error",	/* RFC 1475 */
 +	"mobile host redirect",
 +	"IPv6 where-are-you",
 +	"IPv6 i-am-here",
 +	"mobile registration req",
 +	"mobile registration reply",
 +	"domain name request",		/* RFC 1788 */
 +	"domain name reply",		/* RFC 1788 */
 +	"icmp SKIP",
 +	"icmp photuris",		/* RFC 2521 */
  };
 
  /*
 @@ -701,8 +723,12 @@ icmp_stats(u_long off __unused, const ch
  				printf("\tOutput histogram:\n");
  				first = 0;
  			}
 -			printf("\t\t%s: %lu\n", icmpnames[i],
 -				icmpstat.icps_outhist[i]);
 +			if (icmpnames[i] != NULL)
 +				printf("\t\t%s: %lu\n", icmpnames[i],
 +					icmpstat.icps_outhist[i]);
 +			else
 +				printf("\t\tunknown ICMP #%d: %lu\n", i,
 +					icmpstat.icps_outhist[i]);
  		}
  	p(icps_badcode, "\t%lu message%s with bad code fields\n");
  	p(icps_tooshort, "\t%lu message%s < minimum length\n");
 @@ -716,8 +742,12 @@ icmp_stats(u_long off __unused, const ch
  				printf("\tInput histogram:\n");
  				first = 0;
  			}
 -			printf("\t\t%s: %lu\n", icmpnames[i],
 -				icmpstat.icps_inhist[i]);
 +			if (icmpnames[i] != NULL)
 +				printf("\t\t%s: %lu\n", icmpnames[i],
 +			 		icmpstat.icps_inhist[i]);
 +			else
 +				printf("\t\tunknown ICMP #%d: %lu\n", i,
 +					icmpstat.icps_inhist[i]);
  		}
  	p(icps_reflect, "\t%lu message response%s generated\n");
  	p2(icps_badaddr, "\t%lu invalid return address%s\n");
 %%%
 
 I see:
 
                 #28: 1
                 #29: 1
                 icmp traceroute: 1
                 mobile registration req: 1
                 mobile registration reply: 1
                 domain name request: 1
                 domain name reply: 1
                 icmp SKIP: 1
                 icmp photuris: 1
                 unknown ICMP #41: 1
                 unknown ICMP #42: 1
                 unknown ICMP #43: 1
 
 -- 
 Maxim Konovalov