Skip site navigation (1)Skip section navigation (2)
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>