public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array.
@ 2020-06-16 22:25 Mark Wielaard
  2020-06-16 22:25 ` [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi Mark Wielaard
                   ` (9 more replies)
  0 siblings, 10 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

We are using the reloc_nametable zero element as an char array.
So make that element an actual array (we are actually after one
of the next string arrays in the table).

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 backends/ChangeLog      | 6 ++++++
 backends/common-reloc.c | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/backends/ChangeLog b/backends/ChangeLog
index 7d3578b0..c85dfd2a 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,9 @@
+2020-06-16  Mark Wielard  <mark@klomp.org>
+
+	* common-reloc.c (reloc_nametable): Make zero a 1 char array.
+	Initialize it as an array { '\0' }.
+	(reloc_type_name): Access zero as an array.
+
 2020-06-10  Mark Wielard  <mark@klomp.org>
 
 	* aarch64_init.c (aarch64_init): Remove ehlen, return eh.
diff --git a/backends/common-reloc.c b/backends/common-reloc.c
index 096ed1c7..a91bc87d 100644
--- a/backends/common-reloc.c
+++ b/backends/common-reloc.c
@@ -45,14 +45,14 @@
 
 static const struct EBLHOOK(reloc_nametable)
 {
-  char zero;
+  char zero[1];
 #define	RELOC_TYPE(type, uses) \
   char name_##type[sizeof R_NAME (type)];
 #include RELOC_TYPES
 #undef RELOC_TYPE
 } EBLHOOK(reloc_nametable) =
   {
-    '\0',
+    { '\0' },
 #define	RELOC_TYPE(type, uses) R_NAME (type),
 #include RELOC_TYPES
 #undef RELOC_TYPE
@@ -92,7 +92,7 @@ EBLHOOK(reloc_type_name) (int reloc,
 #endif
 
   if (reloc >= 0 && reloc < nreloc && EBLHOOK(reloc_nameidx)[reloc] != 0)
-    return &reloc_namestr[EBLHOOK(reloc_nameidx)[reloc]];
+    return reloc_namestr[EBLHOOK(reloc_nameidx)[reloc]];
   return NULL;
 }
 
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 12:56   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error Mark Wielaard
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdw/ChangeLog | 4 ++++
 libdw/cfi.c     | 1 +
 2 files changed, 5 insertions(+)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index c75b0958..72cd5003 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,7 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* cfi.c (execute_cfi): Add missing FALLTHROUGH.
+
 2020-05-08  Mark Wielaard  <mark@klomp.org>
 
 	* libdw_visit_scopes.c (walk_children): Don't recurse into imported
diff --git a/libdw/cfi.c b/libdw/cfi.c
index 341e055b..6705294f 100644
--- a/libdw/cfi.c
+++ b/libdw/cfi.c
@@ -229,6 +229,7 @@ execute_cfi (Dwarf_CFI *cache,
 	case DW_CFA_offset_extended:
 	  get_uleb128 (operand, program, end);
 	  cfi_assert (program < end);
+	  FALLTHROUGH;
 	case DW_CFA_offset + 0 ... DW_CFA_offset + CFI_PRIMARY_MAX:
 	  get_uleb128 (offset, program, end);
 	  offset *= cie->data_alignment_factor;
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
  2020-06-16 22:25 ` [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 13:48   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others Mark Wielaard
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

We just want to make sure that any section data is decompressed before
use, if the section was already decompressed that is fine, so just ignore
any errors. The make this more clear, explicitly check for errors, then
don't do anything. This is better than silently ignoring since everywhere
else in the code we do explicitly check for errors.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ChangeLog | 5 +++++
 src/elflint.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 512d7b54..e78bc358 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* elflint.c (check_elf_header): Explicitly check and ignore
+	any error from elf_compress.
+
 2020-06-07  Mark Wielaard  <mark@klomp.org>
 
 	* nm.c (sort_by_name_strtab): Replace by...
diff --git a/src/elflint.c b/src/elflint.c
index 72584de0..9cdcccca 100644
--- a/src/elflint.c
+++ b/src/elflint.c
@@ -467,7 +467,7 @@ invalid number of section header table entries\n"));
 	  break;
 	/* If the section wasn't compressed this does nothing, but
 	   returns an error.  We don't care.  */
-	elf_compress (scn, 0, 0);
+	if (elf_compress (scn, 0, 0) < 0) { ; }
      }
   if (scnt < shnum)
     ERROR (gettext ("Can only check %u headers, shnum was %u\n"), scnt, shnum);
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
  2020-06-16 22:25 ` [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi Mark Wielaard
  2020-06-16 22:25 ` [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 17:03   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid Mark Wielaard
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

We try to find a compressed vmlinux image ending with either .gz, bz2 or
xz. Stop searching if we find one. Otherwise we will leak a file descriptor
for an earlier one we opened.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdwfl/ChangeLog              | 5 +++++
 libdwfl/linux-kernel-modules.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 4f1ec9da..44b3ece7 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* linux-kernel-modules.c (try_kernel_name): Don't try other
+	compressed kernels if we already found an compressed image.
+
 2020-05-09  Mark Wielaard  <mark@klomp.org>
 
 	* find-debuginfo.c (dwfl_standard_find_debuginfo): Return failure
diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c
index 0434f1e5..84a05f28 100644
--- a/libdwfl/linux-kernel-modules.c
+++ b/libdwfl/linux-kernel-modules.c
@@ -128,7 +128,7 @@ try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
 
   if (fd < 0)
     for (size_t i = 0;
-	 i < sizeof vmlinux_suffixes / sizeof vmlinux_suffixes[0];
+	 i < sizeof vmlinux_suffixes / sizeof vmlinux_suffixes[0] && fd < 0;
 	 ++i)
       {
 	char *zname;
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (2 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 17:58   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors Mark Wielaard
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

If the CIE return address register is invalid (unknown) for the
architecture immediately flag an error and return.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdwfl/ChangeLog      | 5 +++++
 libdwfl/frame_unwind.c | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 44b3ece7..5a3d566f 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* frame_unwind.c (handle_cfi): Flag an error if
+	return_address_register is invalid.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
 	* linux-kernel-modules.c (try_kernel_name): Don't try other
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index d7dfa5a9..bdceeb3e 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -562,7 +562,11 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
   /* The return register is special for setting the unwound->pc_state.  */
   unsigned ra = frame->fde->cie->return_address_register;
   bool ra_set = false;
-  ebl_dwarf_to_regno (ebl, &ra);
+  if (! ebl_dwarf_to_regno (ebl, &ra))
+    {
+      __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
+      return;
+    }
 
   for (unsigned regno = 0; regno < nregs; regno++)
     {
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (3 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 18:52   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path Mark Wielaard
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

In ar and ranlib we don't mind if the fchown call fails (it normally
would, then the file simply gets own by the current user). We used to
call fchown before fchmod, but that might ignore (or reset) some mode
flags, so call fchown first, then try fchmod (like we already do in
elfcompress). Also explicitly test and then ignore any errors for
chown. We used to do some asm trick, but that confuses some static
analyzers (and it is somewhat unreadable). Also split out the giant
if statements to make them a little bit more understandable.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ChangeLog |  8 +++++++
 src/ar.c      | 65 ++++++++++++++++++++++++++++++---------------------
 src/ranlib.c  | 44 +++++++++++++++++++---------------
 3 files changed, 72 insertions(+), 45 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index e78bc358..0129b8bf 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,11 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* ar.c (do_oper_extract): Split large if statement. Call fchown
+	before fchmod and explicitly ignore the return value.
+	(do_oper_delete): Likewise.
+	(do_oper_insert): Likewise.
+	* ranlib.c (handle_file): Likewise.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
 	* elflint.c (check_elf_header): Explicitly check and ignore
diff --git a/src/ar.c b/src/ar.c
index d70f1f46..7d33d814 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -787,26 +787,30 @@ cannot rename temporary file to %.*s"),
 	      else
 		rest_off = SARMAG;
 
-	      if ((symtab.symsnamelen != 0
+	      if (symtab.symsnamelen != 0
 		   && ((write_retry (newfd, symtab.symsoff,
 				     symtab.symsofflen)
 			!= (ssize_t) symtab.symsofflen)
 		       || (write_retry (newfd, symtab.symsname,
 					symtab.symsnamelen)
 			   != (ssize_t) symtab.symsnamelen)))
-		  /* Even if the original file had content before the
-		     symbol table, we write it in the correct order.  */
-		  || (index_off != SARMAG
-		      && copy_content (elf, newfd, SARMAG, index_off - SARMAG))
-		  || copy_content (elf, newfd, rest_off, st.st_size - rest_off)
-		  /* Set the mode of the new file to the same values the
-		     original file has.  */
-		  || fchmod (newfd, st.st_mode & ALLPERMS) != 0
-		  /* Never complain about fchown failing.  */
-		  || (({asm ("" :: "r" (fchown (newfd, st.st_uid,
-						st.st_gid))); }),
-		      close (newfd) != 0)
-		  || (newfd = -1, rename (tmpfname, arfname) != 0))
+		goto nonew_unlink;
+	      /* Even if the original file had content before the
+		 symbol table, we write it in the correct order.  */
+	      if ((index_off != SARMAG
+		   && copy_content (elf, newfd, SARMAG, index_off - SARMAG))
+		  || copy_content (elf, newfd, rest_off, st.st_size - rest_off))
+		goto nonew_unlink;
+
+	      /* Never complain about fchown failing.  */
+	      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)
+		goto nonew_unlink;
+	      newfd = -1;
+	      if (rename (tmpfname, arfname) != 0)
 		goto nonew_unlink;
 	    }
 	}
@@ -1052,12 +1056,15 @@ do_oper_delete (const char *arfname, char **argv, int argc,
     }
 
   /* Set the mode of the new file to the same values the original file
-     has.  */
+     has.  Never complain about fchown failing.  But do it before
+     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
-      /* Never complain about fchown failing.  */
-      || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-	  close (newfd) != 0)
-      || (newfd = -1, rename (tmpfname, arfname) != 0))
+      || close (newfd) != 0)
+    goto nonew_unlink;
+  newfd = -1;
+  if (rename (tmpfname, arfname) != 0)
     goto nonew_unlink;
 
  errout:
@@ -1534,13 +1541,19 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc,
 
   /* Set the mode of the new file to the same values the original file
      has.  */
-  if (fd != -1
-      && (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-	  /* Never complain about fchown failing.  */
-	  || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-	      close (newfd) != 0)
-	  || (newfd = -1, rename (tmpfname, arfname) != 0)))
-      goto nonew_unlink;
+  if (fd != -1)
+    {
+      /* Never complain about fchown failing.  But do it before
+	 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)
+        goto nonew_unlink;
+      newfd = -1;
+      if (rename (tmpfname, arfname) != 0)
+	goto nonew_unlink;
+    }
 
  errout:
   for (int cnt = 0; cnt < argc; ++cnt)
diff --git a/src/ranlib.c b/src/ranlib.c
index b9083484..483a1b65 100644
--- a/src/ranlib.c
+++ b/src/ranlib.c
@@ -245,25 +245,31 @@ handle_file (const char *fname)
 	  else
 	    rest_off = SARMAG;
 
-	  if ((symtab.symsnamelen != 0
-	       && ((write_retry (newfd, symtab.symsoff,
-				 symtab.symsofflen)
-		    != (ssize_t) symtab.symsofflen)
-		   || (write_retry (newfd, symtab.symsname,
-				    symtab.symsnamelen)
-		       != (ssize_t) symtab.symsnamelen)))
-	      /* Even if the original file had content before the
-		 symbol table, we write it in the correct order.  */
-	      || (index_off > SARMAG
-		  && copy_content (arelf, newfd, SARMAG, index_off - SARMAG))
-	      || copy_content (arelf, newfd, rest_off, st.st_size - rest_off)
-	      /* Set the mode of the new file to the same values the
-		 original file has.  */
-	      || fchmod (newfd, st.st_mode & ALLPERMS) != 0
-	      /* Never complain about fchown failing.  */
-	      || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-		  close (newfd) != 0)
-	      || (newfd = -1, rename (tmpfname, fname) != 0))
+	  if (symtab.symsnamelen != 0
+	      && ((write_retry (newfd, symtab.symsoff,
+				symtab.symsofflen)
+		   != (ssize_t) symtab.symsofflen)
+		  || (write_retry (newfd, symtab.symsname,
+				   symtab.symsnamelen)
+		      != (ssize_t) symtab.symsnamelen)))
+	    goto nonew_unlink;
+
+	  /* Even if the original file had content before the
+	     symbol table, we write it in the correct order.  */
+	  if ((index_off > SARMAG
+	       && copy_content (arelf, newfd, SARMAG, index_off - SARMAG))
+	      || copy_content (arelf, newfd, rest_off, st.st_size - rest_off))
+	    goto nonew_unlink;
+
+	  /* Never complain about fchown failing.  */
+	  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)
+	    goto nonew_unlink;
+	  newfd = -1;
+	  if (rename (tmpfname, fname) != 0)
 	    goto nonew_unlink;
 	}
     }
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (4 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-24 15:04   ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 08/10] debuginfod: Make sure suffix can place zero terminator when copying filename Mark Wielaard
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

Although we check for and/or create the interval_path right before,
there is still a possibility that the fopen call fails. Handle that
as if the file is unreadable.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           | 5 +++++
 debuginfod/debuginfod-client.c | 9 +++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index bc3bce32..66511a3f 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_clean_cache): Handle failing
+	fopen (interval_path).
+
 2020-03-29  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_add_http_header): Check header
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index a7dfbfb1..0cfc6dfc 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -244,9 +244,14 @@ debuginfod_clean_cache(debuginfod_client *c,
   /* Check timestamp of interval file to see whether cleaning is necessary.  */
   time_t clean_interval;
   interval_file = fopen(interval_path, "r");
-  if (fscanf(interval_file, "%ld", &clean_interval) != 1)
+  if (interval_file)
+    {
+      if (fscanf(interval_file, "%ld", &clean_interval) != 1)
+        clean_interval = cache_clean_default_interval_s;
+      fclose(interval_file);
+    }
+  else
     clean_interval = cache_clean_default_interval_s;
-  fclose(interval_file);
 
   if (time(NULL) - st.st_mtime < clean_interval)
     /* Interval has not passed, skip cleaning.  */
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 08/10] debuginfod: Make sure suffix can place zero terminator when copying filename
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (5 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 09/10] debuginfod: Fix build_id hexadecimal length check Mark Wielaard
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

We need to make sure that we can always place a zero terminator at
the end of suffix when we are copying the filename. So add one more
char to the suffix array. And make sure that we can always add an
extra escape character when we need to escape the current character.

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

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 66511a3f..9ff2e111 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_query_server): Increase suffix
+	array and prepare having to escape 1 character with 2.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_clean_cache): Handle failing
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 0cfc6dfc..e9c2ca83 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -474,7 +474,7 @@ debuginfod_query_server (debuginfod_client *c,
   char *target_cache_dir = NULL;
   char *target_cache_path = NULL;
   char *target_cache_tmppath = NULL;
-  char suffix[PATH_MAX];
+  char suffix[PATH_MAX + 1]; /* +1 for zero terminator.  */
   char build_id_bytes[MAX_BUILD_ID_BYTES * 2 + 1];
   int rc;
 
@@ -511,7 +511,7 @@ debuginfod_query_server (debuginfod_client *c,
 
       /* copy the filename to suffix, s,/,#,g */
       unsigned q = 0;
-      for (unsigned fi=0; q < PATH_MAX-1; fi++)
+      for (unsigned fi=0; q < PATH_MAX-2; fi++) /* -2, escape is 2 chars.  */
         switch (filename[fi])
           {
           case '\0':
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 09/10] debuginfod: Fix build_id hexadecimal length check.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (6 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 08/10] debuginfod: Make sure suffix can place zero terminator when copying filename Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-16 22:25 ` [PATCH 10/10] debuginfod: Make sure handle_data can be allocated and is always freed Mark Wielaard
  2020-06-19 11:43 ` [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
  9 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

When is debuginfod_query_server is given an hexadecimal string as
build-id build_id_len will be zero. We were checking the size of
the build_id_bytes destination string instead of the string length
of build_id input string. Make sure the input string is not too
big or strcpy might overwrite then end of the build_id_bytes array.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           | 5 +++++
 debuginfod/debuginfod-client.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 9ff2e111..d6bbfac8 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_query_server): Replace sizeof
+	build_id_bytes check with strlen build_id check.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_query_server): Increase suffix
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index e9c2ca83..7b53cb31 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -496,7 +496,7 @@ debuginfod_query_server (debuginfod_client *c,
   /* Copy lowercase hex representation of build_id into buf.  */
   if ((build_id_len >= MAX_BUILD_ID_BYTES) ||
       (build_id_len == 0 &&
-       sizeof(build_id_bytes) > MAX_BUILD_ID_BYTES*2 + 1))
+       strlen ((const char *) build_id) > MAX_BUILD_ID_BYTES*2))
     return -EINVAL;
   if (build_id_len == 0) /* expect clean hexadecimal */
     strcpy (build_id_bytes, (const char *) build_id);
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 10/10] debuginfod: Make sure handle_data can be allocated and is always freed.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (7 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 09/10] debuginfod: Fix build_id hexadecimal length check Mark Wielaard
@ 2020-06-16 22:25 ` Mark Wielaard
  2020-06-19 11:43 ` [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
  9 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-16 22:25 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

When allocating handle_data we should check for out of memory failures.
Also when the allocation has succeeded make sure we always clean up by
going to out1 on any future errors. So move the curl_multi_init call
earlier, because that goes to out0 on failure.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           |  5 +++++
 debuginfod/debuginfod-client.c | 22 ++++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index d6bbfac8..2bbd5db5 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_query_server): Check malloc.
+	Move curl_multi_init call before handle_data malloc call.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_query_server): Replace sizeof
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 7b53cb31..c2aa4e10 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -665,10 +665,24 @@ debuginfod_query_server (debuginfod_client *c,
         && (i == 0 || server_urls[i - 1] == url_delim_char))
       num_urls++;
 
+  CURLM *curlm = curl_multi_init();
+  if (curlm == NULL)
+    {
+      rc = -ENETUNREACH;
+      goto out0;
+    }
+
   /* Tracks which handle should write to fd. Set to the first
      handle that is ready to write the target file to the cache.  */
   CURL *target_handle = NULL;
   struct handle_data *data = malloc(sizeof(struct handle_data) * num_urls);
+  if (data == NULL)
+    {
+      rc = -ENOMEM;
+      goto out0;
+    }
+
+  /* thereafter, goto out1 on error.  */
 
   /* Initalize handle_data with default values. */
   for (int i = 0; i < num_urls; i++)
@@ -677,14 +691,6 @@ debuginfod_query_server (debuginfod_client *c,
       data[i].fd = -1;
     }
 
-  CURLM *curlm = curl_multi_init();
-  if (curlm == NULL)
-    {
-      rc = -ENETUNREACH;
-      goto out0;
-    }
-  /* thereafter, goto out1 on error.  */
-
   char *strtok_saveptr;
   char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
 
-- 
2.18.4


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array.
  2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
                   ` (8 preceding siblings ...)
  2020-06-16 22:25 ` [PATCH 10/10] debuginfod: Make sure handle_data can be allocated and is always freed Mark Wielaard
@ 2020-06-19 11:43 ` Mark Wielaard
  9 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 11:43 UTC (permalink / raw)
  To: elfutils-devel

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> We are using the reloc_nametable zero element as an char array.
> So make that element an actual array (we are actually after one
> of the next string arrays in the table).

Pushed to master.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi.
  2020-06-16 22:25 ` [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi Mark Wielaard
@ 2020-06-19 12:56   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 12:56 UTC (permalink / raw)
  To: elfutils-devel

Pushed to master.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error.
  2020-06-16 22:25 ` [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error Mark Wielaard
@ 2020-06-19 13:48   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 13:48 UTC (permalink / raw)
  To: elfutils-devel

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> We just want to make sure that any section data is decompressed before
> use, if the section was already decompressed that is fine, so just ignore
> any errors. The make this more clear, explicitly check for errors, then
> don't do anything. This is better than silently ignoring since everywhere
> else in the code we do explicitly check for errors.

Pushed to master.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others
  2020-06-16 22:25 ` [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others Mark Wielaard
@ 2020-06-19 17:03   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 17:03 UTC (permalink / raw)
  To: elfutils-devel

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> We try to find a compressed vmlinux image ending with either .gz, bz2 or
> xz. Stop searching if we find one. Otherwise we will leak a file descriptor
> for an earlier one we opened.

Pushed to master

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid.
  2020-06-16 22:25 ` [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid Mark Wielaard
@ 2020-06-19 17:58   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 17:58 UTC (permalink / raw)
  To: elfutils-devel

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> If the CIE return address register is invalid (unknown) for the
> architecture immediately flag an error and return.

Pushed to master.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors.
  2020-06-16 22:25 ` [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors Mark Wielaard
@ 2020-06-19 18:52   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-19 18:52 UTC (permalink / raw)
  To: elfutils-devel

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> In ar and ranlib we don't mind if the fchown call fails (it normally
> would, then the file simply gets own by the current user). We used to
> call fchown before fchmod, but that might ignore (or reset) some mode
> flags, so call fchown first, then try fchmod (like we already do in
> elfcompress). Also explicitly test and then ignore any errors for
> chown. We used to do some asm trick, but that confuses some static
> analyzers (and it is somewhat unreadable). Also split out the giant
> if statements to make them a little bit more understandable.

Pushed to master.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path.
  2020-06-16 22:25 ` [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path Mark Wielaard
@ 2020-06-24 15:04   ` Mark Wielaard
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Wielaard @ 2020-06-24 15:04 UTC (permalink / raw)
  To: elfutils-devel

Hi,

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> Although we check for and/or create the interval_path right before,
> there is still a possibility that the fopen call fails. Handle that
> as if the file is unreadable.

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> We need to make sure that we can always place a zero terminator at
> the end of suffix when we are copying the filename. So add one more
> char to the suffix array. And make sure that we can always add an
> extra escape character when we need to escape the current character.

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> When is debuginfod_query_server is given an hexadecimal string as
> build-id build_id_len will be zero. We were checking the size of
> the build_id_bytes destination string instead of the string length
> of build_id input string. Make sure the input string is not too
> big or strcpy might overwrite then end of the build_id_bytes array.

On Wed, 2020-06-17 at 00:25 +0200, Mark Wielaard wrote:
> When allocating handle_data we should check for out of memory failures.
> Also when the allocation has succeeded make sure we always clean up by
> going to out1 on any future errors. So move the curl_multi_init call
> earlier, because that goes to out0 on failure.

Aaron was nice enough to review these debuginfod related fixes (off-
list). Pushed all 4 to master now.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-06-24 15:04 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 22:25 [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard
2020-06-16 22:25 ` [PATCH 02/10] libdw: Add missing FALLTHROUGH in execute_cfi Mark Wielaard
2020-06-19 12:56   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 03/10] elflint: Explicitly check and ignore elf_compress error Mark Wielaard
2020-06-19 13:48   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 04/10] libdwfl: When we find a compressed image, use that, don't search for others Mark Wielaard
2020-06-19 17:03   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 05/10] libdwfl: Flag an error if CIE return_address_register is invalid Mark Wielaard
2020-06-19 17:58   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors Mark Wielaard
2020-06-19 18:52   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 07/10] debuginfod: Handle not being able to fopen interval_path Mark Wielaard
2020-06-24 15:04   ` Mark Wielaard
2020-06-16 22:25 ` [PATCH 08/10] debuginfod: Make sure suffix can place zero terminator when copying filename Mark Wielaard
2020-06-16 22:25 ` [PATCH 09/10] debuginfod: Fix build_id hexadecimal length check Mark Wielaard
2020-06-16 22:25 ` [PATCH 10/10] debuginfod: Make sure handle_data can be allocated and is always freed Mark Wielaard
2020-06-19 11:43 ` [PATCH 01/10] backends: Make the reloc_nametable zero element an one char array Mark Wielaard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).