Date: Mon, 18 May 2020 21:05:38 +0200 From: Michael Tuexen <tuexen@freebsd.org> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r361209 - head/sys/netinet Message-ID: <064C2DCD-6279-4442-A900-0ECCD50CC4FA@freebsd.org> In-Reply-To: <CAG6CVpU3QpCr9WVsuc1eRJrEV3XuQEWGHRCZ9FHMmRmB2N67Mg@mail.gmail.com> References: <202005181007.04IA713t089936@repo.freebsd.org> <CAG6CVpWA1gKLzacDeAFOGTKJcNrg608k5yu8CJg48_WFS0omnA@mail.gmail.com> <E8554788-B6CD-44A9-8ABF-7F3CD129F87F@freebsd.org> <CAG6CVpU3QpCr9WVsuc1eRJrEV3XuQEWGHRCZ9FHMmRmB2N67Mg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 18. May 2020, at 20:23, Conrad Meyer <cem@freebsd.org> wrote:
>=20
> On Mon, May 18, 2020 at 10:35 AM Michael Tuexen <tuexen@freebsd.org> =
wrote:
>>=20
>>> On 18. May 2020, at 17:38, Conrad Meyer <cem@FreeBSD.org> wrote:
>>>=20
>>> 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
>>=20
>> 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.
>>=20
>>> 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?
>>=20
>> If you want, I can revert the change and use the code only on =
non-FreeBSD
>> platforms.
>=20
> Hi Michael,
>=20
> 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] =3D '\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.
>=20
> Re: documentation of snprintf nul-termination, I would look at this
> part of the FreeBSD manual page:
>=20
> 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 =E2=80=98\0=E2=80=99); 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.
>=20
> 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:
>=20
> ERRORS
> In addition to the errors documented for the write(2) system call, =
the
> printf() family of functions may fail if:
>=20
> [EILSEQ] An invalid wide character code was encountered.
>=20
> [ENOMEM] Insufficient storage space is available.
>=20
> [EOVERFLOW] The size argument exceeds INT_MAX + 1, or the =
return
> value would be too large to be represented by =
an int.
>=20
> 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.
>=20
> Re: POSIX definition, POSIX is not the canonical definition of
> snprintf; the C standard is. C (2018) reads:
>=20
>> 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.
>=20
> 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):
>=20
>> The snprintf() function shall fail if: [EOVERFLOW], The value of n is =
greater than INT_MAX.
>=20
> 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
>=20
> 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.
>=20
> Best regards,
> Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?064C2DCD-6279-4442-A900-0ECCD50CC4FA>