Messages in this thread | | | Subject | Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name) | From | Alejandro Colomar <> | Date | Thu, 24 Sep 2020 15:09:52 +0200 |
| |
Hi Michael,
On 2020-09-24 13:38, Michael Kerrisk (man-pages) wrote: > On 9/24/20 11:35 AM, Alejandro Colomar wrote: >> Hi, >> >> On 2020-09-23 22:35, Michael Kerrisk (man-pages) wrote: >>> On 9/15/20 12:03 PM, Stefan Puiu wrote: >>>> Hi, >>>> >>>> On Fri, Sep 11, 2020 at 6:28 PM Alejandro Colomar >>>> <colomar.6.4.3@gmail.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@gmail.com> wrote: >>>>> >> >>>>> >> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.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, >>> >>> So, the fundamental problem is that C is nearly 50 years old. >>> It's a great high-level assembly language, but when it comes >>> to nuances like this it gets pretty painful. One can do macro >>> magic of the kind you suggest, but I agree with Stefan that it >>> gets confusing and distracting for the reader. I think I also >>> lean to solution 1. Yes, it's not perfect, but it's easy to >>> understand, and I don't think we can or should try and solve >>> the broken-ness of C in the manual pages. >>> >>> Thanks, >>> >>> Michael >>> >>> >> >> I was reverting the 3 patches I introduced (they changed from solution 1 >> to solution 2), and also was grepping for already existing solution 2 in >> the pages (it seems that solution 2 was a bit more extended than >> solution 1). >> >> While doing that, I've been thinking about it again... >> >> There's a good thing about sizeof (even though I admit it's very >> insecure; and I never use it for myself), especially for the man pages: >> >> I'll copy here a sample from getnameinfo.3 to ilustrate it: >> >> [[ >> .EX >> struct sockaddr *addr; /* input */ >> socklen_t addrlen; /* input */ >> char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV]; >> >> if (getnameinfo(addr, addrlen, hbuf, sizeof(hbuf), sbuf, >> sizeof(sbuf), NI_NUMERICHOST | NI_NUMERICSERV) == 0) >> printf("host=%s, serv=%s\en", hbuf, sbuf); >> .EE >> ]] >> >> Here, it's clear to the reader that the 4th argument to 'getnameinfo()' >> is the size of the buffer passed as the 3rd argument. >> >> If the function call was changed to >> >> [[ >> getnameinfo(addr, addrlen, hbuf, NI_MAXHOST, sbuf, >> sizeof(sbuf), NI_NUMERICHOST | NI_NUMERICSERV) >> ]] >> >> then it would be less clear, and the reader should go back and forth to >> see where that comes from. In this short example it is relatively very >> clear, but in some examples it might be less clear. >> >> Would you maintain your preference for solution 1? >> >> >> Also... I am trying to patch glibc to provide a safe version of >> 'nitems()', and shortly after they accept that patch (if they do), I'll >> send another one to add a safe 'array_bytes()' based on 'nitems()'. >> >> Maybe the examples could use 'array_bytes()'; although is will be a >> glibc extension, and non-existent in any other POSIX systems, of course, >> which would make the examples non-portable, but still can be solved with >> a simple >> >> [[ >> #if !defined(array_bytes) >> #define array_bytes() sizeof() >> #endif >> ]] >> >> But again it complicates the examples... > > (And I'd rather not...) > >> I'm not sure at all about what should be done. Please comment. If you >> still prefer solution 1, I'll send you a patch with the revert + fixes, >> but I think it's very delicate. > > Okay -- I agree with your perspective of the getnameinfo example. > > So, I think maybe solution 1 is clearer sometimes, and other times > solution 2 is clearer. I don't feel too strongly about this
Me neither.
> (because we can't solve the bugger problem, which is C). > I'm fine with not reverting the three patches you > refer to. I'm going to leave this decision to you :-)! > > Thanks, > > Michael > >
I'll keep it as is now. :)
Thanks,
Alex
| |