[PATCH 1/4] libdwfl: Make sure mapped is always set in unzip

[PATCH 1/4] libdwfl: Make sure mapped is always set in unzip

[Mark Wielaard]

Found by GCC14 -Wanalyzer-null-argument.

When unzip is called with mapped NULL, but *_whole not NULL, *_whole
contains the first part of the input. But we check against mapped to
make sure the MAGIC bytes are there.

This only worked because this code path was never taken, unzip is
currently always called with *_whole being NULL.

	  * libdwfl/gzip.c (unzip): Set mapped = state.input_buffer
          when *whole is not NULL.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdwfl/gzip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libdwfl/gzip.c b/libdwfl/gzip.c
index 002afc4e916b..9c74abdafc19 100644
--- a/libdwfl/gzip.c
+++ b/libdwfl/gzip.c
@@ -212,6 +212,7 @@ unzip (int fd, off_t start_offset,
       else
 	{
 	  state.input_buffer = *state.whole;
+	  mapped = state.input_buffer;
 	  state.input_pos = state.mapped_size = *whole_size;
 	}
     }
-- 
2.45.2

[PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory

[Mark Wielaard]

Found by GCC -fanalyzer.

When allocating the notcvt buffer fails we leak the shdr. goto
free_and_out on malloc failure.

	     * libelf/elf32_getshdr.c (load_shdr_wrlock): goto
             free_and_out on second malloc failure.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/elf32_getshdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libelf/elf32_getshdr.c b/libelf/elf32_getshdr.c
index fc696302b8b6..19b690a8e87b 100644
--- a/libelf/elf32_getshdr.c
+++ b/libelf/elf32_getshdr.c
@@ -126,7 +126,7 @@ load_shdr_wrlock (Elf_Scn *scn)
 	      if (unlikely (notcvt == NULL))
 		{
 		  __libelf_seterrno (ELF_E_NOMEM);
-		  goto out;
+		  goto free_and_out;
 		}
 	      memcpy (notcvt, ((char *) elf->map_address
 			       + elf->start_offset + ehdr->e_shoff),
-- 
2.45.2

[PATCH 3/4] debuginfod-client: Don't leak id/version with duplicate os-release entries

[Mark Wielaard]

Found by GCC14 -Wanalyzer-double-free.

If the os-release file would contain multiple ID or VERSION_ID entries
we would leak the originally parsed one. Fix by seeing whether id or
version is already set and ignore any future entries.

	* debuginfod/debuginfod-client.c (add_default_headers): Check
	whether id or version is already set before resetting them.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/debuginfod-client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 95f2a92b701c..24ede19af385 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -673,9 +673,9 @@ add_default_headers(debuginfod_client *client)
               v++;
               s[len - 1] = '\0';
             }
-          if (strcmp (s, "ID") == 0)
+          if (id == NULL && strcmp (s, "ID") == 0)
             id = strdup (v);
-          if (strcmp (s, "VERSION_ID") == 0)
+          if (version == NULL && strcmp (s, "VERSION_ID") == 0)
             version = strdup (v);
         }
       fclose (f);
-- 
2.45.2

[PATCH 4/4] ar, ranlib: Don't double close file descriptors

[Mark Wielaard]

Found by GCC14 -Wanalyzer-fd-double-close.

close always closes the given file descriptor even on error. So don't
try to close a file descriptor again on error (even on EINTR). This
could be bad in a multi-threaded environment.

      * src/ar.c (do_oper_extract): Call close and set newfd to -1.
      (do_oper_delete): Likewise.
      (do_oper_insert): Likewise.
      * src/ranlib.c (handle_file): Likewise.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ar.c     | 12 ++++++------
 src/ranlib.c |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/ar.c b/src/ar.c
index fcb8bfb90a9f..9ace28b918d3 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -808,9 +808,9 @@ cannot rename temporary file to %.*s"),
 	      if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
 	      /* Set the mode of the new file to the same values the
 		 original file has.  */
-	      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-		  || close (newfd) != 0)
+	      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0)
 		goto nonew_unlink;
+	      close (newfd);
 	      newfd = -1;
 	      if (rename (tmpfname, arfname) != 0)
 		goto nonew_unlink;
@@ -1061,9 +1061,9 @@ do_oper_delete (const char *arfname, char **argv, int argc,
      setting the mode (which might be reset/ignored if the owner is
      wrong.  */
   if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
-  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-      || close (newfd) != 0)
+  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0)
     goto nonew_unlink;
+  close (newfd);
   newfd = -1;
   if (rename (tmpfname, arfname) != 0)
     goto nonew_unlink;
@@ -1547,9 +1547,9 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc,
 	 setting the modes, or they might be reset/ignored if the
 	 owner is wrong.  */
       if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
-      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-	  || close (newfd) != 0)
+      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0)
         goto nonew_unlink;
+      close (newfd);
       newfd = -1;
       if (rename (tmpfname, arfname) != 0)
 	goto nonew_unlink;
diff --git a/src/ranlib.c b/src/ranlib.c
index 7838d69eaec6..073df8c551af 100644
--- a/src/ranlib.c
+++ b/src/ranlib.c
@@ -264,9 +264,9 @@ handle_file (const char *fname)
 	  if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
 	  /* Set the mode of the new file to the same values the
 	     original file has.  */
-	  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-	      || close (newfd) != 0)
+	  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0)
 	    goto nonew_unlink;
+	  close (newfd);
 	  newfd = -1;
 	  if (rename (tmpfname, fname) != 0)
 	    goto nonew_unlink;
-- 
2.45.2

Re: [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip

[Mark Wielaard]

Hi,

On Sun, Jun 23, 2024 at 01:50:10AM +0200, Mark Wielaard wrote:
> Found by GCC14 -Wanalyzer-null-argument.

Aaron took a quick look on irc and said all 4 patches looked
reasonable. So I pushed them all to main now.

Cheers,

Mark