svn commit: r361209 - head/sys/netinet

Michael Tuexen tuexen at freebsd.org
Mon May 18 19:05:43 UTC 2020


> On 18. May 2020, at 20:23, Conrad Meyer <cem at freebsd.org> wrote:
> 
> On Mon, May 18, 2020 at 10:35 AM Michael Tuexen <tuexen at freebsd.org> wrote:
>> 
>>> On 18. May 2020, at 17:38, Conrad Meyer <cem at FreeBSD.org> wrote:
>>> 
>>> These changes are a bit odd.  The only reason a standards-compliant
>>> snprintf() would fail to nul-terminate a buffer is if the provided
>>> buffer had length zero.  Since this is not the case in any of these
>>> uses, I wonder why this revision was made?  Does a SCTP downstream
>> 
>> when compiling the code in userland with gcc 10, it warns that
>> the output might be truncated. That is true and intended.
>> So checking that the call doesn't fail silences this warning and
>> ensures the code works in case snprintf() returns an error. I don't
>> see in the POSIX specification a statement limiting the case where
>> it could fail.
>> 
>>> have a broken snprintf implementation, and if so, wouldn't it make
>>> more sense to create a standards-compliant portability shim for that
>>> platform instead of this more invasive change?
>> 
>> If you want, I can revert the change and use the code only on non-FreeBSD
>> platforms.
> 
> Hi Michael,
> 
> If truncation is intended, the GCC warning is spurious.  Given how
> often snprintf is used in this way, I wonder if it would make sense to
> just disable it across the entire tree.  Regardless, IMO it makes
The issue wasn't reported against the kernel code, but running the code
in userland. I don't really control the flags people are using.
> sense to disable the warning, rather than make these changes to check
> for errors that can't happen.  It does not even "fix" the thing GCC is
OK. I'll revert this change and replace it upstream by something like

#if defined(__FreeBSD_)
	snprintf(msg, sizeof(msg), "Never sent serial number %8.8x", serial_num)
#else
	if (snprintf(msg, sizeof(msg), "Never sent serial number %8.8x", serial_num) < 0) {
		msg[0] = '\0';
	}
#endif

I don't know if other platforms guarantee that snprintf() can't fail.
If it fails, the stack would send out un-initialized stack memory on
the network.
> warning about, since we aren't testing for truncation at all; it's
> just a warning defeat mechanism.  -Wno- is a better warning-defeat
> mechanism.
> 
> Re: documentation of snprintf nul-termination, I would look at this
> part of the FreeBSD manual page:
> 
>     The snprintf() and vsnprintf() functions will write at most size-1 of the
>     characters printed into the output string (the size'th character then
>     gets the terminating ‘\0’); if the return value is greater than or equal
>     to the size argument, the string was too short and some of the printed
>     characters were discarded.  The output is always null-terminated, unless
>     size is 0.
> 
> Note the last sentence especially.  As far as error conditions, those
> are canonically documented in the ERRORS section of the manual page
> rather than RETURN VALUES.  For whatever reason, mdoc(7) standard puts
> EXAMPLES between the two sections, and additionally snprintf.3 has a
> non-standard COMPATIBILITY section between the two, so they are not
> directly adjacent.  Here's that section, though:
> 
> ERRORS
>     In addition to the errors documented for the write(2) system call, the
>     printf() family of functions may fail if:
> 
>     [EILSEQ]           An invalid wide character code was encountered.
> 
>     [ENOMEM]           Insufficient storage space is available.
> 
>     [EOVERFLOW]        The size argument exceeds INT_MAX + 1, or the return
>                        value would be too large to be represented by an int.
> 
> The section is unfortunately generalized and non-specific; snprintf
> probably cannot fail with ENOMEM, for example, nor write(2) errors.
> But EOVERFLOW is well-documented.
> 
> Re: POSIX definition, POSIX is not the canonical definition of
> snprintf; the C standard is.  C (2018) reads:
> 
>> If n is zero, nothing shall be written and s may be a null pointer. Otherwise, output bytes beyond the n-1st shall be discarded instead of being written to the array, and a null byte is written at the end of the bytes actually written into the array.
> 
> Emphasis on the last clause.  (POSIX uses the exact same language.)
> As far as conditions where snprintf may fail, POSIX only defines a
> single case (covered in snprintf.3 above):
> 
>> The snprintf() function shall fail if: [EOVERFLOW], The value of n is greater than INT_MAX.
> 
> That is not the case in any of these invocations.
Just to be clear: My problem is NOT that the output is not zero terminated.
I use snprintf() in a way that it is, if it does not fail.

I was just adding protection code for the case it fails and leaves the
buffer uninitialized. since I don't want so sent out uninitialized stack memory.

I learnt that on FreeBSD this is not a problem and I'll remove that protection
code for this platform.

Best regards
Michael
> 
> Probably snprintf(9) should be specifically documented; printf(9) does
> not cover it yet.  This is a documentation gap.  Additionally, the
> COMPATIBILITY section of snprintf.3 should probably be moved to
> STANDARDS (to help move ERRORS and RETURN VALUES closer together).
> Finally, it might be nice to have kernel snprintf(9) _Static_assert
> that the provided length is shorter than INT_MAX (when it is a
> compiler constant, and detect non-constant cases at runtime).
> Currently, snprintf(9) fails to detect buffers that would produce a
> result which overflows.
> 
> Best regards,
> Conrad



More information about the svn-src-head mailing list