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