Re: [PATCH] libdwfl: Rewrite reading of ar_size in elf_begin_rand

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2022-07-28 09:48, Mark Wielaard wrote:
> With GCC 12.1.1, glibc 2.3a, -fsanitize=undefined and
> -D_FORTIFY_SOURCE=3 we get the following error message:
> 
> In file included from /usr/include/ar.h:22,
>                   from ../libelf/libelfP.h:33,
>                   from core-file.c:31:
> In function ‘pread’,
>      inlined from ‘pread_retry’ at ../lib/system.h:188:21,
>      inlined from ‘elf_begin_rand’ at core-file.c:86:16,
>      inlined from ‘core_file_read_eagerly’ at core-file.c:205:15:
> /usr/include/bits/unistd.h:74:10: error: ‘__pread_alias’ writing 58 or more bytes into a region of size 10 overflows the destination [-Werror=stringop-overflow=]
>     74 |   return __glibc_fortify (pread, __nbytes, sizeof (char),
>        |          ^~~~~~~~~~~~~~~
> /usr/include/ar.h: In function ‘core_file_read_eagerly’:
> /usr/include/ar.h:41:10: note: destination object ‘ar_size’ of size 10
>     41 |     char ar_size[10];           /* File size, in ASCII decimal.  */
>        |          ^~~~~~~
> /usr/include/bits/unistd.h:50:16: note: in a call to function ‘__pread_alias’ declared with attribute ‘access (write_only, 2, 3)’
>     50 | extern ssize_t __REDIRECT (__pread_alias,
>        |                ^~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> The warning disappears when dropping either -fsanitize=undefined
> or when using -D_FORTIFY_SOURCE=2. It looks like a false positive.
> But I haven't figured out how/why it happens.

Interesting, I'll take a closer look at this from the gcc context.  I 
obviously don't have any strong opinions about the elfutils patch :)

Thanks,
Sid

> The code is a little tricky to proof correct though. The ar_size
> field is a not-zero terminated string ASCII decimal, right-paddedr
> with spaces. Which is then converted with strtoll. Relying on the
> fact that the struct ar_hdr is zero initialized, so there will be
> a zero byte after the ar_size field.
> 
> Rewrite the code to just use a zero byte terminated char array.
> Which is much easier to reason about. As a bonus the error disappears.
> 
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> ---
>   libdwfl/ChangeLog   |  5 +++++
>   libdwfl/core-file.c | 26 ++++++++++++++++----------
>   2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index 75c53948..acdaa013 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-07-28  Mark Wielaard  <mark@klomp.org>
> +
> +	* core-file.c (elf_begin_rand): Replace struct ar_hdr h with
> +	a char ar_size[AR_SIZE_CHARS + 1] array to read size.
> +
>   2022-07-18  Shahab Vahedi  <shahab@synopsys.com>
>   
>   	* debuginfod-client.c (dwfl_get_debuginfod_client stub):
> diff --git a/libdwfl/core-file.c b/libdwfl/core-file.c
> index cefc3db0..4418ef33 100644
> --- a/libdwfl/core-file.c
> +++ b/libdwfl/core-file.c
> @@ -75,26 +75,32 @@ elf_begin_rand (Elf *parent, off_t offset, off_t size, off_t *next)
>        from the archive header to override SIZE.  */
>     if (parent->kind == ELF_K_AR)
>       {
> -      struct ar_hdr h = { .ar_size = "" };
> -
> -      if (unlikely (parent->maximum_size - offset < sizeof h))
> +      /* File size, in ASCII decimal, right-padded with ASCII spaces.
> +         Max 10 characters. Not zero terminated. So make this ar_size
> +         array one larger and explicitly zero terminate it.  As needed
> +         for strtoll.  */
> +      #define AR_SIZE_CHARS 10
> +      char ar_size[AR_SIZE_CHARS + 1];
> +      ar_size[AR_SIZE_CHARS] = '\0';
> +
> +      if (unlikely (parent->maximum_size - offset < sizeof (struct ar_hdr)))
>   	return fail (ELF_E_RANGE);
>   
>         if (parent->map_address != NULL)
> -	memcpy (h.ar_size, parent->map_address + parent->start_offset + offset,
> -		sizeof h.ar_size);
> +	memcpy (ar_size, parent->map_address + parent->start_offset + offset,
> +		AR_SIZE_CHARS);
>         else if (unlikely (pread_retry (parent->fildes,
> -				      h.ar_size, sizeof (h.ar_size),
> +				      ar_size, AR_SIZE_CHARS,
>   				      parent->start_offset + offset
>   				      + offsetof (struct ar_hdr, ar_size))
> -			 != sizeof (h.ar_size)))
> +			 != AR_SIZE_CHARS))
>   	return fail (ELF_E_READ_ERROR);
>   
> -      offset += sizeof h;
> +      offset += sizeof (struct ar_hdr);
>   
>         char *endp;
> -      size = strtoll (h.ar_size, &endp, 10);
> -      if (unlikely (endp == h.ar_size)
> +      size = strtoll (ar_size, &endp, 10);
> +      if (unlikely (endp == ar_size)
>   	  || unlikely ((off_t) parent->maximum_size - offset < size))
>   	return fail (ELF_E_INVALID_ARCHIVE);
>       }