Date: Tue, 24 Nov 1998 12:59:09 -0800 (PST) From: Archie Cobbs <archie@whistle.com> To: bde@zeta.org.au (Bruce Evans) Cc: dillon@apollo.backplane.com, freebsd-current@FreeBSD.ORG, grog@lemis.com, rnordier@nordier.com Subject: Re: snprintf() in the kernel Message-ID: <199811242059.MAA05816@bubba.whistle.com> In-Reply-To: <199811241035.VAA30568@godzilla.zeta.org.au> from Bruce Evans at "Nov 24, 98 09:35:27 pm"
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199811242059.MAA05816>