Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 23:09, Ian Lepore wrote: > > On Mon, 2020-05-18 at 23:01 +0200, Michael Tuexen wrote: >>> On 18. May 2020, at 22:48, Ian Lepore wrote: >>> >>> On Mon, 2020-05-18 at 22:43 +0200, Michael Tuexen wrote: > 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. >>> >>> I'm trying to figure out why your original commit was a problem. I >>> understand why it was questioned, but once the answer came out, >>> it's >>> clear that the code you originally committed does what it's >>> supposed to >>> without any harmful side effects. Sure, freebsd doesn't strictly >>> need >> >> I guess the point Conrad is making, that on FreeBSD the check is not >> needed, since the call can not fail. So the FreeBSD code base would >> not >> be consistent: within the SCTP related code the return code is >> checked, >> in the other code it is not. >>> it, but the code is shared among projects, so what's the harm in >>> the >>> extra check that helps other projects sharing the code? It's >>> certainly >>> a lot less confusion and code clutter than any of the "remedies" >>> that >>> have been discussed. >> >> Yepp, sharing code between platforms makes things harder. Running the >> same >> code in kernel land and userland does not make it simpler. Different >> groups >> have different opinions/styles/... >> >> I'll revert the commit tomorrow and a variadic macros >> SCTP_SNPRINTF(), which >> will map on FreeBSD to snprintf() and on the other platforms will do >> the check. >> >> If the build problem comes up on FreeBSD userland (and I have no idea >> if that >> is the case, since I don't know how Firefox / Chrome are build on >> FreeBSD), >> I leave it up to the port maintainer of the application to deal with >> it. >> >> Best regards >> Michael >>> >>> -- Ian >>> >> >> > > Well it seems to me you're being asked to do a lot of extra work that > has the final result of making the code LESS clear and MORE complex, > because of one person's opinion. I'm actually a bit tempted to Yes, it is one person. But it is one person who thinks the change is bad enough that he needs to speak up. So I think this has to be addressed. > complain about the change, because to me it reduces rather than > improves code quality. Well, we have abstracted from FreeBSD specifics by using macros in other cases as well. Adding another macro will make reading a bit harder and you have to lookup the platform specific implementation of the code to figure out what is going on, but that way, I guess, people will get a result they can live with. Best regards Michael > > -- Ian > > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
On Mon, 2020-05-18 at 23:01 +0200, Michael Tuexen wrote: > > On 18. May 2020, at 22:48, Ian Lepore wrote: > > > > On Mon, 2020-05-18 at 22:43 +0200, Michael Tuexen wrote: > > > > 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. > > > > I'm trying to figure out why your original commit was a problem. I > > understand why it was questioned, but once the answer came out, > > it's > > clear that the code you originally committed does what it's > > supposed to > > without any harmful side effects. Sure, freebsd doesn't strictly > > need > > I guess the point Conrad is making, that on FreeBSD the check is not > needed, since the call can not fail. So the FreeBSD code base would > not > be consistent: within the SCTP related code the return code is > checked, > in the other code it is not. > > it, but the code is shared among projects, so what's the harm in > > the > > extra check that helps other projects sharing the code? It's > > certainly > > a lot less confusion and code clutter than any of the "remedies" > > that > > have been discussed. > > Yepp, sharing code between platforms makes things harder. Running the > same > code in kernel land and userland does not make it simpler. Different > groups > have different opinions/styles/... > > I'll revert the commit tomorrow and a variadic macros > SCTP_SNPRINTF(), which > will map on FreeBSD to snprintf() and on the other platforms will do > the check. > > If the build problem comes up on FreeBSD userland (and I have no idea > if that > is the case, since I don't know how Firefox / Chrome are build on > FreeBSD), > I leave it up to the port maintainer of the application to deal with > it. > > Best regards > Michael > > > > -- Ian > > > > Well it seems to me you're being asked to do a lot of extra work that has the final result of making the code LESS clear and MORE complex, because of one person's opinion. I'm actually a bit tempted to complain about the change, because to me it reduces rather than improves code quality. -- Ian ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 22:48, Ian Lepore wrote: > > On Mon, 2020-05-18 at 22:43 +0200, Michael Tuexen wrote: >>> 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. > > I'm trying to figure out why your original commit was a problem. I > understand why it was questioned, but once the answer came out, it's > clear that the code you originally committed does what it's supposed to > without any harmful side effects. Sure, freebsd doesn't strictly need I guess the point Conrad is making, that on FreeBSD the check is not needed, since the call can not fail. So the FreeBSD code base would not be consistent: within the SCTP related code the return code is checked, in the other code it is not. > it, but the code is shared among projects, so what's the harm in the > extra check that helps other projects sharing the code? It's certainly > a lot less confusion and code clutter than any of the "remedies" that > have been discussed. Yepp, sharing code between platforms makes things harder. Running the same code in kernel land and userland does not make it simpler. Different groups have different opinions/styles/... I'll revert the commit tomorrow and a variadic macros SCTP_SNPRINTF(), which will map on FreeBSD to snprintf() and on the other platforms will do the check. If the build problem comes up on FreeBSD userland (and I have no idea if that is the case, since I don't know how Firefox / Chrome are build on FreeBSD), I leave it up to the port maintainer of the application to deal with it. Best regards Michael > > -- Ian > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
On Mon, 2020-05-18 at 22:43 +0200, Michael Tuexen wrote: > > 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. I'm trying to figure out why your original commit was a problem. I understand why it was questioned, but once the answer came out, it's clear that the code you originally committed does what it's supposed to without any harmful side effects. Sure, freebsd doesn't strictly need it, but the code is shared among projects, so what's the harm in the extra check that helps other projects sharing the code? It's certainly a lot less confusion and code clutter than any of the "remedies" that have been discussed. -- Ian ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 22:17, Conrad Meyer wrote:
>
> Hi Michael,
>
> On Mon, May 18, 2020 at 12:05 PM Michael Tuexen wrote:
>>
>>> On 18. May 2020, at 20:23, Conrad Meyer 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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
Hi Michael,
On Mon, May 18, 2020 at 12:05 PM Michael Tuexen wrote:
>
> > On 18. May 2020, at 20:23, Conrad Meyer 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.
> 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
defining an snprintf wrapper function for non-FreeBSD platforms and
using it universally.
> 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.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 20:23, Conrad Meyer wrote:
>
> On Mon, May 18, 2020 at 10:35 AM Michael Tuexen wrote:
>>
>>> On 18. May 2020, at 17:38, Conrad Meyer 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
Re: svn commit: r361209 - head/sys/netinet
On Mon, May 18, 2020 at 10:35 AM Michael Tuexen wrote: > > > On 18. May 2020, at 17:38, Conrad Meyer 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 ‘\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. 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 ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 17:48, Conrad Meyer wrote:
>
> (In fact, I don't believe snprintf(9) can return a negative value at
> all. And snprintf(3) will only do so in some special circumstances
> for features snprintf(9) does not support: buffer size or formatted
> result longer than INT_MAX; invalid *nn$ field width or precision
> specifiers. I don't think either case applies to these strings,
> although I did not read all of them thoroughly.)
I have no problem to revert this change for FreeBSD if it is there
impossible (and hopefully gcc 10 knows about this in case someone
builds the userland stack with gcc 10 on FreeBSD). In the posix
specification I don't see this.
For FreeBSD, it would be great to update the man page, which currently
states
RETURN VALUES
These functions return the number of characters printed (not including
the trailing `\0' used to end output to strings), except for snprintf()
and vsnprintf(), which return the number of characters that would have
been printed if the size were unlimited (again, not including the final
`\0'). These functions return a negative value if an error occurs.
So it would be great to be specific about when an error occurs in the
last sentence.
Best regards
Michael
>
> On Mon, May 18, 2020 at 8:38 AM Conrad Meyer wrote:
>>
>> Hi Michael,
>>
>> 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
>> 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?
>>
>> FreeBSD's snprintf(9) does not have this bug, nor does its snprintf(3).
>>
>> Best regards,
>> Conrad
>>
>> On Mon, May 18, 2020 at 3:07 AM Michael Tuexen wrote:
>>>
>>> Author: tuexen
>>> Date: Mon May 18 10:07:01 2020
>>> New Revision: 361209
>>> URL: https://svnweb.freebsd.org/changeset/base/361209
>>>
>>> Log:
>>> Handle failures of snprintf().
>>>
>>> MFC after:3 days
>>>
>>> Modified:
>>> head/sys/netinet/sctp_asconf.c
>>> head/sys/netinet/sctp_indata.c
>>> head/sys/netinet/sctp_input.c
>>> head/sys/netinet/sctp_output.c
>>> head/sys/netinet/sctp_pcb.c
>>>
>>> Modified: head/sys/netinet/sctp_asconf.c
>>> ==
>>> --- head/sys/netinet/sctp_asconf.c Mon May 18 09:46:51 2020
>>> (r361208)
>>> +++ head/sys/netinet/sctp_asconf.c Mon May 18 10:07:01 2020
>>> (r361209)
>>> @@ -1706,8 +1706,9 @@ sctp_handle_asconf_ack(struct mbuf *m, int offset,
>>>char msg[SCTP_DIAG_INFO_LEN];
>>>
>>>SCTPDBG(SCTP_DEBUG_ASCONF1, "handle_asconf_ack: got
>>> unexpected next serial number! Aborting asoc!\n");
>>> - snprintf(msg, sizeof(msg), "Never sent serial number %8.8x",
>>> - serial_num);
>>> + if (snprintf(msg, sizeof(msg), "Never sent serial number
>>> %8.8x", serial_num) < 0) {
>>> + msg[0] = '\0';
>>> + }
>>>op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
>>> msg);
>>>sctp_abort_an_association(stcb->sctp_ep, stcb, op_err,
>>> SCTP_SO_NOT_LOCKED);
>>>*abort_no_unlock = 1;
>>>
>>> Modified: head/sys/netinet/sctp_indata.c
>>> ==
>>> --- head/sys/netinet/sctp_indata.c Mon May 18 09:46:51 2020
>>> (r361208)
>>> +++ head/sys/netinet/sctp_indata.c Mon May 18 10:07:01 2020
>>> (r361209)
>>> @@ -434,22 +434,26 @@ sctp_abort_in_reasm(struct sctp_tcb *stcb,
>>>struct mbuf *oper;
>>>
>>>if (stcb->asoc.idata_supported) {
>>> - snprintf(msg, sizeof(msg),
>>> + if (snprintf(msg, sizeof(msg),
>>>"Reass %x,CF:%x,TSN=%8.8x,SID=%4.4x,FSN=%8.8x,MID:%8.8x",
>>>opspot,
>>>control->fsn_included,
>>>chk->rec.data.tsn,
>>>chk->rec.data.sid,
>>> - chk->rec.data.fsn, chk->rec.data.mid);
>>> + chk->rec.data.fsn, chk->rec.data.mid) < 0) {
>>> + msg[0] = '\0';
>>> + }
>>>} else {
>>> - snprintf(msg, sizeof(msg),
>>> + if (snprintf(msg, sizeof(msg),
>>>"Reass %x,CI:%x,TSN=%8.8x,SID=%4.4x,FSN=%4.4x,SSN:%4.4x",
>>>opspot,
>>>control->fsn_included,
>>>chk->rec.data.tsn,
>>>chk->rec.data.sid,
>>>chk->rec.data.fsn,
>>> - (uint16_t)chk->rec.data.mid);
>>> +
Re: svn commit: r361209 - head/sys/netinet
> On 18. May 2020, at 17:38, Conrad Meyer wrote:
>
> Hi Michael,
>
> 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
Hi Conrad,
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.
Best regards
Michael
>
> FreeBSD's snprintf(9) does not have this bug, nor does its snprintf(3).
>
> Best regards,
> Conrad
>
> On Mon, May 18, 2020 at 3:07 AM Michael Tuexen wrote:
>>
>> Author: tuexen
>> Date: Mon May 18 10:07:01 2020
>> New Revision: 361209
>> URL: https://svnweb.freebsd.org/changeset/base/361209
>>
>> Log:
>> Handle failures of snprintf().
>>
>> MFC after:3 days
>>
>> Modified:
>> head/sys/netinet/sctp_asconf.c
>> head/sys/netinet/sctp_indata.c
>> head/sys/netinet/sctp_input.c
>> head/sys/netinet/sctp_output.c
>> head/sys/netinet/sctp_pcb.c
>>
>> Modified: head/sys/netinet/sctp_asconf.c
>> ==
>> --- head/sys/netinet/sctp_asconf.c Mon May 18 09:46:51 2020
>> (r361208)
>> +++ head/sys/netinet/sctp_asconf.c Mon May 18 10:07:01 2020
>> (r361209)
>> @@ -1706,8 +1706,9 @@ sctp_handle_asconf_ack(struct mbuf *m, int offset,
>>char msg[SCTP_DIAG_INFO_LEN];
>>
>>SCTPDBG(SCTP_DEBUG_ASCONF1, "handle_asconf_ack: got
>> unexpected next serial number! Aborting asoc!\n");
>> - snprintf(msg, sizeof(msg), "Never sent serial number %8.8x",
>> - serial_num);
>> + if (snprintf(msg, sizeof(msg), "Never sent serial number
>> %8.8x", serial_num) < 0) {
>> + msg[0] = '\0';
>> + }
>>op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
>> msg);
>>sctp_abort_an_association(stcb->sctp_ep, stcb, op_err,
>> SCTP_SO_NOT_LOCKED);
>>*abort_no_unlock = 1;
>>
>> Modified: head/sys/netinet/sctp_indata.c
>> ==
>> --- head/sys/netinet/sctp_indata.c Mon May 18 09:46:51 2020
>> (r361208)
>> +++ head/sys/netinet/sctp_indata.c Mon May 18 10:07:01 2020
>> (r361209)
>> @@ -434,22 +434,26 @@ sctp_abort_in_reasm(struct sctp_tcb *stcb,
>>struct mbuf *oper;
>>
>>if (stcb->asoc.idata_supported) {
>> - snprintf(msg, sizeof(msg),
>> + if (snprintf(msg, sizeof(msg),
>>"Reass %x,CF:%x,TSN=%8.8x,SID=%4.4x,FSN=%8.8x,MID:%8.8x",
>>opspot,
>>control->fsn_included,
>>chk->rec.data.tsn,
>>chk->rec.data.sid,
>> - chk->rec.data.fsn, chk->rec.data.mid);
>> + chk->rec.data.fsn, chk->rec.data.mid) < 0) {
>> + msg[0] = '\0';
>> + }
>>} else {
>> - snprintf(msg, sizeof(msg),
>> + if (snprintf(msg, sizeof(msg),
>>"Reass %x,CI:%x,TSN=%8.8x,SID=%4.4x,FSN=%4.4x,SSN:%4.4x",
>>opspot,
>>control->fsn_included,
>>chk->rec.data.tsn,
>>chk->rec.data.sid,
>>chk->rec.data.fsn,
>> - (uint16_t)chk->rec.data.mid);
>> + (uint16_t)chk->rec.data.mid) < 0) {
>> + msg[0] = '\0';
>> + }
>>}
>>oper = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, msg);
>>sctp_m_freem(chk->data);
>> @@ -533,15 +537,19 @@ sctp_queue_data_to_stream(struct sctp_tcb *stcb,
>> */
>>TAILQ_INSERT_HEAD(>inqueue, control, next_instrm);
>>if (asoc->idata_supported) {
>> - snprintf(msg, sizeof(msg), "Delivered MID=%8.8x, got
>> TSN=%8.8x, SID=%4.4x, MID=%8.8x",
>> + if (snprintf(msg, sizeof(msg), "Delivered MID=%8.8x,
>> got TSN=%8.8x, SID=%4.4x, MID=%8.8x",
>>strm->last_mid_delivered, control->sinfo_tsn,
>> - control->sinfo_stream, control->mid);
>> + control->sinfo_stream, control->mid) < 0) {
>> +
Re: svn commit: r361209 - head/sys/netinet
(In fact, I don't believe snprintf(9) can return a negative value at
all. And snprintf(3) will only do so in some special circumstances
for features snprintf(9) does not support: buffer size or formatted
result longer than INT_MAX; invalid *nn$ field width or precision
specifiers. I don't think either case applies to these strings,
although I did not read all of them thoroughly.)
On Mon, May 18, 2020 at 8:38 AM Conrad Meyer wrote:
>
> Hi Michael,
>
> 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
> 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?
>
> FreeBSD's snprintf(9) does not have this bug, nor does its snprintf(3).
>
> Best regards,
> Conrad
>
> On Mon, May 18, 2020 at 3:07 AM Michael Tuexen wrote:
> >
> > Author: tuexen
> > Date: Mon May 18 10:07:01 2020
> > New Revision: 361209
> > URL: https://svnweb.freebsd.org/changeset/base/361209
> >
> > Log:
> > Handle failures of snprintf().
> >
> > MFC after:3 days
> >
> > Modified:
> > head/sys/netinet/sctp_asconf.c
> > head/sys/netinet/sctp_indata.c
> > head/sys/netinet/sctp_input.c
> > head/sys/netinet/sctp_output.c
> > head/sys/netinet/sctp_pcb.c
> >
> > Modified: head/sys/netinet/sctp_asconf.c
> > ==
> > --- head/sys/netinet/sctp_asconf.c Mon May 18 09:46:51 2020
> > (r361208)
> > +++ head/sys/netinet/sctp_asconf.c Mon May 18 10:07:01 2020
> > (r361209)
> > @@ -1706,8 +1706,9 @@ sctp_handle_asconf_ack(struct mbuf *m, int offset,
> > char msg[SCTP_DIAG_INFO_LEN];
> >
> > SCTPDBG(SCTP_DEBUG_ASCONF1, "handle_asconf_ack: got
> > unexpected next serial number! Aborting asoc!\n");
> > - snprintf(msg, sizeof(msg), "Never sent serial number %8.8x",
> > - serial_num);
> > + if (snprintf(msg, sizeof(msg), "Never sent serial number
> > %8.8x", serial_num) < 0) {
> > + msg[0] = '\0';
> > + }
> > op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
> > msg);
> > sctp_abort_an_association(stcb->sctp_ep, stcb, op_err,
> > SCTP_SO_NOT_LOCKED);
> > *abort_no_unlock = 1;
> >
> > Modified: head/sys/netinet/sctp_indata.c
> > ==
> > --- head/sys/netinet/sctp_indata.c Mon May 18 09:46:51 2020
> > (r361208)
> > +++ head/sys/netinet/sctp_indata.c Mon May 18 10:07:01 2020
> > (r361209)
> > @@ -434,22 +434,26 @@ sctp_abort_in_reasm(struct sctp_tcb *stcb,
> > struct mbuf *oper;
> >
> > if (stcb->asoc.idata_supported) {
> > - snprintf(msg, sizeof(msg),
> > + if (snprintf(msg, sizeof(msg),
> > "Reass
> > %x,CF:%x,TSN=%8.8x,SID=%4.4x,FSN=%8.8x,MID:%8.8x",
> > opspot,
> > control->fsn_included,
> > chk->rec.data.tsn,
> > chk->rec.data.sid,
> > - chk->rec.data.fsn, chk->rec.data.mid);
> > + chk->rec.data.fsn, chk->rec.data.mid) < 0) {
> > + msg[0] = '\0';
> > + }
> > } else {
> > - snprintf(msg, sizeof(msg),
> > + if (snprintf(msg, sizeof(msg),
> > "Reass
> > %x,CI:%x,TSN=%8.8x,SID=%4.4x,FSN=%4.4x,SSN:%4.4x",
> > opspot,
> > control->fsn_included,
> > chk->rec.data.tsn,
> > chk->rec.data.sid,
> > chk->rec.data.fsn,
> > - (uint16_t)chk->rec.data.mid);
> > + (uint16_t)chk->rec.data.mid) < 0) {
> > + msg[0] = '\0';
> > + }
> > }
> > oper = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, msg);
> > sctp_m_freem(chk->data);
> > @@ -533,15 +537,19 @@ sctp_queue_data_to_stream(struct sctp_tcb *stcb,
> > */
> > TAILQ_INSERT_HEAD(>inqueue, control, next_instrm);
> > if (asoc->idata_supported) {
> > - snprintf(msg, sizeof(msg), "Delivered MID=%8.8x,
> > got TSN=%8.8x, SID=%4.4x, MID=%8.8x",
> > + if (snprintf(msg, sizeof(msg), "Delivered
> > MID=%8.8x, got TSN=%8.8x, SID=%4.4x, MID=%8.8x",
> > strm->last_mid_delivered, control->sinfo_tsn,
> > - control->sinfo_stream, control->mid);
> > +
Re: svn commit: r361209 - head/sys/netinet
Hi Michael,
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
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?
FreeBSD's snprintf(9) does not have this bug, nor does its snprintf(3).
Best regards,
Conrad
On Mon, May 18, 2020 at 3:07 AM Michael Tuexen wrote:
>
> Author: tuexen
> Date: Mon May 18 10:07:01 2020
> New Revision: 361209
> URL: https://svnweb.freebsd.org/changeset/base/361209
>
> Log:
> Handle failures of snprintf().
>
> MFC after:3 days
>
> Modified:
> head/sys/netinet/sctp_asconf.c
> head/sys/netinet/sctp_indata.c
> head/sys/netinet/sctp_input.c
> head/sys/netinet/sctp_output.c
> head/sys/netinet/sctp_pcb.c
>
> Modified: head/sys/netinet/sctp_asconf.c
> ==
> --- head/sys/netinet/sctp_asconf.c Mon May 18 09:46:51 2020
> (r361208)
> +++ head/sys/netinet/sctp_asconf.c Mon May 18 10:07:01 2020
> (r361209)
> @@ -1706,8 +1706,9 @@ sctp_handle_asconf_ack(struct mbuf *m, int offset,
> char msg[SCTP_DIAG_INFO_LEN];
>
> SCTPDBG(SCTP_DEBUG_ASCONF1, "handle_asconf_ack: got
> unexpected next serial number! Aborting asoc!\n");
> - snprintf(msg, sizeof(msg), "Never sent serial number %8.8x",
> - serial_num);
> + if (snprintf(msg, sizeof(msg), "Never sent serial number
> %8.8x", serial_num) < 0) {
> + msg[0] = '\0';
> + }
> op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
> msg);
> sctp_abort_an_association(stcb->sctp_ep, stcb, op_err,
> SCTP_SO_NOT_LOCKED);
> *abort_no_unlock = 1;
>
> Modified: head/sys/netinet/sctp_indata.c
> ==
> --- head/sys/netinet/sctp_indata.c Mon May 18 09:46:51 2020
> (r361208)
> +++ head/sys/netinet/sctp_indata.c Mon May 18 10:07:01 2020
> (r361209)
> @@ -434,22 +434,26 @@ sctp_abort_in_reasm(struct sctp_tcb *stcb,
> struct mbuf *oper;
>
> if (stcb->asoc.idata_supported) {
> - snprintf(msg, sizeof(msg),
> + if (snprintf(msg, sizeof(msg),
> "Reass %x,CF:%x,TSN=%8.8x,SID=%4.4x,FSN=%8.8x,MID:%8.8x",
> opspot,
> control->fsn_included,
> chk->rec.data.tsn,
> chk->rec.data.sid,
> - chk->rec.data.fsn, chk->rec.data.mid);
> + chk->rec.data.fsn, chk->rec.data.mid) < 0) {
> + msg[0] = '\0';
> + }
> } else {
> - snprintf(msg, sizeof(msg),
> + if (snprintf(msg, sizeof(msg),
> "Reass %x,CI:%x,TSN=%8.8x,SID=%4.4x,FSN=%4.4x,SSN:%4.4x",
> opspot,
> control->fsn_included,
> chk->rec.data.tsn,
> chk->rec.data.sid,
> chk->rec.data.fsn,
> - (uint16_t)chk->rec.data.mid);
> + (uint16_t)chk->rec.data.mid) < 0) {
> + msg[0] = '\0';
> + }
> }
> oper = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, msg);
> sctp_m_freem(chk->data);
> @@ -533,15 +537,19 @@ sctp_queue_data_to_stream(struct sctp_tcb *stcb,
> */
> TAILQ_INSERT_HEAD(>inqueue, control, next_instrm);
> if (asoc->idata_supported) {
> - snprintf(msg, sizeof(msg), "Delivered MID=%8.8x, got
> TSN=%8.8x, SID=%4.4x, MID=%8.8x",
> + if (snprintf(msg, sizeof(msg), "Delivered MID=%8.8x,
> got TSN=%8.8x, SID=%4.4x, MID=%8.8x",
> strm->last_mid_delivered, control->sinfo_tsn,
> - control->sinfo_stream, control->mid);
> + control->sinfo_stream, control->mid) < 0) {
> + msg[0] = '\0';
> + }
> } else {
> - snprintf(msg, sizeof(msg), "Delivered SSN=%4.4x, got
> TSN=%8.8x, SID=%4.4x, SSN=%4.4x",
> + if (snprintf(msg, sizeof(msg), "Delivered SSN=%4.4x,
> got TSN=%8.8x, SID=%4.4x, SSN=%4.4x",
> (uint16_t)strm->last_mid_delivered,
> control->sinfo_tsn,
> control->sinfo_stream,
> - (uint16_t)control->mid);
> + (uint16_t)control->mid) < 0)
svn commit: r361209 - head/sys/netinet
Author: tuexen
Date: Mon May 18 10:07:01 2020
New Revision: 361209
URL: https://svnweb.freebsd.org/changeset/base/361209
Log:
Handle failures of snprintf().
MFC after:3 days
Modified:
head/sys/netinet/sctp_asconf.c
head/sys/netinet/sctp_indata.c
head/sys/netinet/sctp_input.c
head/sys/netinet/sctp_output.c
head/sys/netinet/sctp_pcb.c
Modified: head/sys/netinet/sctp_asconf.c
==
--- head/sys/netinet/sctp_asconf.c Mon May 18 09:46:51 2020
(r361208)
+++ head/sys/netinet/sctp_asconf.c Mon May 18 10:07:01 2020
(r361209)
@@ -1706,8 +1706,9 @@ sctp_handle_asconf_ack(struct mbuf *m, int offset,
char msg[SCTP_DIAG_INFO_LEN];
SCTPDBG(SCTP_DEBUG_ASCONF1, "handle_asconf_ack: got unexpected
next serial number! Aborting asoc!\n");
- snprintf(msg, sizeof(msg), "Never sent serial number %8.8x",
- serial_num);
+ if (snprintf(msg, sizeof(msg), "Never sent serial number
%8.8x", serial_num) < 0) {
+ msg[0] = '\0';
+ }
op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
msg);
sctp_abort_an_association(stcb->sctp_ep, stcb, op_err,
SCTP_SO_NOT_LOCKED);
*abort_no_unlock = 1;
Modified: head/sys/netinet/sctp_indata.c
==
--- head/sys/netinet/sctp_indata.c Mon May 18 09:46:51 2020
(r361208)
+++ head/sys/netinet/sctp_indata.c Mon May 18 10:07:01 2020
(r361209)
@@ -434,22 +434,26 @@ sctp_abort_in_reasm(struct sctp_tcb *stcb,
struct mbuf *oper;
if (stcb->asoc.idata_supported) {
- snprintf(msg, sizeof(msg),
+ if (snprintf(msg, sizeof(msg),
"Reass %x,CF:%x,TSN=%8.8x,SID=%4.4x,FSN=%8.8x,MID:%8.8x",
opspot,
control->fsn_included,
chk->rec.data.tsn,
chk->rec.data.sid,
- chk->rec.data.fsn, chk->rec.data.mid);
+ chk->rec.data.fsn, chk->rec.data.mid) < 0) {
+ msg[0] = '\0';
+ }
} else {
- snprintf(msg, sizeof(msg),
+ if (snprintf(msg, sizeof(msg),
"Reass %x,CI:%x,TSN=%8.8x,SID=%4.4x,FSN=%4.4x,SSN:%4.4x",
opspot,
control->fsn_included,
chk->rec.data.tsn,
chk->rec.data.sid,
chk->rec.data.fsn,
- (uint16_t)chk->rec.data.mid);
+ (uint16_t)chk->rec.data.mid) < 0) {
+ msg[0] = '\0';
+ }
}
oper = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, msg);
sctp_m_freem(chk->data);
@@ -533,15 +537,19 @@ sctp_queue_data_to_stream(struct sctp_tcb *stcb,
*/
TAILQ_INSERT_HEAD(>inqueue, control, next_instrm);
if (asoc->idata_supported) {
- snprintf(msg, sizeof(msg), "Delivered MID=%8.8x, got
TSN=%8.8x, SID=%4.4x, MID=%8.8x",
+ if (snprintf(msg, sizeof(msg), "Delivered MID=%8.8x,
got TSN=%8.8x, SID=%4.4x, MID=%8.8x",
strm->last_mid_delivered, control->sinfo_tsn,
- control->sinfo_stream, control->mid);
+ control->sinfo_stream, control->mid) < 0) {
+ msg[0] = '\0';
+ }
} else {
- snprintf(msg, sizeof(msg), "Delivered SSN=%4.4x, got
TSN=%8.8x, SID=%4.4x, SSN=%4.4x",
+ if (snprintf(msg, sizeof(msg), "Delivered SSN=%4.4x,
got TSN=%8.8x, SID=%4.4x, SSN=%4.4x",
(uint16_t)strm->last_mid_delivered,
control->sinfo_tsn,
control->sinfo_stream,
- (uint16_t)control->mid);
+ (uint16_t)control->mid) < 0) {
+ msg[0] = '\0';
+ }
}
op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION,
msg);
stcb->sctp_ep->last_abort_code = SCTP_FROM_SCTP_INDATA +
SCTP_LOC_2;
@@ -648,9 +656,10 @@ sctp_queue_data_to_stream(struct sctp_tcb *stcb,
* to put it on the queue.
*/
if (sctp_place_control_in_stream(strm, asoc, control)) {
- snprintf(msg, sizeof(msg),
- "Queue to str MID: %u duplicate",
- control->mid);
+ if (snprintf(msg, sizeof(msg),
+ "Queue to str MID: %u duplicate", control->mid) <
0) {
+ msg[0] =
