From owner-freebsd-current Tue Nov 24 13:00:25 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id NAA05540 for freebsd-current-outgoing; Tue, 24 Nov 1998 13:00:25 -0800 (PST) (envelope-from owner-freebsd-current@FreeBSD.ORG) Received: from whistle.com (s205m131.whistle.com [207.76.205.131]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id NAA05535 for ; Tue, 24 Nov 1998 13:00:21 -0800 (PST) (envelope-from archie@whistle.com) Received: (from smap@localhost) by whistle.com (8.7.5/8.6.12) id MAA16650; Tue, 24 Nov 1998 12:59:11 -0800 (PST) Received: from bubba.whistle.com( 207.76.205.7) by whistle.com via smap (V2.0) id xma016648; Tue, 24 Nov 98 12:59:10 -0800 Received: (from archie@localhost) by bubba.whistle.com (8.8.7/8.6.12) id MAA05816; Tue, 24 Nov 1998 12:59:09 -0800 (PST) From: Archie Cobbs Message-Id: <199811242059.MAA05816@bubba.whistle.com> Subject: Re: snprintf() in the kernel In-Reply-To: <199811241035.VAA30568@godzilla.zeta.org.au> from Bruce Evans at "Nov 24, 98 09:35:27 pm" To: bde@zeta.org.au (Bruce Evans) Date: Tue, 24 Nov 1998 12:59:09 -0800 (PST) Cc: dillon@apollo.backplane.com, freebsd-current@FreeBSD.ORG, grog@lemis.com, rnordier@nordier.com X-Mailer: ELM [version 2.4ME+ PL38 (25)] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Bruce Evans writes: > >For example, if a function takes a string argument, when can you > >assume an upper bound on how long the string is? > > strncpy() already takes a length arg. Changing from strncpy() to > snprintf() just wastes space and time unless the value returned by > snprintf() is actually checked and cases where the string doesn't > fit are actually handled. Except when there needs to be a trailing NUL byte and the original code doesn't manually put one in the last buffer byte. For example, devstat_add_entry() and ibcs2_utssys() (just noticed a new bug.. potential crash in index() if your hostname is "longhostname"); > >Of the strncpy() replacements, there are three categories: > > > >- "Possibly unterminated string", where one is required: > > > > netatm/spans/spans_print.c > > It's interesting that it doesn't just printf everything (using separate > printfs because spans-addr_print() returns a pointer to static storage). > Most of the strncpy's seem to be to copy the static storage to local > storage. All buffers for this seem to have size 80, so there are no > truncation problems. Yes, the code is technically correct, but figuring this out requires reverse engineering a bunch of function calls, etc. before you can conclude that there's no actual bug there. Why force a future programmer to have to rediscover that the buffers won't overflow? Why not make it obvious? This is not a bug fix but rather a maintainability fix, as are most of these.. > > netatm/uni/uniarp_cache.c > > pc98/pc98/diskslice_machdep.c > > pci/pci_compat.c > > > >- "Simplification" (eg, replacing constants like "16" with sizeof()) > > with otherwise no functional effect (including strncpy()'s zero'ing > > out of the buffer): > > > > i386/ibcs2/ibcs2_stat.c > > i386/ibcs2/ibcs2_xenix.c > > i386/linux/linux_misc.c > > These depend on a previous bzero() to zero out the buffer (and any padding > in the struct). The strncpy()'s of course should have used sizeof() instead > of literal constants. The explicit \0 termiation could have been avoided > in ibcs2_stat.c by subtracting 1 from the sizes. This is already done in > ibcs2_xenix.c and linux_misc.c, but it is hard to see because sizeof() is > not used and the subtraction is done at code-write time. Again, the code is technically correct but less than pleasant to look at and requires analysis and cross-checking to conclude that there's no actual bug.. yuck. Revision #3 is now ready :-) Thanks, -Archie ___________________________________________________________________________ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message