[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 19 May 2021 10:51:39 -0400
From: Rich Felker <dalias@...c.org>
To: Joe Ramsay <Joe.Ramsay@....com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: [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 <elf.h>
> #include <bits/hwcap.h>
>
> -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 <features.h>
>
> -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 <unistd.h>
> #include <elf.h>
> #include <string.h>
> +#include <sys/auxv.h>
> #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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
