On Sun, Oct 18, 2026 at 6:09 PM Mark Wielaard <[email protected]> wrote: > > 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.
Pushed with the redundant assert removed and the comment reworded. Aaron > > Thanks, > > Mark >
