misc/112126: netstat segfaults on unusual ICMP statistics

[Christoph Weber-Fahr]

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

From: Christoph Weber-Fahr <cwf-ml at arcor.de>
To: Maxim Konovalov <maxim at macomnet.ru>, bug-followup at freebsd.org
Cc:  
Subject: Re: misc/112126: netstat segfaults on unusual ICMP statistics
Date: Wed, 25 Apr 2007 22:37:37 +0200

 Hello,
 
 Maxim Konovalov <maxim at macomnet.ru> wrote:
 
 > 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++).
 
 No!
 
 It does precise ly not do that - that upper loop is in place
 in the current (2005) version, and it eveidently did not prevent
 this problem fromn happeneing.
 
 Because that upper loop checks ICMP_MAXTYPES, which comes
 from the kernel includes, while the second check explicitly checks
 the Index size of the local const array, which is defined in the
 same file directly below it!
 
 (There might be more beautiful ways to do that in c, but as
 I'm not a c guru, none of these came in handy. You could think of
 stuff like (sizeof (icmpnames) / sizeof (icmpnames[1])), but
 I have not much of a clue about either feasibility or portability
 of these - I program C too rarely to know that without major
 reading.  Also, putting abstract knowledge about data structures
 into code isn't a programming style I like - that's a compiler's
 business, and who knows what a compiler does next year.
 
 ( and I'm not even sure that abovementioned expression works
 at all - aren't arrays in C just a convenient way to describe a
 little fancy pointer arithmetic, and the sizeof(arry) is
 actually always a pointer, too ?)
 
 > Can I suggest an alternative patch:
 
 Very funny. That's where I started - but I consider it only
 the first half of a solution. Fixing a problem isn't all,
 you should also strive to prevent its repetition.
 
 Your "fix" will break the moment someone again raises
 ICMP_MAXTYPE without changing netstat/inet.c, too.
 
 If you need a utility to rely on kernel interfaces, have
 it import them via includes.  Defining them locally, and
 crashing when the two definitions go out of sync is just
 bad coding style.
 
 Regards
 
 Christoph Weber-Fahr