Hi Aaron, On Wed, Sep 30, 2026 at 03:06:50PM -0400, Aaron Merey wrote: > __libelf_readall reads an entire image into memory and sets > ELF_F_MALLOCED for the associated elf descriptor. For ELF_K_AR > descriptors, member/child descriptors do not have ELF_F_MALLOCED > set even though their image is in a malloced buffer. > > This causes an assert failure in elf32_getshdr.c:load_shdr_wrlock > when attempting to read an shdr from an archive member. The function > asserts that ELF_F_MALLOCED is set because the member descriptor's > map_address is non-NULL. > > Fix this by introducing a new internal flag ELF_F_PARENT_MALLOCED > to indicate that this descriptor's image has been allocated but the > memory is owned by a parent descriptor.
This seems to work. But the asserts look somewhat overcomplicated. So not really a fan. But also not opposed. > Signed-off-by: Aaron Merey <[email protected]> > --- > libelf/elf32_getshdr.c | 7 ++++--- > libelf/elf_readall.c | 4 ++++ > libelf/libelfP.h | 3 ++- > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/libelf/elf32_getshdr.c b/libelf/elf32_getshdr.c > index e4bebe18..7c6c2acb 100644 > --- a/libelf/elf32_getshdr.c > +++ b/libelf/elf32_getshdr.c > @@ -91,11 +91,12 @@ load_shdr_wrlock (Elf_Scn *scn) > /* All the data is already mapped. If we could use it > directly this would already have happened. Unless > we allocated the memory ourselves and the ELF_F_MALLOCED > - flag is set. */ > + flag is set, or a parent archive allocated the memory > + and ELF_F_PARENT_MALLOCED is set. */ > void *file_shdr = ((char *) elf->map_address > + elf->start_offset + ehdr->e_shoff); > > - assert ((elf->flags & ELF_F_MALLOCED) > + assert ((elf->flags & (ELF_F_MALLOCED | ELF_F_PARENT_MALLOCED)) > || ehdr->e_ident[EI_DATA] != MY_ELFDATA > || elf->cmd == ELF_C_READ_MMAP > || (! ALLOW_UNALIGNED Good comment and assert update. > @@ -105,7 +106,7 @@ load_shdr_wrlock (Elf_Scn *scn) > /* Now copy the data and at the same time convert the byte order. */ > if (ehdr->e_ident[EI_DATA] == MY_ELFDATA) > { > - assert ((elf->flags & ELF_F_MALLOCED) > + assert ((elf->flags & (ELF_F_MALLOCED | ELF_F_PARENT_MALLOCED)) > || elf->cmd == ELF_C_READ_MMAP > || ! ALLOW_UNALIGNED); > memcpy (shdr, file_shdr, size); OK, same assert update. Existing issue. But it does feel this assert is somewhat redundant, it would only trigger if the assert 5 lines earlier would trigger. > diff --git a/libelf/elf_readall.c b/libelf/elf_readall.c > index 4ef8fe97..2e3a3c6f 100644 > --- a/libelf/elf_readall.c > +++ b/libelf/elf_readall.c > @@ -49,8 +49,12 @@ set_address (Elf *elf, size_t offset) > { > if (child->map_address == NULL) > { > + /* The archive image is malloced but owned by the parent > + archive, so the child must not free it. */ > + child->flags |= ELF_F_PARENT_MALLOCED; > child->map_address = elf->map_address; > child->start_offset -= offset; > + Yes, but I think the comment is slightly misleading. This really is about making sure some invariants hold (not mmapped, so malloc directly or through parent), it hasn't to do with freeing the memory. > if (child->kind == ELF_K_AR) > child->state.ar.offset -= offset; > > diff --git a/libelf/libelfP.h b/libelf/libelfP.h > index 2403d796..3f351553 100644 > --- a/libelf/libelfP.h > +++ b/libelf/libelfP.h > @@ -83,7 +83,8 @@ enum > { > ELF_F_MMAPPED = 0x40, > ELF_F_MALLOCED = 0x80, > - ELF_F_FILEDATA = 0x100 > + ELF_F_FILEDATA = 0x100, > + ELF_F_PARENT_MALLOCED = 0x200 > }; 0x200 is the next "free" flag value. I like the comment to be reworded, but that is really just a nitpick, so feel free to commit as is. Thanks, Mark
