Date: Wed, 30 Aug 1995 07:21:24 +1000 From: Bruce Evans <bde@zeta.org.au> To: guido@spooky.lss.cp.philips.com, pst@freefall.FreeBSD.org Cc: security@freefall.FreeBSD.org Subject: Re: please code review proposed fix for syslog problem Message-ID: <199508292121.HAA01499@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>One question: what if SPACELEFT(X) is smaller then zero?
>snprintf assumes size_t which is unsigned....Should this be guarded
>against? (I always mix up signed unsigned operations and am
>to lazy to look it up right now)
Yes, it should. However, all of the SPACELEFT()s except the one
in the loop for copying the format are guaranteed to be large and
positive unless LogTag is unreasonably long. The problems only start
occurring when user-supplied data is handled.
>> if (LogTag != NULL) {
>> *p++ = ':';
>> *p++ = ' ';
> ^^^ can overwrite outside of the buffer as well.XXX
Not a big problem, because the user data hasn't been handled yet.
>> for (t = fmt_cpy; ch = *fmt; ++fmt)
>> if (ch == '%' && fmt[1] == 'm') {
>> ++fmt;
>> ! t += snprintf(t, sizeof(fmt_cpy) - (t - fmt_cpy), "%s",
>> ! strerror(saved_errno));
Another bug: snprintf() returns the number of chars that would be printed
if the fitted, so t may end up after the end of the array. This causes
pedantically undefined behaviour. In practice, (t - fmt_copy) will
probably be > sizeof(fmt_cpy), so the space will be "negative" (actually
about SIZE_T_MAX).
Robust library code sure is hard to write. I prefer kernel code :-).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199508292121.HAA01499>