svn commit: r217589 - head/usr.sbin/syslogd

[Bruce Evans]

On Wed, 19 Jan 2011, David Malone wrote:

> Log:
>  Here v->iov_len has been assigned the return value from snprintf.
>  Checking if it is > 0 doesn't make sense, because snprintf returns
>  how much space is needed if the buffer is too small. Instead, check
>  if the return value was greater than the buffer size, and truncate
>  the message if it was too long.
>
>  It isn't clear if snprintf can return a negative value in the case
>  of an error - I don't believe it can. If it can, then testing
>  v->iov_len won't help 'cos it is a size_t, not an ssize_t.

 	snprintf(buf, 2, "%*s%*s%*s", INT_MAX, "223", INT_MAX, "", 3, "bar");

must return a negative value, since the value that would be written if
all the output fitted in the buffer is not representable in snprintf()'s
return type.  C99 and POSIX have too little to say about this.  In C99,
a negative return value for sprintf() is only mentioned in connection
with an encoding error, and not even that is mentioned for snprintf().
The above examples shows that errors or undefined behaviour must occur
for some parameters, so we can't trust the standard.

Of course, this reason for an error can only happen for exotic formats
that won't be used in practice.  I only use the above to test that
snprintf() handles this error.  As a side effect, this demonstrates the
slowness of snprintf().  It takes about 2 seconds on a 2GHz machine to
do add 1 for each of the ~INT_MAX ~= 0x7FFFFFFF characters that don't
fit in the buffer.

> Modified: head/usr.sbin/syslogd/syslogd.c
> ==============================================================================
> --- head/usr.sbin/syslogd/syslogd.c	Wed Jan 19 17:11:52 2011	(r217588)
> +++ head/usr.sbin/syslogd/syslogd.c	Wed Jan 19 17:17:37 2011	(r217589)
> @@ -1093,8 +1093,9 @@ fprintlog(struct filed *f, int flags, co
> 		v->iov_len = snprintf(greetings, sizeof greetings,
> 		    "\r\n\7Message from syslogd@%s at %.24s ...\r\n",
> 		    f->f_prevhost, f->f_lasttime);

If snprintf() returns a negative value, then overflow on asignment occurs
here.  Since iov_len is an unsigned type, the behaviour is defined.  It
gives the value of (SIZE_MAX + 1) added to the negative value in infinite
precision and then converted to a size_t.  If the negative value is not
too unreasonable, the result is useful.  E.g., if the negative value is
-1, then the result is (SIZE_MAX - 1)...

> -		if (v->iov_len > 0)
> -			v++;
> +		if (v->iov_len >= sizeof greetings)

... (SIZE_MAX - 1) exceeds any reasonable object size, so tests like this
normally work...

> +			v->iov_len = sizeof greetings - 1;
> +		v++;

... but in recalcantrix, size_t is smaller than int, and snprintf is
carefuly to return a value that is small when downcast to size_t.
E.g., size_t might be uint64_t and int int128_t.  snprintf() then
returns INT_MIN = 0x8000....000 in 2's complement so that the result
is 0 when downcast.

So although the above works, it is unportable and hard to understand.
Careful code avoids all overflow and sign extension bug possibilities
by only assigning return values to variables of the same type as the
function.

> 		v->iov_base = nul;
> 		v->iov_len = 0;
> 		v++;
>

Bruce