Date: Mon, 18 May 2020 22:43:27 +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: <ED7C6ECA-23E4-43D9-B08D-2A39027FC210@freebsd.org> In-Reply-To: <CAG6CVpVrJE7hnEeGxzeBMUEWsi_N2EtGXdH8ZbZbEQ0GtnDovw@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> <064C2DCD-6279-4442-A900-0ECCD50CC4FA@freebsd.org> <CAG6CVpVrJE7hnEeGxzeBMUEWsi_N2EtGXdH8ZbZbEQ0GtnDovw@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
> On 18. May 2020, at 22:17, Conrad Meyer <cem@FreeBSD.org> wrote:
>
> Hi Michael,
>
> On Mon, May 18, 2020 at 12:05 PM Michael Tuexen <tuexen@freebsd.org> wrote:
>>
>>> On 18. May 2020, at 20:23, Conrad Meyer <cem@freebsd.org> wrote:
>>
>>> 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.
>
> Sure. You can certainly ignore user reports corresponding to bogus
> flags, though, and encourage use of various flags.
I could, but decided to improve the situation for some people, but
wasn't realising that I made it worse for others. Sorry about that.
>
>> 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
>
> This seems like a messy solution. I'd suggest either putting
> unconditional "msg[0] = '\0';" before snprintf() invocations, or
That would assume that in case of an error the first byte is overwitten.
> defining an snprintf wrapper function for non-FreeBSD platforms and
> using it universally.
Yeah, one can use a Macro SCTP_SNPRINTF(). Let me see...
>
>> 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.
>
> Sure, that's a good concern. That said,
>
> Glibc: https://github.com/bminor/glibc/blob/5f72f9800b250410cad3abfeeb09469ef12b2438/libio/vsnprintf.c#L93-L114
> (always nul terminates)
> MS: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=vs-2019
> ("The snprintf function always stores a terminating null character…")
> OpenBSD: https://github.com/openbsd/src/blob/master/lib/libc/stdio/vsnprintf.c#L60-L63
> (always nul terminates)
> NetBSD: https://github.com/NetBSD/src/blob/trunk/lib/libc/stdio/vsnprintf.c#L97-L101
> (always nul terminates)
> Linux (kernel):
> https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L2645
> (always nul terminates)
>
> None of these are conditional on error status.
>
> The only exception I found is musl libc, and in that it is a case you
> cannot encounter here (size > INT_MAX). Arguably this is a bug in
> musl libc. I did not dive deeper into musl and determine whether
> other errors were nul terminated or not.
>
> Conrad
>
> P.S., It seems dubious to be sending diagnostic formatted error
> messages out across the network.
It was and still is very helpful when debuging interop problems if you only have access
to a tracefile and can't change the running code. Like people asking you why is your
implementation sending back an ABORT when it sees this packet.
Best regards
Michael
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?ED7C6ECA-23E4-43D9-B08D-2A39027FC210>