Re: [musl] Minor style patch to exit.c

[Alexander Cherepanov <ch3root@openwall.com>]

On 19/01/2020 14.31, Szabolcs Nagy wrote:
> * Markus Wichmann <nullplan@gmx.net> [2020-01-19 12:07:43 +0100]:
>> The previous version did have a maze of parentheses here. But the actual
>> logic of the function is not to iterate over some numbers that happen to
>> be convertible to pointers, but rather to iterate over an array of
>> function pointers. So let us use pointer arithmetic correctly.
>> ---
>>   src/exit/exit.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/exit/exit.c b/src/exit/exit.c
>> index a6869b37..e02ba2be 100644
>> --- a/src/exit/exit.c
>> +++ b/src/exit/exit.c
>> @@ -16,9 +16,9 @@ extern weak hidden void (*const __fini_array_start)(void), (*const __fini_array_
>>
>>   static void libc_exit_fini(void)
>>   {
>> -	uintptr_t a = (uintptr_t)&__fini_array_end;
>> -	for (; a>(uintptr_t)&__fini_array_start; a-=sizeof(void(*)()))
>> -		(*(void (**)())(a-sizeof(void(*)())))();
>> +	void (*const *a)() = &__fini_array_end;
>> +	while (a > &__fini_array_start)
>> +		(*--a)();

[This is part of my reply was sent to Markus off-list by mistake, sorry 
for the dup.]

Iteration over pointers like this is wrong C (even if it's somewhat 
disputed) and was miscompiled by gcc at some point -- for example you 
can follow links from 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61502#c19 (see also the 
next comment there for a reply from a gcc developer).

> this has undefined behaviour.
> 
> the original code was carefully written to avoid that.
> you need to keep the uintptr_t cast since -- and > are
> undefined for pointers that go out of bound or don't
> point to the same object (and the _start, _end symbols
> don't represent the same c language object, they are
> independent).

I think this only solves a part of the problem.

1. Casts from integers to pointers are implementation-defined and only 
guaranteed to give sane results when those integers are in turn casted 
from pointers. Arithmetic on integers between casts is outside any 
guarantees. It would be funny if not for the next issue.

2. As is defined, not only _start and _end symbols represent different 
objects, locations between them represent different objects too. 
Starting with a pointer to one object and iterating over integers until 
you get to another object doesn't give you a valid pointer to the second 
object, at least in gcc -- see 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65752 .

Couldn't _start defined as an array? Then separate values could be 
accessed simply as elements of this array. And casts to integers could 
be limited to calculating the number of elements, the terminating value 
or something.

Beware though, even if you get it perfect from the C POV gcc has some 
bugs in this area, e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49330 .

-- 
Alexander Cherepanov