[musl] [PATCH] Update some address/pointer types

[musl] [PATCH] Update some address/pointer types

[Joe Ramsay]

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

 #ifdef __cplusplus
 }
diff --git a/src/env/__init_tls.c b/src/env/__init_tls.c
index a93141ed..8096bb37 100644
--- a/src/env/__init_tls.c
+++ b/src/env/__init_tls.c
@@ -79,12 +79,12 @@ typedef Elf64_Phdr Phdr;

 extern weak hidden const size_t _DYNAMIC[];

-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;

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

 #endif
diff --git a/src/internal/libc.h b/src/internal/libc.h
index 619bba86..0ee1b074 100644
--- a/src/internal/libc.h
+++ b/src/internal/libc.h
@@ -4,6 +4,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <limits.h>
+#include <stdint.h>

 struct __locale_map;

@@ -23,7 +24,7 @@ struct __libc {
        char secure;
        volatile signed char need_locks;
        int threads_minus_1;
-       size_t *auxv;
+       uintptr_t *auxv;
        struct tls_module *tls_head;
        size_t tls_size, tls_align, tls_cnt;
        size_t page_size;
@@ -38,7 +39,7 @@ extern hidden struct __libc __libc;
 #define libc __libc

 hidden void __init_libc(char **, char *);
-hidden void __init_tls(size_t *);
+hidden void __init_tls(uintptr_t *);
 hidden void __init_ssp(void *);
 hidden void __libc_start_init(void);
 hidden void __funcs_on_exit(void);
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;
 }

diff --git a/src/misc/getauxval.c b/src/misc/getauxval.c
index 57f21eed..45df6e83 100644
--- a/src/misc/getauxval.c
+++ b/src/misc/getauxval.c
@@ -2,7 +2,7 @@
 #include <errno.h>
 #include "libc.h"

-unsigned long __getauxval(unsigned long item)
+uintptr_t __getauxval(unsigned long item)
 {
        size_t *auxv = libc.auxv;
        if (item == AT_SECURE) return libc.secure;
--
2.24.3 (Apple Git-128)


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [musl] [PATCH] Update some address/pointer types

[Rich Felker]

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