linux-kernel - Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACKs7VDzgUyDM9FhRR69Aqw2-0xiZC86EhkqSmD5P68derRBFw@mail.gmail.com>
Date:   Tue, 15 Sep 2020 13:03:17 +0300
From:   Stefan Puiu <stefan.puiu@...il.com>
To:     Alejandro Colomar <colomar.6.4.3@...il.com>
Cc:     Michael Kerrisk <mtk.manpages@...il.com>,
        lnx-man <linux-man@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size
 (instead of hardcoding macro name)

Hi,

On Fri, Sep 11, 2020 at 6:28 PM Alejandro Colomar
<colomar.6.4.3@...il.com> wrote:
>
> Hi Stefan,
>
> On 2020-09-11 16:35, Stefan Puiu wrote:
>  > Hi,
>  >
>  > On Fri, Sep 11, 2020 at 12:15 AM Alejandro Colomar
>  > <colomar.6.4.3@...il.com> wrote:
>  >>
>  >> Signed-off-by: Alejandro Colomar <colomar.6.4.3@...il.com>
>  >> ---
>  >>   man3/getgrent_r.3 | 2 +-
>  >>   1 file changed, 1 insertion(+), 1 deletion(-)
>  >>
>  >> diff --git a/man3/getgrent_r.3 b/man3/getgrent_r.3
>  >> index 81d81a851..76deec370 100644
>  >> --- a/man3/getgrent_r.3
>  >> +++ b/man3/getgrent_r.3
>  >> @@ -186,7 +186,7 @@ main(void)
>  >>
>  >>       setgrent();
>  >>       while (1) {
>  >> -        i = getgrent_r(&grp, buf, BUFLEN, &grpp);
>  >> +        i = getgrent_r(&grp, buf, sizeof(buf), &grpp);
>  >
>  > I'm worried that less attentive people might copy/paste parts of this
>  > in their code, where maybe buf is just a pointer, and expect it to
>  > work. Maybe leaving BUFLEN here is useful as a reminder that they need
>  > to change something to adapt the code?
>  >
>  > Just my 2 cents,
>  > Stefan.
>  >
> That's a very good point.
>
> So we have 3 options and I will propose now a 4th one.  Let's see all
> of them and see which one is better for the man pages.
>
> 1.-     Use the macro everywhere.
>
> pros:
> - It is still valid when the buffer is a pointer and not an array.
> cons:
> - Hardcodes the initializer.  If the array is later initialized with a
>    different value, it may produce a silent bug, or a compilation break.
>
> 2.-     Use sizeof() everywhere, and the macro for the initializer.
>
> pros:
> - It is valid as long as the buffer is an array.
> cons:
> - If the code gets into a function, and the buffer is then a pointer,
>    it will definitively produce a silent bug.
>
> 3.-     Use sizeof() everywhere, and a magic number for the initializer.
>
> The same as 2.
>
> 4.-     Use ARRAY_BYTES() macro
>
> pros:
> - It is always safe and when code changes, it may break compilation, but
>    never a silent bug.
> cons:
> - Add a few lines of code.  Maybe too much complexity for an example.
>    But I'd say that it is the only safe option, and in real code it
>    should probably be used more, so maybe it's good to show a good practice.

If you ask me, I think examples should be simple and easy to
understand, and easy to copy/paste in your code. I'd settle for easy
enough, not perfect or completely foolproof. If you need to look up
obscure gcc features to understand an example, that's not very
helpful. So I'd be more inclined to prefer version 1 above. But let's
see Michael's opinion on this.

Just my 2c,
Stefan.