From owner-svn-src-head@freebsd.org Mon May 18 19:05:43 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 3DAE72DAD48; Mon, 18 May 2020 19:05:43 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (mail-n.franken.de [193.175.24.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.franken.de", Issuer "COMODO RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49QpPf660Zz4CJq; Mon, 18 May 2020 19:05:42 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from mbp.fritz.box (ip4d1600c4.dynamic.kabel-deutschland.de [77.22.0.196]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id ED7477220F406; Mon, 18 May 2020 21:05:38 +0200 (CEST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: svn commit: r361209 - head/sys/netinet From: Michael Tuexen In-Reply-To: Date: Mon, 18 May 2020 21:05:38 +0200 Cc: src-committers , svn-src-all , svn-src-head Content-Transfer-Encoding: quoted-printable Message-Id: <064C2DCD-6279-4442-A900-0ECCD50CC4FA@freebsd.org> References: <202005181007.04IA713t089936@repo.freebsd.org> To: cem@freebsd.org X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=disabled version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mail-n.franken.de X-Rspamd-Queue-Id: 49QpPf660Zz4CJq X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; ASN(0.00)[asn:680, ipnet:193.174.0.0/15, country:DE]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 May 2020 19:05:43 -0000 > On 18. May 2020, at 20:23, Conrad Meyer wrote: >=20 > On Mon, May 18, 2020 at 10:35 AM Michael Tuexen = wrote: >>=20 >>> On 18. May 2020, at 17:38, Conrad Meyer 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