[PATCH 0/7] libctf: leak-adjacent fixes

[PATCH 0/7] libctf: leak-adjacent fixes

[Nick Alcock]

This series fixes a bad memory leak on a libctf error path, fixes a couple
of smaller problems on the way, and soups up the testsuite a bit to make it
easier to write the rather nasty test that should prevent this leak from
recurring.

I likely won't be committing it for a couple of weeks, and probably not
until I've added more to the series, but this is posted now anyway in case
anyone wants to take a look and e.g. perhaps tell me that my malloc
interposition skills are simply dreadful :)

Nick Alcock (7):
  libctf: typos
  libctf: failure to open parent dicts that exist should be an error
  libctf: ctf_archive_iter: fix tiny leak
  libctf: test: add lookup_link
  libctf: test: add host
  libctf: test: add wrapper
  libctf: fix leak of entire dict when dict opening fails

 libctf/ctf-archive.c                          |  29 ++-
 libctf/ctf-open.c                             |   8 +-
 libctf/testsuite/lib/ctf-lib.exp              |  28 ++-
 .../libctf-regression/open-error-free.c       | 185 ++++++++++++++++++
 .../libctf-regression/open-error-free.lk      |   5 +
 5 files changed, 244 insertions(+), 11 deletions(-)
 create mode 100644 libctf/testsuite/libctf-regression/open-error-free.c
 create mode 100644 libctf/testsuite/libctf-regression/open-error-free.lk

-- 
2.44.0.273.ge0bd14271f

[PATCH 1/7] libctf: typos

[Nick Alcock]

Some functions were renamed without the comments catching up.

libctf/
	* ctf-open.c (upgrade_types_v1): Fix comment typos.
---
 libctf/ctf-open.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c
index 9cbf07626cc..03faf2d886f 100644
--- a/libctf/ctf-open.c
+++ b/libctf/ctf-open.c
@@ -438,7 +438,7 @@ upgrade_types_v1 (ctf_dict_t *fp, ctf_header_t *cth)
   tbuf = (ctf_type_v1_t *) (fp->ctf_buf + cth->cth_typeoff);
   tend = (ctf_type_v1_t *) (fp->ctf_buf + cth->cth_stroff);
 
-  /* Much like init_types(), this is a two-pass process.
+  /* Much like init_static_types(), this is a two-pass process.
 
      First, figure out the new type-section size needed.  (It is possible,
      in theory, for it to be less than the old size, but this is very
@@ -636,7 +636,7 @@ upgrade_types_v1 (ctf_dict_t *fp, ctf_header_t *cth)
 
   /* Verify that the entire region was converted.  If not, we are either
      converting too much, or too little (leading to a buffer overrun either here
-     or at read time, in init_types().) */
+     or at read time, in init_static_types().) */
 
   assert ((size_t) t2p - (size_t) fp->ctf_buf == cth->cth_stroff);
 
-- 
2.44.0.273.ge0bd14271f

[PATCH 2/7] libctf: failure to open parent dicts that exist should be an error

[Nick Alcock]

CTF archive member opening (via ctf_arc_open_by_name, ctf_archive_iter, et
al) attempts to be helpful and auto-open and import any needed parent dict
in the same archive.  But if this fails, the error is not reported but
simply discarded, and you silently get back a dict with no parent, that
*you* suddenly have to remember to import.

This is not helpful behaviour: if the parent is corrupted or we run out of
memory or something, the caller is going to want to know!  Split it in two:
if the dict cites a parent that doesn't exist at all (a lot of historic
dicts name "PARENT" as their parent, even when they're not even children, or
perhaps the parent dict is stored separately and you plan to manually
associate it), we skip it as now, but if the import fails with an actual
error other than ECTF_ARNNAME, return the error and fail the open.

libctf/
	* ctf-archive.c (ctf_arc_import_parent):  Return failure if
        parent opening fails for reasons other thnn nonexistence.
	(ctf_dict_open_sections): Adjust.
---
 libctf/ctf-archive.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/libctf/ctf-archive.c b/libctf/ctf-archive.c
index 3f12c0da85d..451d6c69735 100644
--- a/libctf/ctf-archive.c
+++ b/libctf/ctf-archive.c
@@ -44,7 +44,8 @@ static void *arc_mmap_file (int fd, size_t size);
 static int arc_mmap_writeout (int fd, void *header, size_t headersz,
 			      const char **errmsg);
 static int arc_mmap_unmap (void *header, size_t headersz, const char **errmsg);
-static void ctf_arc_import_parent (const ctf_archive_t *arc, ctf_dict_t *fp);
+static int ctf_arc_import_parent (const ctf_archive_t *arc, ctf_dict_t *fp,
+				  int *errp);
 
 /* Flag to indicate "symbol not present" in ctf_archive_internal.ctfi_symdicts
    and ctfi_symnamedicts.  Never initialized.  */
@@ -602,7 +603,11 @@ ctf_dict_open_sections (const ctf_archive_t *arc,
       if (ret)
 	{
 	  ret->ctf_archive = (ctf_archive_t *) arc;
-	  ctf_arc_import_parent (arc, ret);
+	  if (ctf_arc_import_parent (arc, ret, errp) < 0)
+	    {
+	      ctf_dict_close (ret);
+	      return NULL;
+	    }
 	}
       return ret;
     }
@@ -757,19 +762,26 @@ ctf_arc_open_by_name_sections (const ctf_archive_t *arc,
    already set, and a suitable archive member exists.  No error is raised if
    this is not possible: this is just a best-effort helper operation to give
    people useful dicts to start with.  */
-static void
-ctf_arc_import_parent (const ctf_archive_t *arc, ctf_dict_t *fp)
+static int
+ctf_arc_import_parent (const ctf_archive_t *arc, ctf_dict_t *fp, int *errp)
 {
   if ((fp->ctf_flags & LCTF_CHILD) && fp->ctf_parname && !fp->ctf_parent)
     {
+      int err;
       ctf_dict_t *parent = ctf_dict_open_cached ((ctf_archive_t *) arc,
-						 fp->ctf_parname, NULL);
+						 fp->ctf_parname, &err);
+      if (errp)
+	*errp = err;
+
       if (parent)
 	{
 	  ctf_import (fp, parent);
 	  ctf_dict_close (parent);
 	}
+      else if (err != ECTF_ARNNAME)
+	return -1;				/* errno is set for us.  */
     }
+  return 0;
 }
 
 /* Return the number of members in an archive.  */
-- 
2.44.0.273.ge0bd14271f

[PATCH 3/7] libctf: ctf_archive_iter: fix tiny leak

[Nick Alcock]

If iteration fails because opening a dict has failed, ctf_archive_next does
not destroy the iterator, so the caller can keep going and try to open other
dicts further into the archive.  ctf_archive_iter just returns, though, so
it should free the iterator rather than leaking it.

libctf/
	* ctf-archive.c (ctf_archive_iter): Don't leak the iterator on
	failure.
---
 libctf/ctf-archive.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libctf/ctf-archive.c b/libctf/ctf-archive.c
index 451d6c69735..f459c02e702 100644
--- a/libctf/ctf-archive.c
+++ b/libctf/ctf-archive.c
@@ -1063,7 +1063,7 @@ ctf_archive_iter (const ctf_archive_t *arc, ctf_archive_member_f *func,
   ctf_next_t *i = NULL;
   ctf_dict_t *fp;
   const char *name;
-  int err;
+  int err = 0;
 
   while ((fp = ctf_archive_next (arc, &i, &name, 0, &err)) != NULL)
     {
@@ -1077,6 +1077,11 @@ ctf_archive_iter (const ctf_archive_t *arc, ctf_archive_member_f *func,
 	}
       ctf_dict_close (fp);
     }
+  if (err != ECTF_NEXT_END && err != 0)
+    {
+      ctf_next_destroy (i);
+      return -1;
+    }
   return 0;
 }
 
-- 
2.44.0.273.ge0bd14271f

[PATCH 4/7] libctf: test: add lookup_link

[Nick Alcock]

This .lk option lets you link the lookup program with extra libraries
in addition to -lctf.

libctf/
	* testsuite/lib/ctf-lib.exp (run_lookup_test): Add lookup_link.
---
 libctf/testsuite/lib/ctf-lib.exp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libctf/testsuite/lib/ctf-lib.exp b/libctf/testsuite/lib/ctf-lib.exp
index 3df76198d66..523a8c31e1e 100644
--- a/libctf/testsuite/lib/ctf-lib.exp
+++ b/libctf/testsuite/lib/ctf-lib.exp
@@ -100,6 +100,9 @@ proc compile_link_one_host_cc { src output additional_args } {
 #   link_flags:
 #	If set, extra flags to pass to the linker.
 #
+#   lookup_link:
+#       If set, extra libraries to link the lookup program with.
+#
 #   xfail: GLOB|PROC ...
 #	This test is expected to fail on a specified list of targets.
 #
@@ -137,6 +140,7 @@ proc run_lookup_test { name } {
     set shared "-shared"
     set opts(link) {}
     set opts(link_flags) {}
+    set opts(lookup_link) {}
     set opts(nonshared) {}
     set opts(lookup) {}
     set opts(name) {}
@@ -191,7 +195,7 @@ proc run_lookup_test { name } {
     }
 
     # Compile and link the lookup program.
-    set comp_output [prune_warnings [compile_link_one_host_cc $opts(lookup) "tmpdir/lookup" "libctf.la"]]
+    set comp_output [prune_warnings [compile_link_one_host_cc $opts(lookup) "tmpdir/lookup" "libctf.la $opts(lookup_link)"]]
 
     if { $comp_output != ""} {
 	send_log "compilation of lookup program $opts(lookup) failed with <$comp_output>"
-- 
2.44.0.273.ge0bd14271f

[PATCH 5/7] libctf: test: add host

[Nick Alcock]

This .lk option lets you execute particular tests only on specific host
architectures.

libctf/
	* testsuite/lib/ctf-lib.exp (run_lookup_test): Add host.
---
 libctf/testsuite/lib/ctf-lib.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/libctf/testsuite/lib/ctf-lib.exp b/libctf/testsuite/lib/ctf-lib.exp
index 523a8c31e1e..1f76e0b1aba 100644
--- a/libctf/testsuite/lib/ctf-lib.exp
+++ b/libctf/testsuite/lib/ctf-lib.exp
@@ -109,6 +109,9 @@ proc compile_link_one_host_cc { src output additional_args } {
 #   no_cross:
 #       If set, do not run this test when host != target.
 #
+#   host:
+#       If set, only run this test on hosts matching the given glob.
+#
 # Each option may occur at most once unless otherwise mentioned.
 #
 # After the option lines come regexp lines.  run_lookup_test calls
@@ -147,6 +150,7 @@ proc run_lookup_test { name } {
     set opts(source) {}
     set opts(xfail) {}
     set opts(no_cross) {}
+    set opts(host) {}
 
     foreach i $opt_array {
 	set opt_name [lindex $i 0]
@@ -170,6 +174,11 @@ proc run_lookup_test { name } {
 	return
     }
 
+    if { [llength $opts(host)] != 0 && ![ishost $opts(host)] } {
+	untested "$subdiir/$name only runs on $opts(host)"
+	return
+    }
+
     if { [llength $opts(lookup)] == 0 } {
 	set opts(lookup) "$file.c"
     } else {
-- 
2.44.0.273.ge0bd14271f

[PATCH 6/7] libctf: test: add wrapper

[Nick Alcock]

This .lk option lets you run the lookup program via a wrapper executable.
For example, to run under valgrind and check for leaks (albeit noisily
because of the libtool shell script wrapper):

libctf/
	* testsuite/lib/ctf-lib.exp (run_lookup_test): Add wrapper.
---
 libctf/testsuite/lib/ctf-lib.exp | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/libctf/testsuite/lib/ctf-lib.exp b/libctf/testsuite/lib/ctf-lib.exp
index 1f76e0b1aba..20bcf0c8bbe 100644
--- a/libctf/testsuite/lib/ctf-lib.exp
+++ b/libctf/testsuite/lib/ctf-lib.exp
@@ -112,6 +112,10 @@ proc compile_link_one_host_cc { src output additional_args } {
 #   host:
 #       If set, only run this test on hosts matching the given glob.
 #
+#   wrapper:
+#       Wrap invocations of LOOKUP in this command.  (Useful for valgrind
+#       invocations, etc.)
+#
 # Each option may occur at most once unless otherwise mentioned.
 #
 # After the option lines come regexp lines.  run_lookup_test calls
@@ -151,6 +155,7 @@ proc run_lookup_test { name } {
     set opts(xfail) {}
     set opts(no_cross) {}
     set opts(host) {}
+    set opts(wrapper) {}
 
     foreach i $opt_array {
 	set opt_name [lindex $i 0]
@@ -257,9 +262,13 @@ proc run_lookup_test { name } {
 	}
     }
 
-    # Invoke the lookup program on the outputs.
+    # Invoke the lookup program on the outputs, possibly through the wrapper.
 
-    set results [run_host_cmd tmpdir/lookup $lookup_output]
+    if { [llength $opts(wrapper)] == 0 } {
+	set results [run_host_cmd tmpdir/lookup $lookup_output]
+    } else {
+	set results [run_host_cmd "$opts(wrapper) tmpdir/lookup" $lookup_output]
+    }
 
     set f [open "tmpdir/lookup.out" "w"]
     puts $f $results
-- 
2.44.0.273.ge0bd14271f

[PATCH 7/7] libctf: fix leak of entire dict when dict opening fails

[Nick Alcock]

Ever since commit 1fa7a0c24e78e7f ("libctf: sort out potential refcount
loops") ctf_dict_close has only freed anything if the refcount on entry
to the function is precisely 1.  >1 obviously just decrements the
refcount, but the linker machinery can sometimes cause freeing to recurse
from a dict to another dict and then back to the first dict again, so
we interpret a refcount of 0 as an indication that this is a recursive call
and we should just return, because a caller is already freeing this dict.

Unfortunately there is one situation in which this is not true: the bad:
codepath in ctf_bufopen entered when opening fails.  Because the refcount is
bumped only at the very end of ctf_bufopen, any failure causes
ctf_dict_close to be entered with a refcount of zero, and it frees nothing
and we leak the entire dict.

The solution is to bump the refcount to 1 right before freeing... but this
codepath is clearly delicate enough that we need to properly validate it,
so we add a test that uses malloc interposition to count allocations and
frees, creates a dict, writes it out, intentionally corrupts it (by setting
a bunch of bytes after the header to a value high enough that it is
definitely not a valid CTF type kind), then tries to open it again and
counts the malloc/free pairs to make sure they're matched.  (Test run only
on *-linux-gnu, because malloc interposition is not a thing you can rely
upon working everywhere, and this test is not arch-dependent so if it
passes on one arch it can be assumed to pass on all of them.)

libctf/
	* ctf-open.c (ctf_bufopen): Bump the refcount on failure.
	* testsuite/libctf-regression/open-error-free.*: New test.
---
 libctf/ctf-open.c                             |   4 +
 .../libctf-regression/open-error-free.c       | 185 ++++++++++++++++++
 .../libctf-regression/open-error-free.lk      |   5 +
 3 files changed, 194 insertions(+)
 create mode 100644 libctf/testsuite/libctf-regression/open-error-free.c
 create mode 100644 libctf/testsuite/libctf-regression/open-error-free.lk

diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c
index 03faf2d886f..59c6ed0622a 100644
--- a/libctf/ctf-open.c
+++ b/libctf/ctf-open.c
@@ -1724,6 +1724,10 @@ ctf_bufopen (const ctf_sect_t *ctfsect, const ctf_sect_t *symsect,
 bad:
   ctf_set_open_errno (errp, err);
   ctf_err_warn_to_open (fp);
+  /* Without this, the refcnt is zero on entry and ctf_dict_close() won't
+     actually do anything on the grounds that this is a recursive call via
+     another dict being closed.  */
+  fp->ctf_refcnt = 1;
   ctf_dict_close (fp);
   return NULL;
 }
diff --git a/libctf/testsuite/libctf-regression/open-error-free.c b/libctf/testsuite/libctf-regression/open-error-free.c
new file mode 100644
index 00000000000..8319a09d5f5
--- /dev/null
+++ b/libctf/testsuite/libctf-regression/open-error-free.c
@@ -0,0 +1,185 @@
+/* Make sure that, on error, an opened dict is properly freed.  */
+
+#define _GNU_SOURCE 1
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctf-api.h>
+#include <ctf.h>
+
+static unsigned long long malloc_count;
+static unsigned long long free_count;
+
+static void *(*real_malloc) (size_t size);
+static void (*real_free) (void *ptr);
+static void *(*real_realloc) (void *ptr, size_t size);
+static void *(*real_calloc) (size_t nmemb, size_t size);
+
+/* Interpose malloc/free functionss and count calls to spot unbalanced ones.
+   Extra complexity to deal with dlsym() calling dlerror() and thus calloc() --
+   luckily it handles malloc failure fine, so we can just always fail before the
+   hooks are installed.  */
+
+static int in_hooks;
+
+static void hook_init (void)
+{
+  if (!real_calloc)
+    {
+      in_hooks = 1;
+      real_calloc = (void *(*) (size_t, size_t)) dlsym (RTLD_NEXT, "calloc");
+      real_malloc = (void *(*) (size_t)) dlsym (RTLD_NEXT, "malloc");
+      real_free = (void (*) (void *)) dlsym (RTLD_NEXT, "free");
+      real_realloc = (void *(*) (void *, size_t)) dlsym (RTLD_NEXT, "realloc");
+      if (!real_malloc || !real_free || !real_realloc || !real_calloc)
+	{
+	  fprintf (stderr, "Cannot hook malloc\n");
+	  exit(1);
+	}
+      in_hooks = 0;
+    }
+}
+
+void *malloc (size_t size)
+{
+  if (in_hooks)
+    return NULL;
+
+  hook_init();
+  malloc_count++;
+  return real_malloc (size);
+}
+
+void *realloc (void *ptr, size_t size)
+{
+  void *new_ptr;
+
+  if (in_hooks)
+    return NULL;
+
+  hook_init();
+  new_ptr = real_realloc (ptr, size);
+
+  if (!ptr)
+    malloc_count++;
+
+  if (size == 0)
+    free_count++;
+
+  return new_ptr;
+}
+
+void *calloc (size_t nmemb, size_t size)
+{
+  void *ptr;
+
+  if (in_hooks)
+    return NULL;
+
+  hook_init();
+  ptr = real_calloc (nmemb, size);
+
+  if (ptr)
+    malloc_count++;
+  return ptr;
+}
+
+void free (void *ptr)
+{
+  hook_init();
+
+  if (in_hooks)
+    return;
+
+  if (ptr != NULL)
+    free_count++;
+
+  return real_free (ptr);
+}
+
+int main (void)
+{
+  ctf_dict_t *fp;
+  ctf_encoding_t e = { CTF_INT_SIGNED, 0, sizeof (long) };
+  int err;
+  ctf_id_t type;
+  size_t i;
+  char *written;
+  size_t written_size;
+  char *foo;
+  ctf_next_t *it = NULL;
+  unsigned long long frozen_malloc_count, frozen_free_count;
+
+  if ((fp = ctf_create (&err)) == NULL)
+    goto open_err;
+
+  /* Define an integer, then a pile of unconnected pointers to it, just to
+     use up space..  */
+
+  if ((type = ctf_add_integer (fp, CTF_ADD_ROOT, "long", &e)) == CTF_ERR)
+    goto err;
+
+  for (i = 0; i < 100; i++)
+    {
+      if (ctf_add_pointer (fp, CTF_ADD_ROOT, type) == CTF_ERR)
+	goto err;
+    }
+
+  /* Write the dict out, uncompressed (to stop it failing to open due to decompression
+     failure after we corrupt it: the leak is only observable if the dict gets
+     malloced, which only happens after that point.)  */
+
+  if ((written = ctf_write_mem (fp, &written_size, (size_t) -1)) == NULL)
+    goto write_err;
+
+  ctf_dict_close (fp);
+
+  /* Corrupt the dict.  */
+
+  memset (written + sizeof (ctf_header_t), 64, 64);
+
+  /* Reset the counters: we are interested only in leaks at open
+     time.  */
+  malloc_count = 0;
+  free_count = 0;
+
+  if ((ctf_simple_open (written, written_size, NULL, 0, 0, NULL, 0, &err)) != NULL)
+    {
+      fprintf (stderr, "wildly corrupted dict still opened OK?!\n");
+      exit (1);
+    }
+
+  /* The error log will have accumulated errors which need to be
+     consumed and freed if they are not to appear as a spurious leak.  */
+
+  while ((foo = ctf_errwarning_next (NULL, &it, NULL, NULL)) != NULL)
+    free (foo);
+
+  frozen_malloc_count = malloc_count;
+  frozen_free_count = free_count;
+
+  if (frozen_malloc_count == 0)
+    fprintf (stderr, "Allocation count after failed open is zero: likely hook failure.\n");
+  else if (frozen_malloc_count > frozen_free_count)
+    fprintf (stderr, "Memory leak is present: %lli allocations (%lli allocations, %lli frees).\n",
+	     frozen_malloc_count - frozen_free_count, frozen_malloc_count, frozen_free_count);
+  else if (frozen_malloc_count < frozen_free_count)
+    fprintf (stderr, "Possible double-free: %li allocations, %li frees.\n",
+	     frozen_malloc_count, frozen_free_count);
+
+  printf ("All OK.\n");
+  exit (0);
+
+ open_err:
+  fprintf (stderr, "Cannot open/create: %s\n", ctf_errmsg (err));
+  exit (1);
+
+ err:
+  fprintf (stderr, "Cannot add: %s\n", ctf_errmsg (ctf_errno (fp)));
+  exit (1);
+
+ write_err:
+  fprintf (stderr, "Cannot write: %s\n", ctf_errmsg (ctf_errno (fp)));
+  exit (1);
+}
diff --git a/libctf/testsuite/libctf-regression/open-error-free.lk b/libctf/testsuite/libctf-regression/open-error-free.lk
new file mode 100644
index 00000000000..383cfff20d5
--- /dev/null
+++ b/libctf/testsuite/libctf-regression/open-error-free.lk
@@ -0,0 +1,5 @@
+# source: pptrtab-a.c
+# source: pptrtab-b.c
+# host: *-linux-gnu
+# lookup-link: -ldl
+All OK.
-- 
2.44.0.273.ge0bd14271f