Re: [PATCH][RFC] readelf: partial support of ZSTD compression

[Mark Wielaard <mark@klomp.org>]

Hi Martin,

On Mon, Oct 24, 2022 at 08:16:09PM +0200, Martin Liška wrote:
> Anyway, I'm sending V2 that works fine --with-zstd and --without-zstd as expected.
> 
> Ready for master?

The decompression part looks good. Few small nitpicks below.

Although I like to also have compression working.  Then we can also
add support to src/elfcompress, which makes for a good testcase. See
tests/run-compress.sh (which has found some subtle memory issues when
run under valgrind).

> From 4aea412783b9b0dcaf0f887947bf2e8ee6c5368b Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Mon, 24 Oct 2022 11:53:13 +0200
> Subject: [PATCH] readelf: partial support of ZSTD compression
> 
> Support decompression of ZSTD sections and add support
> for it when -SWz is used:
> 
> ...
> [30] .debug_abbrev        PROGBITS     0000000000000000 00001f9d 00000168  0 C      0   0  1
>      [ELF ZSTD (2) 000002fc  1]
> ...

And for this it would be nice to have a tests/run-readelf-z.sh testcase.

> ChangeLog:
> 
> 	* configure.ac: Add zstd_LIBS.
> 
> libelf/ChangeLog:
> 
> 	* Makefile.am: Use zstd_LIBS.
> 	* elf.h (ELFCOMPRESS_ZSTD): Add new value.
> 	* elf_compress.c (__libelf_decompress): Dispatch based
> 	on the compression algorithm.
> 	(__libelf_decompress_zlib): New.
> 	(__libelf_decompress_zstd): New.
> 	(__libelf_decompress_elf): Pass type of compression to
> 	__libelf_decompress.
> 	* elf_compress_gnu.c (elf_compress_gnu): Use ELFCOMPRESS_ZLIB
> 	as .z* sections can be only compressed with ZLIB.
> 	* libelfP.h (__libelf_decompress): Change signature.
> 
> src/ChangeLog:
> 
> 	* readelf.c (elf_ch_type_name): Use switch and support ZSTD.

> diff --git a/libelf/elf.h b/libelf/elf.h
> index 02a1b3f5..f0f0ec7d 100644
> --- a/libelf/elf.h
> +++ b/libelf/elf.h

We normally sync elf.h from glibc. I'll do that in a second.

> +#ifdef USE_ZSTD
> +void *
> +internal_function
> +__libelf_decompress_zstd (void *buf_in, size_t size_in, size_t size_out)
> +{
> +  /* Malloc might return NULL when requestion zero size.  This is highly

requesting

> +     unlikely, it would only happen when the compression was forced.
> +     But we do need a non-NULL buffer to return and set as result.
> +     Just make sure to always allocate at least 1 byte.  */

> +void *
> +internal_function
> +__libelf_decompress (int chtype, void *buf_in, size_t size_in, size_t size_out)
> +{
> +  if (chtype == ELFCOMPRESS_ZLIB)
> +    return __libelf_decompress_zlib (buf_in, size_in, size_out);
> +  else
> +    {
> +#ifdef USE_ZSTD
> +    return __libelf_decompress_zstd (buf_in, size_in, size_out);
> +#else
> +    return 0;
> +#endif

return NULL for consistency?

> +    }
> +}
> +
>  void *
>  internal_function
>  __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
> @@ -268,7 +316,7 @@ __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
>    if (gelf_getchdr (scn, &chdr) == NULL)
>      return NULL;
>  
> -  if (chdr.ch_type != ELFCOMPRESS_ZLIB)
> +  if (chdr.ch_type != ELFCOMPRESS_ZLIB && chdr.ch_type != ELFCOMPRESS_ZSTD)
>      {

What about the ifndef USE_ZSTD case? Should this then not recognize
ELFCOMPRESS_ZSTD?

Thanks,

Mark