From owner-freebsd-bugs@FreeBSD.ORG Wed Apr 25 21:00:17 2007 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 5B88416A401 for ; Wed, 25 Apr 2007 21:00:17 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [69.147.83.40]) by mx1.freebsd.org (Postfix) with ESMTP id 335A113C45B for ; Wed, 25 Apr 2007 21:00:17 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id l3PL0H82058268 for ; Wed, 25 Apr 2007 21:00:17 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id l3PL0HxB058261; Wed, 25 Apr 2007 21:00:17 GMT (envelope-from gnats) Date: Wed, 25 Apr 2007 21:00:17 GMT Message-Id: <200704252100.l3PL0HxB058261@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Christoph Weber-Fahr Cc: Subject: Re: misc/112126: netstat segfaults on unusual ICMP statistics X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Christoph Weber-Fahr List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Apr 2007 21:00:17 -0000 The following reply was made to PR bin/112126; it has been noted by GNATS. From: Christoph Weber-Fahr To: Maxim Konovalov , bug-followup@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 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