Re: [PATCH] objdump.c: potential NULL pointer dereference in handle_ar

[Mark Wielaard]

Hi Anton,

On Wed, 2024-10-23 at 14:14 +0300, ant.v.morya...@gmail.com wrote:
> From: AntonMoryakov <ant.v.morya...@gmail.com>
> 
> - Added a check to ensure `arhdr` is not NULL before using it in `strcmp` to 
> avoid segmentation fault.
> - This resolves the issue where the pointer returned from `elf_getarhdr` may 
> be NULL and causes a crash when dereferenced.

Kind of the same questions/comments as for previous patches. Please
sign your work, keep with current coding style and if you have a
testcase that would be ideal.

> ---
>  src/objdump.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/objdump.c b/src/objdump.c
> index 1b38da23..33b6fec5 100644
> --- a/src/objdump.c
> +++ b/src/objdump.c
> @@ -313,7 +313,8 @@ handle_ar (int fd, Elf *elf, const char *prefix, const 
> char *fname,
>        Elf_Arhdr *arhdr = elf_getarhdr (subelf);
>  
>        /* Skip over the index entries.  */
> -      if (strcmp (arhdr->ar_name, "/") != 0
> +      if (arhdr != NULL
> +    && strcmp (arhdr->ar_name, "/") != 0
>         && strcmp (arhdr->ar_name, "//") != 0)
>       {
>         if (elf_kind (subelf) == ELF_K_ELF)

I think it would be better to do a separate check for arhdr being NULL,
that should normally not happen imho. Then do an INTERNAL_ERROR (fname)
to stop processing and report the libelf error. e.g something like:

diff --git a/src/objdump.c b/src/objdump.c
index 1b38da23266d..94cc69cb0f6a 100644
--- a/src/objdump.c
+++ b/src/objdump.c
@@ -311,6 +311,8 @@ handle_ar (int fd, Elf *elf, const char *prefix,
const char *fname,
     {
       /* The the header for this element.  */
       Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+      if (ahdr == NULL)
+       INTERNAL_ERROR (fname);
 
       /* Skip over the index entries.  */
       if (strcmp (arhdr->ar_name, "/") != 0

Cheers,

Mark