Date: Wed, 25 Apr 2007 21:00:17 GMT From: Christoph Weber-Fahr <cwf-ml@arcor.de> To: freebsd-bugs@FreeBSD.org Subject: Re: misc/112126: netstat segfaults on unusual ICMP statistics Message-ID: <200704252100.l3PL0HxB058261@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/112126; it has been noted by GNATS. From: Christoph Weber-Fahr <cwf-ml@arcor.de> To: Maxim Konovalov <maxim@macomnet.ru>, 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 <maxim@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200704252100.l3PL0HxB058261>