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: Wed, 25 Apr 2007 22:45:35 +0400 (MSD)

 Hi Christoph,
 
 On Wed, 25 Apr 2007, 15:29-0000, Christoph Weber-Fahr wrote:
 
 > >Description:
 > we have a number of very busy publicly available production servers
 > running FreeBSD.
 >
 > On some of these systems, netstat -s fails with a segmentation
 > fault.  This is especially problematic, since system monitoring
 > tools use netstat periodically to gather statistics.
 >
 > I looked into the issue in the netstats sources. The netstat utility
 > mainatins an internal array for ICMP type names. But for listing the
 > icmp statistics it relies on the ICMP_MAXTYPE kernel define pulled
 > from the syestem include files.
 >
 > Those values are badly out of sync - ICMP_MAXTYPE is at 40, while
 > netstat's name array has just 19 entries. So, as soon as the kernel
 > has logged a stat for an icmp type > 19, netstat -s dumps core.
 >
 
 [...]
 > the appended patch
 >
 > - enhances netstat's array to 40 with names from the include file
 > - provides a mechanism to deal gracefully with such a situation
 >   schould it arise in the future again.
 >
 > The patch was created and tested on FreeBSD 6.1-p15. It was also
 > tested on FreeBSD 6.2-p3.
 >
 > Patch attached with submission follows:
 >
 > --- usr.bin/netstat/inet.c.org	Wed Apr 25 15:44:51 2007
 > +++ usr.bin/netstat/inet.c	Wed Apr 25 17:19:17 2007
 > @@ -653,8 +653,32 @@
 >  	"information request reply",
 >  	"address mask request",
 >  	"address mask reply",
 > +	"#19",
 > +	"#20",
 > +	"#21",
 > +	"#22",
 > +	"#23",
 > +	"#24",
 > +	"#25",
 > +	"#26",
 > +	"#27",
 > +	"#28",
 > +	"#29",
 > +	"icmp traceroute",
 > +	"data conversion error",
 > +	"mobile host redirect",
 > +	"IPv6 where-are-you",
 > +	"IPv6 i-am-here",
 > +	"mobile registration req",
 > +	"mobile registration reply",
 > +	"#37",
 > +	"#38",
 > +	"icmp SKIP",
 > +	"icmp photuris",
 >  };
 >
 > +static const int max_known_icmpname=40;
 > +
 >
 >  /*
 >   * Dump ICMP statistics.
 >   */
 > @@ -698,8 +722,13 @@
 >  				printf("\tOutput histogram:\n");
 >  				first = 0;
 >  			}
 > -			printf("\t\t%s: %lu\n", icmpnames[i],
 > -				icmpstat.icps_outhist[i]);
 > +			if (i <= max_known_icmpname) {
 > +				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");
 > @@ -713,8 +742,13 @@
 >  				printf("\tInput histogram:\n");
 >  				first = 0;
 >  			}
 > -			printf("\t\t%s: %lu\n", icmpnames[i],
 > -				icmpstat.icps_inhist[i]);
 > +			if (i <= max_known_icmpname) {
 > +				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");
 
 It's not clear why do you need to check i <= max_known_icmpname when
 the upper loop already checked that:
 
 for (first = 1, i = 0; i < ICMP_MAXTYPE + 1; i++).
 
 Can I suggest an alternative patch:
 
 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	25 Apr 2007 18:32:13 -0000
 @@ -636,7 +636,7 @@ ip_stats(u_long off __unused, const char
  #undef p1a
  }
 
 -static	const char *icmpnames[] = {
 +static	const char *icmpnames[ICMP_MAXTYPE + 1] = {
  	"echo reply",
  	"#1",
  	"#2",
 @@ -656,6 +656,28 @@ static	const char *icmpnames[] = {
  	"information request reply",
  	"address mask request",
  	"address mask reply",
 +	"#19",
 +	"#20",
 +	"#21",
 +	"#22",
 +	"#23",
 +	"#24",
 +	"#25",
 +	"#26",
 +	"#27",
 +	"#28",
 +	"#29",
 +	"icmp traceroute",
 +	"data conversion error",
 +	"mobile host redirect",
 +	"IPv6 where-are-you",
 +	"IPv6 i-am-here",
 +	"mobile registration req",
 +	"mobile registration reply",
 +	"domain name request",
 +	"domain name reply",
 +	"icmp SKIP",
 +	"icmp photuris",
  };
 
  /*
 %%%
 
 -- 
 Maxim Konovalov