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

["Martin Liška" <mliska@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On 10/29/22 00:21, Mark Wielaard wrote:
> 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).

Hi.

All right, so I'm preparing a full support for ZSTD (both compression and compression)
and I noticed a refactoring would be handy for compress_section function and callers
of the function.

Note right now, there are basically 3 compression types and process_file
function handles basically all combinations of these (3 x 3 options), while adding ZSTD
support would make it even more complicated. However, ZSTD will behave very similar to ZLIB
(not zlib-gnu), except a different algorithm will be used. Plus, in order to distinguish
ZLIB from ZSTD, we need to read GElf_Chdr.

So what do you think about the refactoring as the first step?

Cheers,
Martin

[-- Attachment #2: 0001-Refactor-elf_compare.patch --]
[-- Type: text/x-patch, Size: 12784 bytes --]

From 45b68678cb4a7135532b0f6c5e667ea3a06a0c35 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 28 Nov 2022 14:10:36 +0100
Subject: [PATCH] Refactor elf_compare

---
 src/elfcompress.c | 164 ++++++++++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 63 deletions(-)

diff --git a/src/elfcompress.c b/src/elfcompress.c
index 51ff69d2..16898f6a 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -48,13 +48,20 @@ static bool force = false;
 static bool permissive = false;
 static const char *foutput = NULL;
 
-#define T_UNSET 0
-#define T_DECOMPRESS 1    /* none */
-#define T_COMPRESS_ZLIB 2 /* zlib */
-#define T_COMPRESS_GNU  3 /* zlib-gnu */
+/* Compression algorithm, where all legal values for ch_type
+   (compression algorithm) do match the following enum.  */
+enum ch_type
+{
+  UNSET = -1,
+  NONE,
+  ZLIB,
+
+  ZLIB_GNU = 1 << 16
+};
+
 #define WORD_BITS (8U * sizeof (unsigned int))
 
-static int type = T_UNSET;
+static enum ch_type type = UNSET;
 
 struct section_pattern
 {
@@ -120,22 +127,22 @@ parse_opt (int key, char *arg __attribute__ ((unused)),
       break;
 
     case 't':
-      if (type != T_UNSET)
+      if (type != UNSET)
 	argp_error (state, N_("-t option specified twice"));
 
       if (strcmp ("none", arg) == 0)
-	type = T_DECOMPRESS;
+	type = NONE;
       else if (strcmp ("zlib", arg) == 0 || strcmp ("zlib-gabi", arg) == 0)
-	type = T_COMPRESS_ZLIB;
+	type = ZLIB;
       else if (strcmp ("zlib-gnu", arg) == 0 || strcmp ("gnu", arg) == 0)
-	type = T_COMPRESS_GNU;
+	type = ZLIB_GNU;
       else
 	argp_error (state, N_("unknown compression type '%s'"), arg);
       break;
 
     case ARGP_KEY_SUCCESS:
-      if (type == T_UNSET)
-	type = T_COMPRESS_ZLIB;
+      if (type == UNSET)
+	type = ZLIB;
       if (patterns == NULL)
 	add_pattern (".?(z)debug*");
       break;
@@ -198,14 +205,19 @@ setshdrstrndx (Elf *elf, GElf_Ehdr *ehdr, size_t ndx)
 static int
 compress_section (Elf_Scn *scn, size_t orig_size, const char *name,
 		  const char *newname, size_t ndx,
-		  bool gnu, bool compress, bool report_verbose)
+		  enum ch_type schtype, enum ch_type dchtype,
+		  bool report_verbose)
 {
+  /* We either compress or decompress.  */
+  assert (schtype == NONE || dchtype == NONE);
+  bool compress = dchtype != NONE;
+
   int res;
   unsigned int flags = compress && force ? ELF_CHF_FORCE : 0;
-  if (gnu)
+  if (schtype == ZLIB_GNU || dchtype == ZLIB_GNU)
     res = elf_compress_gnu (scn, compress ? 1 : 0, flags);
   else
-    res = elf_compress (scn, compress ? ELFCOMPRESS_ZLIB : 0, flags);
+    res = elf_compress (scn, dchtype, flags);
 
   if (res < 0)
     error (0, 0, "Couldn't decompress section [%zd] %s: %s",
@@ -446,20 +458,20 @@ process_file (const char *fname)
 
       if (section_name_matches (sname))
 	{
-	  if (!force && type == T_DECOMPRESS
+	  if (!force && type == NONE
 	      && (shdr->sh_flags & SHF_COMPRESSED) == 0
 	      && !startswith (sname, ".zdebug"))
 	    {
 	      if (verbose > 0)
 		printf ("[%zd] %s already decompressed\n", ndx, sname);
 	    }
-	  else if (!force && type == T_COMPRESS_ZLIB
+	  else if (!force && type == ZLIB
 		   && (shdr->sh_flags & SHF_COMPRESSED) != 0)
 	    {
 	      if (verbose > 0)
 		printf ("[%zd] %s already compressed\n", ndx, sname);
 	    }
-	  else if (!force && type == T_COMPRESS_GNU
+	  else if (!force && type == ZLIB_GNU
 		   && startswith (sname, ".zdebug"))
 	    {
 	      if (verbose > 0)
@@ -471,9 +483,9 @@ process_file (const char *fname)
 	      set_section (sections, ndx);
 	      /* Check if we might want to change this section name.  */
 	      if (! adjust_names
-		  && ((type != T_COMPRESS_GNU
+		  && ((type != ZLIB_GNU
 		       && startswith (sname, ".zdebug"))
-		      || (type == T_COMPRESS_GNU
+		      || (type == ZLIB_GNU
 			  && startswith (sname, ".debug"))))
 		adjust_names = true;
 
@@ -634,11 +646,11 @@ process_file (const char *fname)
      and keep track of whether or not to compress them (later in the
      fixup pass).  Also record the original size, so we can report the
      difference later when we do compress.  */
-  int shstrtab_compressed = T_UNSET;
+  enum ch_type shstrtab_compressed = UNSET;
   size_t shstrtab_size = 0;
   char *shstrtab_name = NULL;
   char *shstrtab_newname = NULL;
-  int symtab_compressed = T_UNSET;
+  enum ch_type symtab_compressed = UNSET;
   size_t symtab_size = 0;
   char *symtab_name = NULL;
   char *symtab_newname = NULL;
@@ -677,6 +689,32 @@ process_file (const char *fname)
 	     (de)compressed, invalidating the string pointers.  */
 	  sname = xstrdup (sname);
 
+	  /* Detect source compression that is how is the section compressed
+	     now.  */
+	  GElf_Chdr chdr;
+	  enum ch_type schtype = NONE;
+	  if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
+	    {
+	      if (gelf_getchdr (scn, &chdr) != NULL)
+		{
+		  schtype = (enum ch_type)chdr.ch_type;
+		  if (schtype == NONE)
+		    {
+		      error (0, 0, "Compression type for section %zd"
+			     " can't be zero ", ndx);
+		      goto cleanup;
+		    }
+		}
+	      else
+		{
+		  error (0, 0, "Couldn't get chdr for section %zd", ndx);
+		  goto cleanup;
+		}
+	    }
+	  /* Set ZLIB compression manually for .zdebug* sections.  */
+	  else if (startswith (sname, ".zdebug"))
+	    schtype = ZLIB_GNU;
+
 	  /* We might want to decompress (and rename), but not
 	     compress during this pass since we might need the section
 	     data in later passes.  Skip those sections for now and
@@ -687,35 +725,32 @@ process_file (const char *fname)
 
 	  switch (type)
 	    {
-	    case T_DECOMPRESS:
-	      if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
+	    case NONE:
+	      if (schtype != NONE)
 		{
+		  if (schtype == ZLIB_GNU)
+		    {
+		      snamebuf[0] = '.';
+		      strcpy (&snamebuf[1], &sname[2]);
+		      newname = snamebuf;
+		    }
 		  if (compress_section (scn, size, sname, NULL, ndx,
-					false, false, verbose > 0) < 0)
-		    goto cleanup;
-		}
-	      else if (startswith (sname, ".zdebug"))
-		{
-		  snamebuf[0] = '.';
-		  strcpy (&snamebuf[1], &sname[2]);
-		  newname = snamebuf;
-		  if (compress_section (scn, size, sname, newname, ndx,
-					true, false, verbose > 0) < 0)
+					schtype, NONE, verbose > 0) < 0)
 		    goto cleanup;
 		}
 	      else if (verbose > 0)
 		printf ("[%zd] %s already decompressed\n", ndx, sname);
 	      break;
 
-	    case T_COMPRESS_GNU:
+	    case ZLIB_GNU:
 	      if (startswith (sname, ".debug"))
 		{
-		  if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
+		  if (schtype == ZLIB)
 		    {
 		      /* First decompress to recompress GNU style.
 			 Don't report even when verbose.  */
 		      if (compress_section (scn, size, sname, NULL, ndx,
-					    false, false, false) < 0)
+					    schtype, NONE, false) < 0)
 			goto cleanup;
 		    }
 
@@ -729,7 +764,7 @@ process_file (const char *fname)
 		      if (ndx == shdrstrndx)
 			{
 			  shstrtab_size = size;
-			  shstrtab_compressed = T_COMPRESS_GNU;
+			  shstrtab_compressed = ZLIB_GNU;
 			  if (shstrtab_name != NULL
 			      || shstrtab_newname != NULL)
 			    {
@@ -745,7 +780,7 @@ process_file (const char *fname)
 		      else
 			{
 			  symtab_size = size;
-			  symtab_compressed = T_COMPRESS_GNU;
+			  symtab_compressed = ZLIB_GNU;
 			  symtab_name = xstrdup (sname);
 			  symtab_newname = xstrdup (newname);
 			}
@@ -753,7 +788,7 @@ process_file (const char *fname)
 		  else
 		    {
 		      int result = compress_section (scn, size, sname, newname,
-						     ndx, true, true,
+						     ndx, NONE, type,
 						     verbose > 0);
 		      if (result < 0)
 			goto cleanup;
@@ -764,7 +799,7 @@ process_file (const char *fname)
 		}
 	      else if (verbose >= 0)
 		{
-		  if (startswith (sname, ".zdebug"))
+		  if (schtype == ZLIB_GNU)
 		    printf ("[%zd] %s unchanged, already GNU compressed\n",
 			    ndx, sname);
 		  else
@@ -773,15 +808,15 @@ process_file (const char *fname)
 		}
 	      break;
 
-	    case T_COMPRESS_ZLIB:
+	    case ZLIB:
 	      if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
 		{
-		  if (startswith (sname, ".zdebug"))
+		  if (schtype == ZLIB_GNU)
 		    {
 		      /* First decompress to recompress zlib style.
 			 Don't report even when verbose.  */
 		      if (compress_section (scn, size, sname, NULL, ndx,
-					    true, false, false) < 0)
+					    schtype, NONE, false) < 0)
 			goto cleanup;
 
 		      snamebuf[0] = '.';
@@ -794,7 +829,7 @@ process_file (const char *fname)
 		      if (ndx == shdrstrndx)
 			{
 			  shstrtab_size = size;
-			  shstrtab_compressed = T_COMPRESS_ZLIB;
+			  shstrtab_compressed = ZLIB;
 			  if (shstrtab_name != NULL
 			      || shstrtab_newname != NULL)
 			    {
@@ -811,19 +846,22 @@ process_file (const char *fname)
 		      else
 			{
 			  symtab_size = size;
-			  symtab_compressed = T_COMPRESS_ZLIB;
+			  symtab_compressed = ZLIB;
 			  symtab_name = xstrdup (sname);
 			  symtab_newname = (newname == NULL
 					    ? NULL : xstrdup (newname));
 			}
 		    }
 		  else if (compress_section (scn, size, sname, newname, ndx,
-					     false, true, verbose > 0) < 0)
+					     NONE, type, verbose > 0) < 0)
 		    goto cleanup;
 		}
 	      else if (verbose > 0)
 		printf ("[%zd] %s already compressed\n", ndx, sname);
 	      break;
+
+	    case UNSET:
+	      break;
 	    }
 
 	  free (sname);
@@ -903,28 +941,28 @@ process_file (const char *fname)
 	      /* If the section is (still) compressed we'll need to
 		 uncompress it first to adjust the data, then
 		 recompress it in the fixup pass.  */
-	      if (symtab_compressed == T_UNSET)
+	      if (symtab_compressed == UNSET)
 		{
 		  size_t size = shdr->sh_size;
 		  if ((shdr->sh_flags == SHF_COMPRESSED) != 0)
 		    {
 		      /* Don't report the (internal) uncompression.  */
 		      if (compress_section (newscn, size, sname, NULL, ndx,
-					    false, false, false) < 0)
+					    ZLIB, NONE, false) < 0)
 			goto cleanup;
 
 		      symtab_size = size;
-		      symtab_compressed = T_COMPRESS_ZLIB;
+		      symtab_compressed = ZLIB;
 		    }
 		  else if (startswith (name, ".zdebug"))
 		    {
 		      /* Don't report the (internal) uncompression.  */
 		      if (compress_section (newscn, size, sname, NULL, ndx,
-					    true, false, false) < 0)
+					    ZLIB_GNU, NONE, false) < 0)
 			goto cleanup;
 
 		      symtab_size = size;
-		      symtab_compressed = T_COMPRESS_GNU;
+		      symtab_compressed = ZLIB_GNU;
 		    }
 		}
 
@@ -1037,7 +1075,7 @@ process_file (const char *fname)
 	 or if the section was already compressed (and the user didn't
 	 ask for decompression).  Note somewhat identical code for
 	 symtab below.  */
-      if (shstrtab_compressed == T_UNSET)
+      if (shstrtab_compressed == UNSET)
 	{
 	  /* The user didn't ask for compression, but maybe it was
 	     compressed in the original ELF file.  */
@@ -1067,18 +1105,18 @@ process_file (const char *fname)
 
 	  shstrtab_size = shdr->sh_size;
 	  if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
-	    shstrtab_compressed = T_COMPRESS_ZLIB;
+	    shstrtab_compressed = ZLIB;
 	  else if (startswith (shstrtab_name, ".zdebug"))
-	    shstrtab_compressed = T_COMPRESS_GNU;
+	    shstrtab_compressed = ZLIB_GNU;
 	}
 
       /* Should we (re)compress?  */
-      if (shstrtab_compressed != T_UNSET)
+      if (shstrtab_compressed != UNSET)
 	{
 	  if (compress_section (scn, shstrtab_size, shstrtab_name,
 				shstrtab_newname, shdrstrndx,
-				shstrtab_compressed == T_COMPRESS_GNU,
-				true, verbose > 0) < 0)
+				NONE, shstrtab_compressed,
+				verbose > 0) < 0)
 	    goto cleanup;
 	}
     }
@@ -1178,7 +1216,7 @@ process_file (const char *fname)
 		 us to, or if the section was already compressed (and
 		 the user didn't ask for decompression).  Note
 		 somewhat identical code for shstrtab above.  */
-	      if (symtab_compressed == T_UNSET)
+	      if (symtab_compressed == UNSET)
 		{
 		  /* The user didn't ask for compression, but maybe it was
 		     compressed in the original ELF file.  */
@@ -1208,18 +1246,18 @@ process_file (const char *fname)
 
 		  symtab_size = shdr->sh_size;
 		  if ((shdr->sh_flags & SHF_COMPRESSED) != 0)
-		    symtab_compressed = T_COMPRESS_ZLIB;
+		    symtab_compressed = ZLIB;
 		  else if (startswith (symtab_name, ".zdebug"))
-		    symtab_compressed = T_COMPRESS_GNU;
+		    symtab_compressed = ZLIB_GNU;
 		}
 
 	      /* Should we (re)compress?  */
-	      if (symtab_compressed != T_UNSET)
+	      if (symtab_compressed != UNSET)
 		{
 		  if (compress_section (scn, symtab_size, symtab_name,
 					symtab_newname, symtabndx,
-					symtab_compressed == T_COMPRESS_GNU,
-					true, verbose > 0) < 0)
+					NONE, symtab_compressed,
+					verbose > 0) < 0)
 		    goto cleanup;
 		}
 	    }
-- 
2.38.1