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

[Alejandro Colomar <colomar.6.4.3@gmail.com>]

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.

>
>> - 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).


I wrote an extreme case, but we can agree that 32 bit is much easier to
overflow than 64.


(*): https://en.cppreference.com/w/c/language/operator_precedence


On 8/25/20 1:19 PM, Jakub Wilk wrote:
> * Michael Kerrisk <mtk.manpages@gmail.com>, 2020-08-25, 12:29:
>>>     ``(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.
>
> My GCC (10.2.0) does, even without -Wall:
>
>   $ gcc test-overflow.c
>   test-overflow.c: In function 'main':
>   test-overflow.c:8:52: warning: integer overflow in expression of type
> 'int' results in '-2' [-Woverflow]
>       8 |  printf("INT_MAX * 2 * sizeof(x) = %zu\n", INT_MAX * 2 *
> sizeof(x));
>         |                                                    ^
I guess it will only complain for the first one, doesn't it?

>
>> sizeof((sizeof(x) * INT_MAX * 2)) == 8
>> sizeof(INT_MAX * 2 * sizeof(x)) == 8
>
> Hmm? If there was no overflow, surely you should get a number larger
> than INT_MAX...
>

INT_MAX is INT32_MAX, and given that size_t is 64 bit, sure we can, but
only in the first case.