Hi Anton,
On Sat, Feb 01, 2025 at 02:21:24AM +0300, Anton Moryakov wrote:
> Report of the static analyzer:
> Pointer, returned from function 'elf_getarhdr' at
> objdump.c:314, may be NULL and is dereferenced at
> objdump.c:317. (CWE476, CWE690)
Nice catch.
> Corrections explained:
> When processing archive elements, the code could dereference
> a NULL pointer if 'elf_getarhdr' returns NULL. This patch adds
> a check to ensure 'arhdr' is not NULL before using it.
>
> The fix ensures that the function safely handles cases where
> 'elf_getarhdr' fails, avoiding potential crashes.
>
> Triggers found by static analyzer Svace.
>
> Signed-off-by: Anton Moryakov <ant.v.moryakov at gmail.com>
>
> ---
> src/objdump.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/objdump.c b/src/objdump.c
> index 1b38da23..9a66d362 100644
> --- a/src/objdump.c
> +++ b/src/objdump.c
> @@ -312,6 +312,13 @@ 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 (arhdr == NULL)
> + {
> + error(0, 0, _("%s: failed to get archive header"), fname);
> + result = 1;
> + continue;
> + }
> +
I think this is wrong. You don't do any cleanup of the elf and the
continue seems to just recreate the same subelf, so eventually you run
out of memory.
It is probably simpler to just add the check here:
> /* Skip over the index entries. */
> if (strcmp (arhdr->ar_name, "/") != 0
> && strcmp (arhdr->ar_name, "//") != 0)
if (arhdr != NULL
&& strcmp (arhdr->ar_name, "/") != 0
&& strcmp (arhdr->ar_name, "//") != 0)
Cheers,
Mark
