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

[Bruce Evans]

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,
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

[David Malone]

Post by Bruce Evans
snprintf(buf, 2, "%*s%*s%*s", INT_MAX, "223", INT_MAX, "", 3, "bar");

Ah - good example. Would it be worth adding some clarification to
our man page? The paragraphs discussing return values suggest that
*printf() functions can only fail because of output or malloc errors.
I guess two other cases are invalid wide characters and the return
value not being representable as an int?

David.