From owner-svn-src-all@freebsd.org Mon May 18 18:23:31 2020 Return-Path: Delivered-To: svn-src-all@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 34EE32D9CD2; Mon, 18 May 2020 18:23:31 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-oi1-f176.google.com (mail-oi1-f176.google.com [209.85.167.176]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49QnSy6CBjz47wX; Mon, 18 May 2020 18:23:30 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-oi1-f176.google.com with SMTP id l6so4004908oic.9; Mon, 18 May 2020 11:23:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc:content-transfer-encoding; bh=XJAmKW9HwsEuori9FroHRrGEb9ZBoppojCR3qtAOXiQ=; b=eTeZNe7FgZEjZEJRPsJdOeyBdx3tTgGw8BummbgEavyPoFF1zzkbnba9084cpJc0l4 F/NAYMZ/QFeOkekHRT/iZznoC5nc4tWGQYZS2m9/ZsZTIwh0ZNM1hKk6M+OhJ06Zcb6y KvCLaCtEapa4Zi2zUn3kqMh1z5mGpHG+1sQxN7pTOFMHRTVRs09hU2qQNuBhArO0i/TG gEqQzF//ZQCxbGGqzG+WzQsabCS9TeFveNu4cfQp4rE1ikjkMhWB3nTEEWJ37HBbP/LT +SOhGoub6GFCvTEdAQn0C3MeKx7OfusQ0JoXsMx9WGfr3fv9F3VouaD/Ueig5owjzCUx JIQQ== X-Gm-Message-State: AOAM532jjaliexO01nr5gW/csWrYFqmMjWlk9t/3lyLZCZqwInA1xvR7 1vRE0EZeYo9OQpxHJnTwivkCPDOt X-Google-Smtp-Source: ABdhPJx/JrbRyL7DD+KDr7z7WJKDmRBmrz1PURThCl9CYzlmgk+QZpd7gvbGcfYY+7cL1qC+sv8sXw== X-Received: by 2002:aca:fd14:: with SMTP id b20mr512039oii.14.1589826209357; Mon, 18 May 2020 11:23:29 -0700 (PDT) Received: from mail-oi1-f177.google.com (mail-oi1-f177.google.com. [209.85.167.177]) by smtp.gmail.com with ESMTPSA id k69sm945706oib.26.2020.05.18.11.23.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 May 2020 11:23:29 -0700 (PDT) Received: by mail-oi1-f177.google.com with SMTP id 19so9884796oiy.8; Mon, 18 May 2020 11:23:28 -0700 (PDT) X-Received: by 2002:aca:a854:: with SMTP id r81mr489211oie.81.1589826208690; Mon, 18 May 2020 11:23:28 -0700 (PDT) MIME-Version: 1.0 References: <202005181007.04IA713t089936@repo.freebsd.org> In-Reply-To: Reply-To: cem@freebsd.org From: Conrad Meyer Date: Mon, 18 May 2020 11:23:17 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r361209 - head/sys/netinet To: Michael Tuexen Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 49QnSy6CBjz47wX X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; TAGGED_FROM(0.00)[]; REPLY(-4.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 May 2020 18:23:31 -0000 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 =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