Re: [PATCH v3 08/14] dump/dump: Add section string table support

[Janosch Frank]

On 7/21/22 15:22, 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           | 81 +++++++++++++++++++++++++++++++++++++++++--
>   include/sysemu/dump.h |  1 +
>   2 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 6f3274c5af..944217349a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s)
>       close(s->fd);
>       g_free(s->guest_note);
>       g_free(s->elf_header);
> +    g_array_unref(s->string_table_buf);
>       s->guest_note = NULL;
>       if (s->resume) {
>           if (s->detached) {
> @@ -357,14 +358,47 @@ static size_t prepare_elf_section_hdr_zero(DumpState *s, 
> void *buff)
>       return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>   }
>   
> +static void write_elf_section_hdr_string(DumpState *s, void *buff)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;
> +    int shdr_size;
> +    void *shdr = buff;
> +
> +    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"));
> +        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);
> +}
> +
>   static void prepare_elf_section_hdrs(DumpState *s)
>   {
>       uint8_t *buff_hdr;
> -    size_t len, sizeof_shdr;
> +    size_t len, size = 0, sizeof_shdr;
> +    Elf64_Ehdr *hdr64 = s->elf_header;
> +    Elf32_Ehdr *hdr32 = s->elf_header;
>   
>       /*
>        * Section ordering:
>        * - HDR zero (if needed)
> +     * - String table hdr
>        */
>       sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>       len = sizeof_shdr * s->shdr_num;
> @@ -375,6 +409,22 @@ static void prepare_elf_section_hdrs(DumpState *s)
>       if (s->phdr_num == PN_XNUM) {
>               prepare_elf_section_hdr_zero(s, buff_hdr);
>       }
> +    buff_hdr += size;

If I'm not mistaken this line doesn't make sense at all.
It needs to be in the if and add sizeof_shdr so we can account for the 
zero header.

I.e.:
if (s->phdr_num == PN_XNUM) {
        prepare_elf_section_hdr_zero(s, buff_hdr);
        buff_hdr += sizeof_shdr;
}


Also size isn't really needed, we can directly add 
dump_arch_sections_write_hdr()'s return to buff_hdr in the next patch.

> +
> +    if (s->shdr_num < 2) {
> +        return;
> +    }
> +
> +    /*
> +     * String table needs to be last section since strings are added
> +     * via arch_sections_write_hdr().
> +     */
> +    write_elf_section_hdr_string(s, buff_hdr);
> +    if (dump_is_64bit(s)) {
> +        hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> +    } else {
> +        hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> +    }
>   }