[PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Charles Mirabile]

Previously the getauxval function checked for a doubly null entry (i.e.
one whose type and value both were 0) in the auxv array before exiting
the loop.

At least on x86-64, the ABI only specifies that one more long will be
present with value 0 (type AT_NULL) after the pairs of auxv entries.
Whether or not it has a corresponding value is unspecified. This value is
present on linux, but there is no reason to check it as simply seeing an
auxv entry whose type value is AT_NULL should be enough.

This is a matter of taste, but I think processing the data in a structured
way by coercing it into an array of type value pairs, using multiple
return style, and a for loop with a clear exit condition is more readable
than the existing infinite loop with multiple exit points and a return
value variable.

I also added a call to set errno to ENOENT when the entry is not found as
glibc does which allows programs to disambiguate between the case of an
auxv that is not present, and one that is, but with value zero.

Fixes: c61a078015f3 ("nolibc/stdlib: Implement `getauxval(3)` function")
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
---
 tools/include/nolibc/stdlib.h | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index bacfd35c5156..47be99c5a539 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -104,27 +104,19 @@ char *getenv(const char *name)
 static __attribute__((unused))
 unsigned long getauxval(unsigned long type)
 {
-	const unsigned long *auxv = _auxv;
-	unsigned long ret;
-
-	if (!auxv)
-		return 0;
-
-	while (1) {
-		if (!auxv[0] && !auxv[1]) {
-			ret = 0;
-			break;
-		}
-
-		if (auxv[0] == type) {
-			ret = auxv[1];
-			break;
-		}
-
-		auxv += 2;
-	}
-
-	return ret;
+	const struct {
+		unsigned long type, val;
+	} *auxv = (void *)_auxv;
+
+	if (!auxv || !type)
+		goto out;
+
+	for (; auxv->type; ++auxv)
+		if (auxv->type == type)
+			return auxv->val;
+out:
+	SET_ERRNO(ENOENT);
+	return 0;
 }
 
 static __attribute__((unused))
-- 
2.41.0

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Ammar Faizi]

On Tue, Jan 16, 2024 at 01:11:47PM -0500, Charles Mirabile wrote:
> At least on x86-64, the ABI only specifies that one more long will be
> present with value 0 (type AT_NULL) after the pairs of auxv entries.
> Whether or not it has a corresponding value is unspecified. This value is
> present on linux, but there is no reason to check it as simply seeing an
> auxv entry whose type value is AT_NULL should be enough.

Yeah, I agree with that. I just read the ABI and confirmed that the
'a_un' member is ignored when the type is `AT_NULL`. Let's stop relying
on an unspecified value.

For others who want to check, see page 37 and 38:
https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/uploads/221b09355dd540efcbe61b783b6c0ece/x86-64-psABI-2023-09-26.pdf

> This is a matter of taste, but I think processing the data in a structured
> way by coercing it into an array of type value pairs, using multiple
> return style, and a for loop with a clear exit condition is more readable
> than the existing infinite loop with multiple exit points and a return
> value variable.

Ok. It's more readable using your way. One thing that bothers me a bit
is type of 'a_type'. On page 37, the ABI defines the auxv type-val pair
as:

  typedef struct
  {
    int a_type;
    union {
      long a_val;
      void *a_ptr;
      void (*a_fnc)();
    } a_un;
  } auxv_t;

Assuming the arch is x86-64 Linux. Note that 'a_type' is an 'int' which
is 4 bytes in size, but we use 'unsigned long' instead of 'int' to
represent it. However, since 'a_un' needs to be 8 bytes aligned, the
compiler will put a 4 bytes padding between 'a_type' and 'a_un', so it
ends up just fine (on x86-64).

What do you think about other architectures? Will it potentially be
misinterpreted?

I tried to compare it in my head for i386, but it also ends up just
fine. I don't know other architectures.

> I also added a call to set errno to ENOENT when the entry is not found as
> glibc does which allows programs to disambiguate between the case of an
> auxv that is not present, and one that is, but with value zero.

Good catch!

-- 
Ammar Faizi

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Willy Tarreau]

On Wed, Jan 17, 2024 at 01:52:06AM +0700, Ammar Faizi wrote:
> On Tue, Jan 16, 2024 at 01:11:47PM -0500, Charles Mirabile wrote:
> > At least on x86-64, the ABI only specifies that one more long will be
> > present with value 0 (type AT_NULL) after the pairs of auxv entries.
> > Whether or not it has a corresponding value is unspecified. This value is
> > present on linux, but there is no reason to check it as simply seeing an
> > auxv entry whose type value is AT_NULL should be enough.
> 
> Yeah, I agree with that. I just read the ABI and confirmed that the
> 'a_un' member is ignored when the type is `AT_NULL`. Let's stop relying
> on an unspecified value.
> 
> For others who want to check, see page 37 and 38:
> https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/uploads/221b09355dd540efcbe61b783b6c0ece/x86-64-psABI-2023-09-26.pdf
> 
> > This is a matter of taste, but I think processing the data in a structured
> > way by coercing it into an array of type value pairs, using multiple
> > return style, and a for loop with a clear exit condition is more readable
> > than the existing infinite loop with multiple exit points and a return
> > value variable.
> 
> Ok. It's more readable using your way. One thing that bothers me a bit
> is type of 'a_type'. On page 37, the ABI defines the auxv type-val pair
> as:
> 
>   typedef struct
>   {
>     int a_type;
>     union {
>       long a_val;
>       void *a_ptr;
>       void (*a_fnc)();
>     } a_un;
>   } auxv_t;
> 
> Assuming the arch is x86-64 Linux. Note that 'a_type' is an 'int' which
> is 4 bytes in size, but we use 'unsigned long' instead of 'int' to
> represent it. However, since 'a_un' needs to be 8 bytes aligned, the
> compiler will put a 4 bytes padding between 'a_type' and 'a_un', so it
> ends up just fine (on x86-64).
> 
> What do you think about other architectures? Will it potentially be
> misinterpreted?

Indeed, it would fail on a 64-bit big endian architecture. Let's
just declare the local variable the same way as it is in the spec,
it will be much cleaner and more reliable.

Thanks,
Willy

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Willy Tarreau]

On Tue, Jan 16, 2024 at 07:58:09PM +0100, Willy Tarreau wrote:
> On Wed, Jan 17, 2024 at 01:52:06AM +0700, Ammar Faizi wrote:
> > On Tue, Jan 16, 2024 at 01:11:47PM -0500, Charles Mirabile wrote:
> > > At least on x86-64, the ABI only specifies that one more long will be
> > > present with value 0 (type AT_NULL) after the pairs of auxv entries.
> > > Whether or not it has a corresponding value is unspecified. This value is
> > > present on linux, but there is no reason to check it as simply seeing an
> > > auxv entry whose type value is AT_NULL should be enough.
> > 
> > Yeah, I agree with that. I just read the ABI and confirmed that the
> > 'a_un' member is ignored when the type is `AT_NULL`. Let's stop relying
> > on an unspecified value.
> > 
> > For others who want to check, see page 37 and 38:
> > https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/uploads/221b09355dd540efcbe61b783b6c0ece/x86-64-psABI-2023-09-26.pdf
> > 
> > > This is a matter of taste, but I think processing the data in a structured
> > > way by coercing it into an array of type value pairs, using multiple
> > > return style, and a for loop with a clear exit condition is more readable
> > > than the existing infinite loop with multiple exit points and a return
> > > value variable.
> > 
> > Ok. It's more readable using your way. One thing that bothers me a bit
> > is type of 'a_type'. On page 37, the ABI defines the auxv type-val pair
> > as:
> > 
> >   typedef struct
> >   {
> >     int a_type;
> >     union {
> >       long a_val;
> >       void *a_ptr;
> >       void (*a_fnc)();
> >     } a_un;
> >   } auxv_t;
> > 
> > Assuming the arch is x86-64 Linux. Note that 'a_type' is an 'int' which
> > is 4 bytes in size, but we use 'unsigned long' instead of 'int' to
> > represent it. However, since 'a_un' needs to be 8 bytes aligned, the
> > compiler will put a 4 bytes padding between 'a_type' and 'a_un', so it
> > ends up just fine (on x86-64).
> > 
> > What do you think about other architectures? Will it potentially be
> > misinterpreted?
> 
> Indeed, it would fail on a 64-bit big endian architecture. Let's
> just declare the local variable the same way as it is in the spec,
> it will be much cleaner and more reliable.

With that said, if previous code used to work on such architectures,
maybe the definition above is only for x86_64 and differs on other
archs. Maybe it's really defined as two longs ?

Willy

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Ammar Faizi]

On Tue, Jan 16, 2024 at 07:59:39PM +0100, Willy Tarreau wrote:
> On Tue, Jan 16, 2024 at 07:58:09PM +0100, Willy Tarreau wrote:
> > On Wed, Jan 17, 2024 at 01:52:06AM +0700, Ammar Faizi wrote:
> > > What do you think about other architectures? Will it potentially be
> > > misinterpreted?
> > 
> > Indeed, it would fail on a 64-bit big endian architecture. Let's
> > just declare the local variable the same way as it is in the spec,
> > it will be much cleaner and more reliable.
> 
> With that said, if previous code used to work on such architectures,
> maybe the definition above is only for x86_64 and differs on other
> archs. Maybe it's really defined as two longs ?

I just took a look at the kernel source code:
https://github.com/torvalds/linux/blob/v6.7/fs/binfmt_elf.c#L226-L261

The auxv is stored in `elf_info` variable, the type is `elf_addr_t`. Not
sure what kind of typedef is that. I'll check.

Each auxv entry is added using this macro:

 #define NEW_AUX_ENT(id, val) \
         do { \
                 *elf_info++ = id; \
                 *elf_info++ = val; \
         } while (0)

where `id` is the type. That clearly implies `type` and `val` have the
same size on the Linux kernel.

-- 
Ammar Faizi

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Ammar Faizi]

On Wed, Jan 17, 2024 at 02:11:12AM +0700, Ammar Faizi wrote:
> On Tue, Jan 16, 2024 at 07:59:39PM +0100, Willy Tarreau wrote:
> > On Tue, Jan 16, 2024 at 07:58:09PM +0100, Willy Tarreau wrote:
> > > On Wed, Jan 17, 2024 at 01:52:06AM +0700, Ammar Faizi wrote:
> > > > What do you think about other architectures? Will it potentially be
> > > > misinterpreted?
> > > 
> > > Indeed, it would fail on a 64-bit big endian architecture. Let's
> > > just declare the local variable the same way as it is in the spec,
> > > it will be much cleaner and more reliable.
> > 
> > With that said, if previous code used to work on such architectures,
> > maybe the definition above is only for x86_64 and differs on other
> > archs. Maybe it's really defined as two longs ?
> 
> I just took a look at the kernel source code:
> https://github.com/torvalds/linux/blob/v6.7/fs/binfmt_elf.c#L226-L261
> 
> The auxv is stored in `elf_info` variable, the type is `elf_addr_t`. Not
> sure what kind of typedef is that. I'll check.
> 
> Each auxv entry is added using this macro:
> 
>  #define NEW_AUX_ENT(id, val) \
>          do { \
>                  *elf_info++ = id; \
>                  *elf_info++ = val; \
>          } while (0)
> 
> where `id` is the type. That clearly implies `type` and `val` have the
> same size on the Linux kernel.

So here is the result:

1. 'elf_addr_t' defintion ( https://github.com/torvalds/linux/blob/v6.7/include/linux/elf.h#L38-L62 ):

  (simplified)
  #if ELF_CLASS == ELFCLASS32
  #define elf_addr_t	Elf32_Off
  #else
  #define elf_addr_t	Elf64_Off
  #endif

2. 'Elf32_Off' and 'Elf64_Off' typedefs ( https://github.com/torvalds/linux/blob/v6.7/include/uapi/linux/elf.h#L8-L23 )

  typedef __u32  Elf32_Off;
  typedef __u64  Elf64_Off;

Assuming 'ELFCLASS32' is for 32-bit architectures, then it's two __u64
on 64-bit arch, and two __u32 on 32-bit arch. That is identical to
'unsigned long' for both cases (on Linux). So it's fine to have
'unsigned long' for both 'type' and 'value'.

-- 
Ammar Faizi

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Willy Tarreau]

On Wed, Jan 17, 2024 at 02:23:53AM +0700, Ammar Faizi wrote:
> On Wed, Jan 17, 2024 at 02:11:12AM +0700, Ammar Faizi wrote:
> > On Tue, Jan 16, 2024 at 07:59:39PM +0100, Willy Tarreau wrote:
> > > On Tue, Jan 16, 2024 at 07:58:09PM +0100, Willy Tarreau wrote:
> > > > On Wed, Jan 17, 2024 at 01:52:06AM +0700, Ammar Faizi wrote:
> > > > > What do you think about other architectures? Will it potentially be
> > > > > misinterpreted?
> > > > 
> > > > Indeed, it would fail on a 64-bit big endian architecture. Let's
> > > > just declare the local variable the same way as it is in the spec,
> > > > it will be much cleaner and more reliable.
> > > 
> > > With that said, if previous code used to work on such architectures,
> > > maybe the definition above is only for x86_64 and differs on other
> > > archs. Maybe it's really defined as two longs ?
> > 
> > I just took a look at the kernel source code:
> > https://github.com/torvalds/linux/blob/v6.7/fs/binfmt_elf.c#L226-L261
> > 
> > The auxv is stored in `elf_info` variable, the type is `elf_addr_t`. Not
> > sure what kind of typedef is that. I'll check.
> > 
> > Each auxv entry is added using this macro:
> > 
> >  #define NEW_AUX_ENT(id, val) \
> >          do { \
> >                  *elf_info++ = id; \
> >                  *elf_info++ = val; \
> >          } while (0)
> > 
> > where `id` is the type. That clearly implies `type` and `val` have the
> > same size on the Linux kernel.
> 
> So here is the result:
> 
> 1. 'elf_addr_t' defintion ( https://github.com/torvalds/linux/blob/v6.7/include/linux/elf.h#L38-L62 ):
> 
>   (simplified)
>   #if ELF_CLASS == ELFCLASS32
>   #define elf_addr_t	Elf32_Off
>   #else
>   #define elf_addr_t	Elf64_Off
>   #endif
> 
> 2. 'Elf32_Off' and 'Elf64_Off' typedefs ( https://github.com/torvalds/linux/blob/v6.7/include/uapi/linux/elf.h#L8-L23 )
> 
>   typedef __u32  Elf32_Off;
>   typedef __u64  Elf64_Off;
> 
> Assuming 'ELFCLASS32' is for 32-bit architectures, then it's two __u64
> on 64-bit arch, and two __u32 on 32-bit arch. That is identical to
> 'unsigned long' for both cases (on Linux). So it's fine to have
> 'unsigned long' for both 'type' and 'value'.

Yeah I agree, thanks for checking.

Willy

Re: [PATCH] nolibc/stdlib: Improve `getauxval(3)` implementation

[Ammar Faizi]

On Tue, Jan 16, 2024 at 08:46:55PM +0100, Willy Tarreau wrote:
> Yeah I agree, thanks for checking.

Reviewed-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

-- 
Ammar Faizi