Date: Mon, 18 May 2020 11:23:17 -0700 From: Conrad Meyer <cem@freebsd.org> To: Michael Tuexen <tuexen@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: <CAG6CVpU3QpCr9WVsuc1eRJrEV3XuQEWGHRCZ9FHMmRmB2N67Mg@mail.gmail.com> In-Reply-To: <E8554788-B6CD-44A9-8ABF-7F3CD129F87F@freebsd.org> References: <202005181007.04IA713t089936@repo.freebsd.org> <CAG6CVpWA1gKLzacDeAFOGTKJcNrg608k5yu8CJg48_WFS0omnA@mail.gmail.com> <E8554788-B6CD-44A9-8ABF-7F3CD129F87F@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, May 18, 2020 at 10:35 AM Michael Tuexen <tuexen@freebsd.org> wrote:
>
> > On 18. May 2020, at 17:38, Conrad Meyer <cem@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
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
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 =E2=80=98\0=E2=80=99); if the return value is gre=
ater 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, unle=
ss
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 retur=
n
value would be too large to be represented by an in=
t.
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. Other=
wise, output bytes beyond the n-1st shall be discarded instead of being wri=
tten to the array, and a null byte is written at the end of the bytes actua=
lly 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 gre=
ater than INT_MAX.
That is not the case in any of these invocations.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpU3QpCr9WVsuc1eRJrEV3XuQEWGHRCZ9FHMmRmB2N67Mg>