Re: [patch] memusage.1, bind.2, eventfd.2, futex.2, open_by_handle_at.2, perf_event_open.2, poll.2, signalfd.2, sysctl.2, timerfd_create.2, bsearch.3, cmsg.3, getaddrinfo.3, getaddrinfo_a.3 getgrouplist.3, insque.3, malloc_info.3, mbsinit.3, mbstowcs.3, pthread_create.3, pthread_setaffinity_np.3, queue.3, rtnetlink.3, shm_open.3, strptime.3, tsearch.3, aio.7, fanotify.7, inotify.7, unix.7: Use sizeof consistently

Michael Kerrisk (man-pages)
2020-08-25T11:41:49Z

Hello Alex,

On Tue, 25 Aug 2020 at 13:34, Alejandro Colomar <colomar.6.4.3@gmail.com> wrote:
>
> Hello Michael & Jakub,
>
> On 8/25/20 12:29 PM, Michael Kerrisk (man-pages) wrote:
> > I would really have preferred three patches here, since:
>
> I can do that.
>
> >
> >> - Never use a space after ``sizeof``, and always use parentheses
> >> instead.
> >>
> >> Rationale:
> >>
> >> https://www.kernel.org/doc/html/v5.8/process/coding-style.html#spaces
> >
> > (1) This is completely unproblematic from my point of view.
>
> Actually there was only one appearance of that (and another one that
> used a space before the parentheses). It's unproblematic, but it's so
> minor that it can be fixed easily.
>
> >> - Use the name of the variable instead of the type as argument
> >> for ``sizeof``, wherever possible.
> >>
> >> Rationale:
> >>
> >>
> https://www.kernel.org/doc/html/v5.8/process/coding-style.html#allocating-memory
> >
> > (2) This one is up for debate. In many cases it makes sense to do
> > this. However, there are cases where I think that using the struct
> > name can actually help readability. And when I grep through the kernel
> > source, of around 139k lines that use "sizeof", some 37k take a
> 'struct type'
> > as an argument. SI, I think this kind of change may need to be
> considered on
> > a case by case basis, rather than as a blanket change.
>
> Ok. I can send a set of patches with a patch for each page.

Okay. Don't do them all at once, since we may change strategy while
discussing the first few patches.

> >> - When the result of ``sizeof`` is multiplied (or otherwise modified),
> >> write ``sizeof`` in the first place.
> >>
> >> Rationale:
> >>
> >> ``(sizeof(x) * INT_MAX * 2)`` doesn't overflow.
> >>
> >> ``(INT_MAX * 2 * sizeof(x))`` overflows, giving incorrect
> >> results.
> >
> > (3) Is this true? "gcc -Wall" does not complain about this. And, I
> > thought that in both cases, all operands in the expression
> > would be promoted to the largest type. And, on my x86-64 system,
> >
> > sizeof((sizeof(x) * INT_MAX * 2)) == 8
> > sizeof(INT_MAX * 2 * sizeof(x)) == 8
> >
> > But, I will say tht I'm not a language lawyer, and C still
> > sometimes has surprises for me. At the least, I'd like to know
> > more about this point.
>
> Well, when I said the first one doesn't overflow, I meant it's much
> less likely.
>
> In C, successive multiplications are evaluated left to right (*), and
> therefore here is what happens:
>
> ``(sizeof(x) * INT_MAX * 2)``:
>
> 1) sizeof(x) * INT_MAX (the type is the largest of both, which is
> size_t (unsigned long; uint64_t)).
> 2) ANS * 2 (the type is again the largest: size_t)
>
> ``(INT_MAX * 2 * sizeof(x))``:
>
> 1) INT_MAX * 2 (the type is the largest of both, which is
> int as both are int (int; int32_t), so the
> result is already truncated as it doesn't fit
> an int; at this point, the intermediate result
> will be 2^32 - 2 (``INT_MAX - 1``) (if I did
> the math right)).
> 2) ANS * 2 (the type is again the largest of both: size_t;
> however, ANS was already incorrect, so the
> result will be an incorrect size_t value)
>
> > sizeof((sizeof(x) * INT_MAX * 2)) == 8
>
> Here you were overflowing a uint64_t (if x were a char, it would not
> overflow, and the result would be close to UINT64_MAX).
>
> > sizeof(INT_MAX * 2 * sizeof(x)) == 8
>
> Here you were overflowing int32_t, and it would overflow regardless of
> sizeof(x).

Thanks for the lesson in C basics. I clearly did need a refresher :-}.

You and Jakub are of course correct.

If you send the patches in the order (as I numbered in my previous reply):

(1)
(3)
(2) as multiple pieces

that would be best, since the first two patches should obviously be
applied, and then we can discuss the last patch(es) case by case.

Thanks,

Michael