> On 18. May 2020, at 20:23, Conrad Meyer <c...@freebsd.org> wrote: > > On Mon, May 18, 2020 at 10:35 AM Michael Tuexen <tue...@freebsd.org> wrote: >> >>> On 18. May 2020, at 17:38, Conrad Meyer <c...@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 _______________________________________________ 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"