Hi Mark,
On Tue, Sep 9, 2025 at 10:24 AM Mark Wielaard <[email protected]> wrote:
>
> Hi Aaron,
>
> On Sun, 2025-09-07 at 22:17 -0400, Aaron Merey wrote:
> > If elf_getarhdr is called on a descriptor that refers to an archive that
> > is itself a member of an outer archive, it may return the Elf_Arhdr of
> > the current member of the inner archive instead of Elf_Arhdr of the inner
> > archive itself.
> >
> > This also causes a memory leak: elf_end only attempts to free
> > Elf_Arhdr fields ar_name and ar_rawname for descriptors that are not
> > ELF_K_AR.
> >
> > Fix this by adding a new field ar_ar_hdr to Elf->state.ar. The
> > ar_ar_hdr field stores the Elf_Arhdr of an archive which is itself a member
> > of an archive. elf_getarhdr now returns ar_ar_hdr for ELF_K_AR descriptors
> > associated with a parent archive and elf_end will free ar_ar_hdr.ar_name and
> > ar_ar_hdr.ar_rawname.
>
> The naming confused me a little. ar_ar_hdr for the ar union member
> plays the same role as the elf_ar_hdr for the elf[32|64] union members.
> But the ar.elf_ar_hdr field us used for a different purpose (holding
> the current/offset ar_hdr).
>
> It might make sense to rename these and even maybe move the elf_ar_hdr
> /ar_ar_hdr out of the union members and just have them as part of the
> Elf struct itself because now every Elf has an "am I an Elf ar member"
> field.
>
> Maybe do this after you added more tests though and after playing with
> pahole to see if there is an ideal layout of the struct/union members.
I agree that it's better to combine state.elf[32|64].elf_ar_hdr and
state.ar.ar_ar_hdr into one struct Elf field outside of elf->state.
It's less confusing and we can set/get elf_ar_hdr without needing to
check elf->kind, like you said.
I used pahole to compare this suggested layout with the layout given
in this patch (new ar_ar_hdr field in state.ar). There is no
difference in the struct Elf size or the amount of padding:
with --enable-thread-safety:
/* size: 368, cachelines: 6, members: 14 */
/* last cacheline: 48 bytes */
with --disable-thread-safety:
/* size: 312, cachelines: 5, members: 13 */
/* sum members: 308, holes: 1, sum holes: 4 */
/* last cacheline: 56 bytes */
I will revise this patch and add more tests.
Aaron
