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

[Janosch Frank <frankja@linux.ibm.com>]

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
>