Hi Anton,
On Thu, Feb 27, 2025 at 10:02:19PM +0100, Mark Wielaard wrote:
> On Thu, Feb 13, 2025 at 07:52:00PM +0300, Anton Moryakov wrote:
> > Static analyzer reported:
> > Return value of a function 'gelf_getehdr' is dereferenced at readelf.c:12443
> > without checking for NULL, but it is usually checked for this function
> > (53/54).
>
> I can see how a static analyzer thinks this gelf_getehdr call can
> fail. But it really cannot in this case. The core Elf was already
> checked for ehdr->type == ET_CORE in one of the callers. Or we
> wouldn't have gotten here.
>
> What you could do to help the analyzer, and make this code slightly
> cleaner, is pass down that ehdr from handle_notes through the various
> handle_* functions.
I implemented this and puhsed it as attached.
Cheers,
Mark
>From 56e7a862d82e646bae79706343afa5f7800e2be1 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@klomp.org>
Date: Tue, 29 Apr 2025 23:19:47 +0200
Subject: [PATCH 2/3] readelf: Pass around GElf_Ehdr instead of calling
gelf_getehdr
handle_core_item calls gelf_getehdr for each item without checking if
the call succeeds. It should always succeed because process_elf_file
already checked gelf_getehdr returned a valid Ehdr before passing it
to handle_notes. Just pass the Ehdr down a couple more function calls.
* readelf.c (handle_core_item): Take const Gelf_Ehdr and use it.
(handle_core_items): Take const Gelf_Ehdr and pass it to
handle_core_item.
(handle_core_note): Take const Gelf_Ehdr and pass it to
handle_core_items.
(handle_notes_data): Pass ehdr to handle_core_note.
Signed-off-by: Mark Wielaard <m...@klomp.org>
---
src/readelf.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/readelf.c b/src/readelf.c
index 5b0e7b474064..8603b3c441a1 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -12322,7 +12322,8 @@ convert (Elf *core, Elf_Type type, uint_fast16_t count,
typedef uint8_t GElf_Byte;
static unsigned int
-handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
+handle_core_item (Elf *core, const GElf_Ehdr *ehdr,
+ const Ebl_Core_Item *item, const void *desc,
unsigned int colno, size_t *repeated_size)
{
uint_fast16_t count = item->count ?: 1;
@@ -12501,8 +12502,6 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item,
const void *desc,
high half is the padding; it's presumably zero, but should
be ignored anyway. For big-endian, it means the 32-bit
field went into the high half of USEC. */
- GElf_Ehdr ehdr_mem;
- GElf_Ehdr *ehdr = gelf_getehdr (core, &ehdr_mem);
if (likely (ehdr->e_ident[EI_DATA] == ELFDATA2MSB))
usec >>= 32;
else
@@ -12594,7 +12593,8 @@ compare_core_item_groups (const void *a, const void *b)
}
static unsigned int
-handle_core_items (Elf *core, const void *desc, size_t descsz,
+handle_core_items (Elf *core, const GElf_Ehdr *ehdr,
+ const void *desc, size_t descsz,
const Ebl_Core_Item *items, size_t nitems)
{
if (nitems == 0)
@@ -12610,7 +12610,7 @@ handle_core_items (Elf *core, const void *desc, size_t
descsz,
{
assert (items[0].offset == 0);
size_t size = descsz;
- colno = handle_core_item (core, items, desc, colno, &size);
+ colno = handle_core_item (core, ehdr, items, desc, colno, &size);
/* If SIZE is not zero here there is some remaining data. But we do not
know how to process it anyway. */
return colno;
@@ -12645,7 +12645,7 @@ handle_core_items (Elf *core, const void *desc, size_t
descsz,
&& ((*item)->group == groups[i][0]->group
|| !strcmp ((*item)->group, groups[i][0]->group)));
++item)
- colno = handle_core_item (core, *item, desc, colno, NULL);
+ colno = handle_core_item (core, ehdr, *item, desc, colno, NULL);
/* Force a line break at the end of the group. */
colno = WRAP_COLUMN;
@@ -13146,7 +13146,7 @@ handle_file_note (Elf *core, GElf_Word descsz, GElf_Off
desc_pos)
}
static void
-handle_core_note (Ebl *ebl, const GElf_Nhdr *nhdr,
+handle_core_note (Ebl *ebl, const GElf_Ehdr *ehdr, const GElf_Nhdr *nhdr,
const char *name, const void *desc)
{
GElf_Word regs_offset;
@@ -13163,7 +13163,7 @@ handle_core_note (Ebl *ebl, const GElf_Nhdr *nhdr,
so that the ITEMS array does not describe the whole thing.
For non-register notes, the actual descsz might be a multiple
of the unit size, not just exactly the unit size. */
- unsigned int colno = handle_core_items (ebl->elf, desc,
+ unsigned int colno = handle_core_items (ebl->elf, ehdr, desc,
nregloc == 0 ? nhdr->n_descsz : 0,
items, nitems);
if (colno != 0)
@@ -13243,10 +13243,10 @@ handle_notes_data (Ebl *ebl, const GElf_Ehdr *ehdr,
break;
default:
- handle_core_note (ebl, &nhdr, name, desc);
+ handle_core_note (ebl, ehdr, &nhdr, name, desc);
}
else
- handle_core_note (ebl, &nhdr, name, desc);
+ handle_core_note (ebl, ehdr, &nhdr, name, desc);
}
else
ebl_object_note (ebl, nhdr.n_namesz, name, nhdr.n_type,
--
2.49.0
