From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.2 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by inbox.vuxu.org (OpenSMTPD) with SMTP id 00133a13 for ; Sun, 19 Jan 2020 13:34:01 +0000 (UTC) Received: (qmail 16158 invoked by uid 550); 19 Jan 2020 13:33:59 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 16134 invoked from network); 19 Jan 2020 13:33:59 -0000 To: musl@lists.openwall.com References: <20200119110743.GD2020@voyager> <20200119113134.GJ23985@port70.net> From: Alexander Cherepanov Message-ID: <8299f261-7870-57a6-37cf-d4ce482ad81e@openwall.com> Date: Sun, 19 Jan 2020 16:33:47 +0300 MIME-Version: 1.0 In-Reply-To: <20200119113134.GJ23985@port70.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [musl] Minor style patch to exit.c On 19/01/2020 14.31, Szabolcs Nagy wrote: > * Markus Wichmann [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