Re: [PATCH v5 12/18] dump/dump: Add section string table support

[Janosch Frank]

On 8/30/22 13:35, Steffen Eiden wrote:
> Hi Janosch,
>
> On 8/11/22 14:11, Janosch Frank wrote:
> > As sections don't have a type like the notes do we need another way to
> > determine their contents. The string table allows us to assign each
> > section an identification string which architectures can then use to
> > tag their sections with.
> >
> > There will be no string table if the architecture doesn't add custom
> > sections which are introduced in a following patch.
> >
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >    dump/dump.c           | 71 +++++++++++++++++++++++++++++++++++++++++++
> >    include/sysemu/dump.h |  4 +++
> >    2 files changed, 75 insertions(+)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index 31eb20108c..0d6dbf453a 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> [ snip ]
> >    }
> >    
> > +static void prepare_elf_section_hdr_string(DumpState *s, void *buff)
> > +{
> > +    Elf32_Shdr shdr32;
> > +    Elf64_Shdr shdr64;
> > +    int shdr_size;
> > +    void *shdr;
> > +
> > +    if (dump_is_64bit(s)) {
> > +        shdr_size = sizeof(Elf64_Shdr);
> > +        memset(&shdr64, 0, shdr_size);
> > +        shdr64.sh_type = SHT_STRTAB;
> > +        shdr64.sh_offset = s->section_offset + s->elf_section_data_size;
> > +        shdr64.sh_name = s->string_table_buf->len;
> > +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> I think you mixed up .strtab and .shstrtab here.
> '.shstrtab' should be used here.
>
> The ELF specs define bots as follows (from man 5 elf) :
>
>          .shstrtab
>                 This section holds section names.  This section is of type
>                 SHT_STRTAB.  No attribute types are used.
>
>          .strtab
>                 This section holds strings, most commonly the strings that
>                 represent the names associated with symbol table entries.
>                 If the file has a loadable segment that includes the
>                 symbol string table, the section's attributes will include
>                 the SHF_ALLOC bit.  Otherwise, the bit will be off.  This
>                 section is of type SHT_STRTAB.
>
> However, the name lookup works, as you correctly specified that this
> section holds the section header names via the 'e_shstrndx' field in the
> elf header.

Sigh
We can make this a shstrtab only strtab since that's effectively what it 
does right now. It annoys me that we'll need a second strtab if we ever 
want to name other structures. Or at least we'll need special handling.

> > +        shdr64.sh_size = s->string_table_buf->len;
> > +        shdr = &shdr64;
> > +    } else {
> > +        shdr_size = sizeof(Elf32_Shdr);
> > +        memset(&shdr32, 0, shdr_size);
> > +        shdr32.sh_type = SHT_STRTAB;
> > +        shdr32.sh_offset = s->section_offset + s->elf_section_data_size;
> > +        shdr32.sh_name = s->string_table_buf->len;
> > +        g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab"));
> > +        shdr32.sh_size = s->string_table_buf->len;
> > +        shdr = &shdr32;
> > +    }
> > +
> > +    memcpy(buff, shdr, shdr_size);
> > +
> [snip]
> Also, with your patches the dump output places the headers in this ordering:
> [elf hdr]
> [section hdrs]
> [program hdrs]
>
> **normally** program hdrs are placed before section hdrs,
> but this is just a convention IIRC.

I don't see why this should be a problem, that's what the offsets are for.

> Steffen