Some more GCC10 -fanalyzer inspired patches

Some more GCC10 -fanalyzer inspired patches

[Mark Wielaard]

I did another build with the final GCC10 and -fanalyzer. The
-Wanalyzer-use-of-uninitialized-value option was removed, which caused
a lot of false positives. Without those it was easier to identify some
real issues. I also tried -fanalyze together with -flto. This takes a
lot of memory (linking libdw.so uses > 12GB) but does allow -fanalyzer
to detect some cross-function issues.

[PATCH 1/7] libdwfl: Cleanup user_core resources on failure in
[PATCH 2/7] tests: Make sure to not call memcmp with NULL arguments.
[PATCH 3/7] libelf: Check __gelf_getehdr_rdlock call doesn't fail in
[PATCH 4/7] libelf: Check for NULL shdr in elf_strptr.
[PATCH 5/7] src: Check ebl_openbackend result before using ebl
[PATCH 6/7] libdwfl: Return failure from dwfl_standard_find_debuginfo
[PATCH 7/7] libcpu: Free unused new bitfield on error in i386_parse.y

I think they all are for real issues, although probably fairly hard
to trigger. All are somewhat trivial and I intent to check them in soon.

Cheers,

Mark

[PATCH 1/7] libdwfl: Cleanup user_core resources on failure in dwfl_core_file_report.

[Mark Wielaard]

GCC10 -fanalyzer noticed that we allocate, but don't always cleanup the
dwfl->user_core if it wasn't set yet on error. In theory dwfl_module_end
should take care of it, but it is cleaner and less confusing to just do
it here.

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

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 3f9cd665..05d5bd4a 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-08  Mark Wielaard  <mark@klomp.org>
+
+	* libdwfl/core-file.c (dwfl_core_file_report): Keep track of
+	new bool cleanup_user_core and cleanup dwfl->user_core in error
+	case.
+
 2020-04-30  Mark Wielaard  <mark@klomp.org>
 
 	* find-debuginfo.c (dwfl_standard_find_debuginfo): When mod->dw
diff --git a/libdwfl/core-file.c b/libdwfl/core-file.c
index 01109f4b..a0ccc9b3 100644
--- a/libdwfl/core-file.c
+++ b/libdwfl/core-file.c
@@ -450,6 +450,7 @@ dwfl_core_file_report (Dwfl *dwfl, Elf *elf, const char *executable)
       return -1;
     }
 
+  bool cleanup_user_core = false;
   if (dwfl->user_core != NULL)
     free (dwfl->user_core->executable_for_core);
   if (executable == NULL)
@@ -461,6 +462,7 @@ dwfl_core_file_report (Dwfl *dwfl, Elf *elf, const char *executable)
     {
       if (dwfl->user_core == NULL)
 	{
+	  cleanup_user_core = true;
 	  dwfl->user_core = calloc (1, sizeof (struct Dwfl_User_Core));
 	  if (dwfl->user_core == NULL)
 	    {
@@ -472,6 +474,11 @@ dwfl_core_file_report (Dwfl *dwfl, Elf *elf, const char *executable)
       dwfl->user_core->executable_for_core = strdup (executable);
       if (dwfl->user_core->executable_for_core == NULL)
 	{
+	  if (cleanup_user_core)
+	    {
+	      free (dwfl->user_core);
+	      dwfl->user_core = NULL;
+	    }
 	  __libdwfl_seterrno (DWFL_E_NOMEM);
 	  return -1;
 	}
@@ -481,7 +488,15 @@ dwfl_core_file_report (Dwfl *dwfl, Elf *elf, const char *executable)
   GElf_Phdr notes_phdr;
   int ndx = dwfl_report_core_segments (dwfl, elf, phnum, &notes_phdr);
   if (unlikely (ndx <= 0))
-    return ndx;
+    {
+      if (cleanup_user_core)
+	{
+	  free (dwfl->user_core->executable_for_core);
+	  free (dwfl->user_core);
+	  dwfl->user_core = NULL;
+	}
+      return ndx;
+    }
 
   /* Next, we should follow the chain from DT_DEBUG.  */
 
-- 
2.20.1

[PATCH 2/7] tests: Make sure to not call memcmp with NULL arguments.

[Mark Wielaard]

GCC10 -fanalyzer thinks we are too clever:

elfputzdata.c: In function ‘main’:
elfputzdata.c:178:8: warning: use of possibly-NULL ‘orig_buf’ where
                     non-null expected [CWE-690]
                     [-Wanalyzer-possible-null-argument]
  178 |     && memcmp (orig_buf, d->d_buf, orig_size) == 0)

orig_buf can only be NULL when orig_size is zero, but it might still
be undefined behaviour. So don't try to be too smart and just check
whether we actually have an buffer.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 tests/ChangeLog     |  5 +++++
 tests/elfputzdata.c | 21 +++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 301b0fb6..083e138d 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-08  Mark Wielaard  <mark@klomp.org>
+
+	* elfputzdata.c (main): Explicitly check orig_buf is not NULL
+	before calling memcmp.
+
 2020-04-17  Mark Wielaard  <mark@klomp.org>
 
 	* test-subr.sh (testrun_on_self_obj): New function.
diff --git a/tests/elfputzdata.c b/tests/elfputzdata.c
index 66ab77ba..0d9c020e 100644
--- a/tests/elfputzdata.c
+++ b/tests/elfputzdata.c
@@ -105,14 +105,17 @@ main (int argc, char *argv[])
 		  printf ("Unexpected data size for orig section %zd\n", idx);
 		  return -1;
 		}
-	      char *orig_buf = malloc (d->d_size);
-	      if (orig_size > 0 && orig_buf == NULL)
+	      char *orig_buf = NULL;
+	      if (orig_size > 0)
 		{
-		  printf ("No memory to copy section %zd data\n", idx);
-		  return -1;
+		  orig_buf = malloc (d->d_size);
+		  if (orig_buf == NULL)
+		    {
+		      printf ("No memory to copy section %zd data\n", idx);
+		      return -1;
+		    }
+		  memcpy (orig_buf, d->d_buf, orig_size);
 		}
-	      if (orig_size > 0)
-		memcpy (orig_buf, d->d_buf, orig_size);
 
 	      bool forced = false;
 	      if (gnu)
@@ -175,7 +178,8 @@ main (int argc, char *argv[])
 		}
 
 	      if (new_size == orig_size
-		  && memcmp (orig_buf, d->d_buf, orig_size) == 0)
+		  && (orig_buf == NULL
+		      || memcmp (orig_buf, d->d_buf, orig_size) == 0))
 		{
 		  printf ("section %zd didn't compress\n", idx);
 		  return -1;
@@ -211,7 +215,8 @@ main (int argc, char *argv[])
 		  return -1;
 		}
 	      if (newer_size != orig_size
-		  && memcmp (orig_buf, d->d_buf, orig_size) != 0)
+		  && (orig_buf == NULL
+		      || memcmp (orig_buf, d->d_buf, orig_size) != 0))
 		{
 		  printf ("section %zd didn't correctly uncompress\n", idx);
 		  return -1;
-- 
2.20.1

[PATCH 3/7] libelf: Check __gelf_getehdr_rdlock call doesn't fail in elf_getdata.

[Mark Wielaard]

GCC10 -fanalyzer with -flto notices __gelf_getehdr_rdlock can fail
and that the result of the call in __libelf_set_rawdata_wrlock isn't
checked, which can cause a dereference of NULL.

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

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 56f5354c..fcea8aa9 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-08  Mark Wielaard  <mark@klomp.org>
+
+	* elf_getdata.c (__libelf_set_rawdata_wrlock): Check
+	__gelf_getehdr_rdlock return value.
+
 2020-04-25  Mark Wielaard  <mark@klomp.org>
 
 	* elf_compress.c (__libelf_compress): Remove free (out_buf).
diff --git a/libelf/elf_getdata.c b/libelf/elf_getdata.c
index 40fe1694..0d8f8d2e 100644
--- a/libelf/elf_getdata.c
+++ b/libelf/elf_getdata.c
@@ -271,6 +271,8 @@ __libelf_set_rawdata_wrlock (Elf_Scn *scn)
 	{
 	  GElf_Ehdr ehdr_mem;
 	  GElf_Ehdr *ehdr = __gelf_getehdr_rdlock (elf, &ehdr_mem);
+	  if (unlikely (ehdr == NULL))
+	    return 1;
 	  entsize = SH_ENTSIZE_HASH (ehdr);
 	}
       else
-- 
2.20.1

[PATCH 4/7] libelf: Check for NULL shdr in elf_strptr.

[Mark Wielaard]

GCC10 -fanalyzer with -flto notices __elf64_getshdr_rdlock can fail
and because the result isn't checked in elf_strptr it can cause a
dereference of NULL.

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

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index fcea8aa9..fd5518dc 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,7 @@
+2020-05-08  Mark Wielaard  <mark@klomp.org>
+
+	* elf_strptr.c (elf_strptr): Check shdr is not NULL.
+
 2020-05-08  Mark Wielaard  <mark@klomp.org>
 
 	* elf_getdata.c (__libelf_set_rawdata_wrlock): Check
diff --git a/libelf/elf_strptr.c b/libelf/elf_strptr.c
index e72a3a36..c7271707 100644
--- a/libelf/elf_strptr.c
+++ b/libelf/elf_strptr.c
@@ -145,7 +145,7 @@ elf_strptr (Elf *elf, size_t idx, size_t offset)
   else
     {
       Elf64_Shdr *shdr = strscn->shdr.e64 ?: __elf64_getshdr_rdlock (strscn);
-      if (unlikely (shdr->sh_type != SHT_STRTAB))
+      if (unlikely (shdr == NULL || shdr->sh_type != SHT_STRTAB))
 	{
 	  /* This is no string section.  */
 	  __libelf_seterrno (ELF_E_INVALID_SECTION);
-- 
2.20.1

[PATCH 5/7] src: Check ebl_openbackend result before using ebl handle.

[Mark Wielaard]

GCC10 -fanalyzer plus -flto sees that ebl_openbackend can fail and
return NULL. Most of the time we will get a dummy ebl, but in case
of out of memory or corrupt ELF header it might return NULL. Make
sure that we report a (fatal) error in that case in all tools.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ChangeLog |  7 +++++++
 src/elflint.c |  9 ++++++++-
 src/nm.c      | 16 +++++++++-------
 src/objdump.c |  3 +++
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 83d58607..8c72e7d1 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,10 @@
+2020-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* elflint.c (process_elf_file): Error out if ebl_openbackend fails.
+	* objdump.c (handle_elf): Likewise.
+	* nm.c (handle_elf): Likewise. Move full name string construction
+	forward, so it can be used in the error message.
+
 2020-04-17  Mark Wielaard  <mark@klomp.org>
 
 	* readelf.c (print_debug): Check .gnu.debuglto_ prefix.
diff --git a/src/elflint.c b/src/elflint.c
index 0ef43236..6ad9bc42 100644
--- a/src/elflint.c
+++ b/src/elflint.c
@@ -4775,7 +4775,14 @@ process_elf_file (Elf *elf, const char *prefix, const char *suffix,
   ebl = ebl_openbackend (elf);
   /* If there is no appropriate backend library we cannot test
      architecture and OS specific features.  Any encountered extension
-     is an error.  */
+     is an error.  Often we'll get a "dummy" ebl, except if something
+     really bad happen, like a totally corrupted ELF file or out of
+     memory situation.  */
+  if (ebl == NULL)
+    {
+      ERROR (gettext ("cannot create backend for ELF file\n"));
+      return;
+    }
 
   /* Go straight by the gABI, check all the parts in turn.  */
   check_elf_header (ebl, ehdr, size);
diff --git a/src/nm.c b/src/nm.c
index b7c2aed6..f6ca3b0a 100644
--- a/src/nm.c
+++ b/src/nm.c
@@ -1510,8 +1510,17 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
   GElf_Ehdr *ehdr;
   Ebl *ebl;
 
+  /* Create the full name of the file.  */
+  if (prefix != NULL)
+    cp = mempcpy (cp, prefix, prefix_len);
+  cp = mempcpy (cp, fname, fname_len);
+  if (suffix != NULL)
+    memcpy (cp - 1, suffix, suffix_len + 1);
+
   /* Get the backend for this object file type.  */
   ebl = ebl_openbackend (elf);
+  if (ebl == NULL)
+    INTERNAL_ERROR (fullname);
 
   /* We need the ELF header in a few places.  */
   ehdr = gelf_getehdr (elf, &ehdr_mem);
@@ -1530,13 +1539,6 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
       goto out;
     }
 
-  /* Create the full name of the file.  */
-  if (prefix != NULL)
-    cp = mempcpy (cp, prefix, prefix_len);
-  cp = mempcpy (cp, fname, fname_len);
-  if (suffix != NULL)
-    memcpy (cp - 1, suffix, suffix_len + 1);
-
   /* Find the symbol table.
 
      XXX Can there be more than one?  Do we print all?  Currently we do.  */
diff --git a/src/objdump.c b/src/objdump.c
index a619674f..82d7bcf6 100644
--- a/src/objdump.c
+++ b/src/objdump.c
@@ -755,6 +755,9 @@ handle_elf (Elf *elf, const char *prefix, const char *fname,
 
   /* Get the backend for this object file type.  */
   Ebl *ebl = ebl_openbackend (elf);
+  if (ebl == NULL)
+    error (EXIT_FAILURE, 0,
+	   gettext ("cannot create backend for elf file"));
 
   printf ("%s: elf%d-%s\n\n",
 	  fname, gelf_getclass (elf) == ELFCLASS32 ? 32 : 64,
-- 
2.20.1

[PATCH 6/7] libdwfl: Return failure from dwfl_standard_find_debuginfo for NULL module.

[Mark Wielaard]

GCC10 -fanalyzer plus -flto notices that some functions called by
dwfl_standard_find_debuginfo check that the given module isn't NULL,
but others expect it to be non-NULL. Just return a failure immediately
when a NULL mod is passed to dwfl_standard_find_debuginfo.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdwfl/ChangeLog        | 5 +++++
 libdwfl/find-debuginfo.c | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 05d5bd4a..4f1ec9da 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* find-debuginfo.c (dwfl_standard_find_debuginfo): Return failure
+	when mod is NULL.
+
 2020-05-08  Mark Wielaard  <mark@klomp.org>
 
 	* libdwfl/core-file.c (dwfl_core_file_report): Keep track of
diff --git a/libdwfl/find-debuginfo.c b/libdwfl/find-debuginfo.c
index 4cfd0b8b..eb68d549 100644
--- a/libdwfl/find-debuginfo.c
+++ b/libdwfl/find-debuginfo.c
@@ -355,6 +355,9 @@ dwfl_standard_find_debuginfo (Dwfl_Module *mod,
 			      GElf_Word debuglink_crc,
 			      char **debuginfo_file_name)
 {
+  if (mod == NULL)
+    return -1;
+
   /* First try by build ID if we have one.  If that succeeds or fails
      other than just by finding nothing, that's all we do.  */
   const unsigned char *bits = NULL;
-- 
2.20.1

[PATCH 7/7] libcpu: Free unused new bitfield on error in i386_parse.y new_bitfield.

[Mark Wielaard]

GCC10 -fanalyzer detected we didn't free the newly created bitfield
on error. Make sure to free it before returning.

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

diff --git a/libcpu/ChangeLog b/libcpu/ChangeLog
index a8b2b951..a342b7f6 100644
--- a/libcpu/ChangeLog
+++ b/libcpu/ChangeLog
@@ -1,3 +1,7 @@
+2020-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* i386_parse.y (new_bitfield): Call free newp on error.
+
 2020-04-16  Mark Wielaard  <mark@klomp.org>
 
 	* i386_disasm.c (i386_disasm): Replace assert with goto invalid_op
diff --git a/libcpu/i386_parse.y b/libcpu/i386_parse.y
index 910d5458..90c7bd93 100644
--- a/libcpu/i386_parse.y
+++ b/libcpu/i386_parse.y
@@ -579,6 +579,7 @@ new_bitfield (char *name, unsigned long int num)
       error (0, 0, "%d: duplicated definition of bitfield '%s'",
 	     i386_lineno, name);
       free (name);
+      free (newp);
       return;
     }
 
-- 
2.20.1

Re: Some more GCC10 -fanalyzer inspired patches

[Mark Wielaard]

On Sun, 2020-05-10 at 21:53 +0200, Mark Wielaard wrote:
> I did another build with the final GCC10 and -fanalyzer. The
> -Wanalyzer-use-of-uninitialized-value option was removed, which caused
> a lot of false positives. Without those it was easier to identify some
> real issues. I also tried -fanalyze together with -flto. This takes a
> lot of memory (linking libdw.so uses > 12GB) but does allow -fanalyzer
> to detect some cross-function issues.
> 
> [PATCH 1/7] libdwfl: Cleanup user_core resources on failure in
> [PATCH 2/7] tests: Make sure to not call memcmp with NULL arguments.
> [PATCH 3/7] libelf: Check __gelf_getehdr_rdlock call doesn't fail in
> [PATCH 4/7] libelf: Check for NULL shdr in elf_strptr.
> [PATCH 5/7] src: Check ebl_openbackend result before using ebl
> [PATCH 6/7] libdwfl: Return failure from dwfl_standard_find_debuginfo
> [PATCH 7/7] libcpu: Free unused new bitfield on error in i386_parse.y
> 
> I think they all are for real issues, although probably fairly hard
> to trigger. All are somewhat trivial and I intent to check them in soon.

I pushed all 7 patches to elfutils git master.