Re: [PATCHv2] support ZSTD compression algorithm

[Mark Wielaard <mark@klomp.org>]

Hi Martin,

On Thu, 2022-12-22 at 10:17 +0100, Martin Liška wrote:
> Is the abort () at the second call site because that cannot happen?
> > Or should that also goto cleanup?
> 
> Yes, it can't happen.

Aha, because it should already have seen that section before, in which
case it would have errored out.

> There's V2 (including ChangeLog) where all issues apart from the
> libzstd configure.ac detection should be addressed.

See also my other email. I think that addresses everything. Just merge
those and push please.

> diff --git a/configure.ac b/configure.ac
> index 59be27ac..ef16f79e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -417,9 +417,11 @@ AC_SUBST([BZ2_LIB])
>  eu_ZIPLIB(lzma,LZMA,lzma,lzma_auto_decoder,[LZMA (xz)])
>  AS_IF([test "x$with_lzma" = xyes], [LIBLZMA="liblzma"],
> [LIBLZMA=""])
>  AC_SUBST([LIBLZMA])
> -eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_decompress,[ZSTD (zst)])
> +eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_compressStream2,[ZSTD (zst)])
>  AS_IF([test "x$with_zstd" = xyes], [LIBZSTD="libzstd"],
> [LIBLZSTD=""])
>  AC_SUBST([LIBZSTD])
> +zstd_LIBS="$LIBS"
> +AC_SUBST([zstd_LIBS])
>  zip_LIBS="$LIBS"
>  LIBS="$save_LIBS"
>  AC_SUBST([zip_LIBS])

See the other email for an idea how to keep most of the checks the
same, but just for decompress (in the case of zstd). Then define a
USE_ZSTD_COMPRESS only for new enough libzstd.

> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
> index d7f53af2..deb585b7 100644
> --- a/libelf/elf_compress.c
> +++ b/libelf/elf_compress.c
> @@ -39,6 +39,10 @@
>  #include <string.h>
>  #include <zlib.h>
>  
> +#ifdef USE_ZSTD
> +#include <zstd.h>
> +#endif
> +
>  /* Cleanup and return result.  Don't leak memory.  */
>  static void *
>  do_deflate_cleanup (void *result, z_stream *z, void *out_buf,
> @@ -54,53 +58,14 @@ do_deflate_cleanup (void *result, z_stream *z,
> void *out_buf,
>  #define deflate_cleanup(result, cdata) \
>      do_deflate_cleanup(result, &z, out_buf, cdata)
>  
> -/* Given a section, uses the (in-memory) Elf_Data to extract the
> -   original data size (including the given header size) and data
> -   alignment.  Returns a buffer that has at least hsize bytes (for
> the
> -   caller to fill in with a header) plus zlib compressed date.  Also
> -   returns the new buffer size in new_size (hsize + compressed data
> -   size).  Returns (void *) -1 when FORCE is false and the
> compressed
> -   data would be bigger than the original data.  */
>  void *
>  internal_function
> -__libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
> -		   size_t *orig_size, size_t *orig_addralign,
> -		   size_t *new_size, bool force)
> +__libelf_compress_zlib (Elf_Scn *scn, size_t hsize, int ei_data,
> +			size_t *orig_size, size_t *orig_addralign,
> +			size_t *new_size, bool force,
> +			Elf_Data *data, Elf_Data *next_data,
> +			void *out_buf, size_t out_size, size_t block)

Missed this in my other patch, but __libelf_compress_zlib also isn't an
internal_function, it can/must just be static.
 
> [...]
> +#ifdef USE_ZSTD

So this is the compression part, guard that with USE_ZSTD_COMPRESS.

> +
>  void *
>  internal_function
> -__libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
> +__libelf_decompress_zlib (void *buf_in, size_t size_in, size_t
> size_out)
>  {

So this isn't an internal function, just mark it static void *.

> +#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 requesting zero size.  This is
> highly
> +     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 *buf_out = malloc (size_out ?: 1);
> +  if (unlikely (buf_out == NULL))
> +    {
> +      __libelf_seterrno (ELF_E_NOMEM);
> +      return NULL;
> +    }
> +
> +  size_t ret = ZSTD_decompress (buf_out, size_out, buf_in, size_in);
> +  if (ZSTD_isError (ret))
> +    {
> +      free (buf_out);
> +      __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE);

This should be ELF_E_DECOMPRESS_ERROR.

> +      return NULL;
> +    }
> +  else
> +    return buf_out;
> +}
> +#endif
> +
> +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
> +    __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);

And this should be ELF_E_UNKNOWN_COMPRESSION_TYPE.

> +    return NULL;
> +#endif
> +    }
> +}

> diff --git a/src/elfcompress.c b/src/elfcompress.c
> index eff765e8..bfdac2b4 100644
> --- a/src/elfcompress.c
> +++ b/src/elfcompress.c
> @@ -55,9 +55,10 @@ enum ch_type
>    UNSET = -1,
>    NONE,
>    ZLIB,
> +  ZSTD,
>  
>    /* Maximal supported ch_type.  */
> -  MAXIMAL_CH_TYPE = ZLIB,
> +  MAXIMAL_CH_TYPE = ZSTD,
>  
>    ZLIB_GNU = 1 << 16
>  };
> @@ -139,6 +140,12 @@ parse_opt (int key, char *arg __attribute__
> ((unused)),
>  	type = ZLIB;
>        else if (strcmp ("zlib-gnu", arg) == 0 || strcmp ("gnu", arg)
> == 0)
>  	type = ZLIB_GNU;
> +      else if (strcmp ("zstd", arg) == 0)
> +#ifdef USE_ZSTD
> +	type = ZSTD;
> +#else
> +	argp_error (state, N_("ZSTD support is not enabled"));
> +#endif

This is slightly trickier now. I think you can use USE_ZSTD_COMPRESS as
guard here, since this is always about compression. In the new setup
you can have either no zstd support, only decompression support or
both.

But you can also just let elf_compress return the right error and make
another change:

diff --git a/src/elfcompress.c b/src/elfcompress.c
index bfdac2b4..1f32331c 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -230,7 +230,8 @@ compress_section (Elf_Scn *scn, size_t orig_size,
const char *name,
     res = elf_compress (scn, dchtype, flags);
 
   if (res < 0)
-    error (0, 0, "Couldn't decompress section [%zd] %s: %s",
+    error (0, 0, "Couldn't %s section [%zd] %s: %s",
+          compress ? "compress" : "decompress",
           ndx, name, elf_errmsg (-1));
   else
     {

So the user knows whether it is compression or decompression that
failed.

> diff --git a/src/readelf.c b/src/readelf.c
> index cc3e0229..451f8400 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -1238,13 +1238,17 @@ get_visibility_type (int value)
>  static const char *
>  elf_ch_type_name (unsigned int code)
>  {
> -  if (code == 0)
> -    return "NONE";
> -
> -  if (code == ELFCOMPRESS_ZLIB)
> -    return "ZLIB";
> -
> -  return "UNKNOWN";
> +  switch (code)
> +    {
> +    case 0:
> +      return "NONE";
> +    case ELFCOMPRESS_ZLIB:
> +      return "ZLIB";
> +    case ELFCOMPRESS_ZSTD:
> +      return "ZSTD";
> +    default:
> +      return "UNKNOWN";
> +    }
>  }
>  
>  /* Print the section headers.  */
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 356b3fbf..c1868307 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -218,6 +218,7 @@ endif
>  
>  if HAVE_ZSTD
>  TESTS += run-readelf-compressed-zstd.sh
> +export ELFUTILS_ZSTD = 1
>  endif

So this is the wrong guard. HAVE_ZSTD is whether we have the zstd
binary or not. Guard export ELFUTILS_ZSTD = 1 with USE_ZSTD_COMPRESS.

Thanks,

Mark