From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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.4 Received: (qmail 20786 invoked from network); 19 May 2021 14:51:55 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 19 May 2021 14:51:55 -0000 Received: (qmail 24060 invoked by uid 550); 19 May 2021 14:51:52 -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 24042 invoked from network); 19 May 2021 14:51:52 -0000 Date: Wed, 19 May 2021 10:51:39 -0400 From: Rich Felker To: Joe Ramsay Cc: "musl@lists.openwall.com" Message-ID: <20210519145139.GP2546@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] Update some address/pointer types On Wed, May 19, 2021 at 08:42:13AM +0000, Joe Ramsay wrote: > Hi all, > > This patch corrects the use of long/size_t types where it would have > been more appropriate to use uintptr_t, as well as changing uintptr_t > to be based on the compiler-defined __UINTPTR_TYPE__ (and intptr_t to > __INTPTR_TYPE__). The conflation of these types is historically not a > problem, as they are generally all the same size, however a more > precise definition will be necessary for implementations where the > pointer size can be more than 8 bytes. > > This patch has been tested against libc-test with gcc 7 on x86 and > clang 9 on AArch64, with no new failures. > > Thanks, > Joe > --- > include/alltypes.h.in | 4 ++-- > include/sys/auxv.h | 2 +- > src/env/__init_tls.c | 4 ++-- > src/env/__libc_start_main.c | 3 ++- > src/include/sys/auxv.h | 2 +- > src/internal/libc.h | 5 +++-- > src/malloc/mallocng/glue.h | 8 ++++---- > src/misc/getauxval.c | 2 +- > 8 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/include/alltypes.h.in b/include/alltypes.h.in > index d47aeea9..07f13973 100644 > --- a/include/alltypes.h.in > +++ b/include/alltypes.h.in > @@ -3,10 +3,10 @@ > #define __USE_TIME_BITS64 1 > > TYPEDEF unsigned _Addr size_t; > -TYPEDEF unsigned _Addr uintptr_t; > +TYPEDEF __UINTPTR_TYPE__ uintptr_t; > TYPEDEF _Addr ptrdiff_t; > TYPEDEF _Addr ssize_t; > -TYPEDEF _Addr intptr_t; > +TYPEDEF __INTPTR_TYPE__ intptr_t; > TYPEDEF _Addr regoff_t; > TYPEDEF _Reg register_t; > TYPEDEF _Int64 time_t; We don't depend on compiler-specific predefined macros like this. The _Addr macro from the arch's alltypes.h.in fragment already defines the right type for each arch ABI. If you're doing an out-of-tree arch that does something weird here, that's the right place to put the change. > diff --git a/include/sys/auxv.h b/include/sys/auxv.h > index ddccf57f..192ebe64 100644 > --- a/include/sys/auxv.h > +++ b/include/sys/auxv.h > @@ -8,7 +8,7 @@ extern "C" { > #include > #include > > -unsigned long getauxval(unsigned long); > +uintptr_t getauxval(unsigned long); This is a documented signature of a public function and cannot be changed. > -static void static_init_tls(size_t *aux) > +static void static_init_tls(uintptr_t *aux) > { > unsigned char *p; > size_t n; > Phdr *phdr, *tls_phdr=0; > - size_t base = 0; > + uintptr_t base = 0; > void *mem; Internally, size_t is assumed to cover the entire possible address space, and used interchangibly with uintptr_t in some places. This should probably be cleaned up to be more consistent, but any such cleanup needs to be isolated to not making invalid changes elsewhere, and needs review for correctness. > for (p=(void *)aux[AT_PHDR],n=aux[AT_PHNUM]; n; n--,p+=aux[AT_PHENT]) { > diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c > index c5b277bd..ec732b10 100644 > --- a/src/env/__libc_start_main.c > +++ b/src/env/__libc_start_main.c > @@ -22,7 +22,8 @@ __attribute__((__noinline__)) > #endif > void __init_libc(char **envp, char *pn) > { > - size_t i, *auxv, aux[AUX_CNT] = { 0 }; > + size_t i = 0; > + uintptr_t *auxv, aux[AUX_CNT] = { 0 }; > __environ = envp; > for (i=0; envp[i]; i++); > libc.auxv = auxv = (void *)(envp+i+1); This introduces a change in initialization for i. Also auxv is an external ABI boundary with the ELF entry point/kernel and should probably use a type matching that specification, if any change is made. > diff --git a/src/include/sys/auxv.h b/src/include/sys/auxv.h > index 9358a4a5..a7fb201d 100644 > --- a/src/include/sys/auxv.h > +++ b/src/include/sys/auxv.h > @@ -5,6 +5,6 @@ > > #include > > -hidden unsigned long __getauxval(unsigned long); > +hidden uintptr_t __getauxval(unsigned long); Since it's an alias, this has to match the type of the public function aliased. > diff --git a/src/malloc/mallocng/glue.h b/src/malloc/mallocng/glue.h > index 151c48b8..fe158e4f 100644 > --- a/src/malloc/mallocng/glue.h > +++ b/src/malloc/mallocng/glue.h > @@ -7,6 +7,7 @@ > #include > #include > #include > +#include > #include "atomic.h" > #include "syscall.h" > #include "libc.h" > @@ -41,10 +42,9 @@ > > static inline uint64_t get_random_secret() > { > - uint64_t secret = (uintptr_t)&secret * 1103515245; > - for (size_t i=0; libc.auxv[i]; i+=2) > - if (libc.auxv[i]==AT_RANDOM) > - memcpy(&secret, (char *)libc.auxv[i+1]+8, sizeof secret); > + uint64_t secret = (size_t)&secret * 1103515245; > + uintptr_t random = getauxval(AT_RANDOM); > + if (random) secret = *((uint64_t*)((char*)random + 8)); > return secret; > } This is a namespace violation. It could have been done with the namespace-safe alias __getauxval, but I believe there was some reason it wasn't, perhaps just for size considerations since this is code that will almost always be linked. Rich