libctf: sort out potential refcount loops

When you link TUs that contain conflicting types together, the resulting
CTF section is an archive containing many CTF dicts.  These dicts appear
in ctf_link_outputs of the shared dict, with each ctf_import'ing that
shared dict.  ctf_importing a dict bumps its refcount to stop it going
away while it's in use -- but if the shared dict (whose refcount is
bumped) has the child dict (doing the bumping) in its ctf_link_outputs,
we have a refcount loop, since the child dict only un-ctf_imports and
drops the parent's refcount when it is freed, but the child is only
freed when the parent's refcount falls to zero.

(In the future, this will be able to go wrong on the inputs too, when an
ld -r'ed deduplicated output with conflicts is relinked.  Right now this
cannot happen because we don't ctf_import such dicts at all.  This will
be fixed in a later commit in this series.)

Fix this by introducing an internal-use-only ctf_import_unref function
that imports a parent dict *witthout* bumping the parent's refcount, and
using it when we create per-CU outputs.  This function is only safe to
use if you know the parent cannot go away while the child exists: but if
the parent *owns* the child, as here, this is necessarily true.

Record in the ctf_file_t whether a parent was imported via ctf_import or
ctf_import_unref, so that if you do another ctf_import later on (or a
ctf_import_unref) it can decide whether to drop the refcount of the
existing parent being replaced depending on which function you used to
import that one.  Adjust ctf_serialize so that rather than doing a
ctf_import (which is wrong if the original import was
ctf_import_unref'fed), we just copy the parent field and refcount over
and forcibly flip the unref flag on on the old copy we are going to
discard.

ctf_file_close also needs a bit of tweaking to only close the parent if
it was not imported with ctf_import_unref: while we're at it, guard
against repeated closes with a refcount of zero and stop them causing
double-frees, even if destruction of things freed *inside*
ctf_file_close cause such recursion.

Verified no leaks or accesses to freed memory after all of this with
valgrind.  (It was leak-happy before.)

libctf/
	* ctf-impl.c (ctf_file_t) <ctf_parent_unreffed>: New.
	(ctf_import_unref): New.
	* ctf-open.c (ctf_file_close) Drop the refcount all the way to
	zero.  Don't recurse back in if the refcount is already zero.
	(ctf_import): Check ctf_parent_unreffed before deciding whether
	to close a pre-existing parent.  Set it to zero.
	(ctf_import_unreffed): New, as above, setting
	ctf_parent_unreffed to 1.
	* ctf-create.c (ctf_serialize): Do not ctf_import into the new
	child: use direct assignment, and set unreffed on the new and
	old children.
	* ctf-link.c (ctf_create_per_cu): Import the parent using
	ctf_import_unreffed.

5 files changed, 67 insertions, 8 deletions

diff --git a/libctf/ChangeLog b/libctf/ChangeLog
index f756fc4..4830284 100644
--- a/libctf/ChangeLog
+++ b/libctf/ChangeLog
@@ -1,5 +1,21 @@
 2020-07-22  Nick Alcock  <nick.alcock@oracle.com>
 
+	* ctf-impl.c (ctf_file_t) <ctf_parent_unreffed>: New.
+	(ctf_import_unref): New.
+	* ctf-open.c (ctf_file_close) Drop the refcount all the way to
+	zero.  Don't recurse back in if the refcount is already zero.
+	(ctf_import): Check ctf_parent_unreffed before deciding whether
+	to close a pre-existing parent.  Set it to zero.
+	(ctf_import_unreffed): New, as above, setting
+	ctf_parent_unreffed to 1.
+	* ctf-create.c (ctf_serialize): Do not ctf_import into the new
+	child: use direct assignment, and set unreffed on the new and
+	old children.
+	* ctf-link.c (ctf_create_per_cu): Import the parent using
+	ctf_import_unreffed.
+
+2020-07-22  Nick Alcock  <nick.alcock@oracle.com>
+
 	* ctf-impl.h (ctf_link_type_mapping_key): Rename to...
 	(ctf_link_type_key): ... this, adjusting member prefixes to
 	match.
diff --git a/libctf/ctf-create.c b/libctf/ctf-create.c
index a538b2d..a964c20 100644
--- a/libctf/ctf-create.c
+++ b/libctf/ctf-create.c
@@ -516,8 +516,9 @@ ctf_serialize (ctf_file_t *fp)
     }
 
   (void) ctf_setmodel (nfp, ctf_getmodel (fp));
-  (void) ctf_import (nfp, fp->ctf_parent);
 
+  nfp->ctf_parent = fp->ctf_parent;
+  nfp->ctf_parent_unreffed = fp->ctf_parent_unreffed;
   nfp->ctf_refcnt = fp->ctf_refcnt;
   nfp->ctf_flags |= fp->ctf_flags & ~LCTF_DIRTY;
   if (nfp->ctf_dynbase == NULL)
@@ -565,6 +566,7 @@ ctf_serialize (ctf_file_t *fp)
   fp->ctf_syn_ext_strtab = NULL;
   fp->ctf_link_cu_mapping = NULL;
   fp->ctf_link_type_mapping = NULL;
+  fp->ctf_parent_unreffed = 1;
 
   fp->ctf_dvhash = NULL;
   memset (&fp->ctf_dvdefs, 0, sizeof (ctf_list_t));
diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h
index b9d52af..4c8a37c 100644
--- a/libctf/ctf-impl.h
+++ b/libctf/ctf-impl.h
@@ -291,6 +291,7 @@ struct ctf_file
   const char *ctf_cuname;	  /* Compilation unit name (if any).  */
   char *ctf_dyncuname;		  /* Dynamically allocated name of CU.  */
   struct ctf_file *ctf_parent;	  /* Parent CTF container (if any).  */
+  int ctf_parent_unreffed;	  /* Parent set by ctf_import_unref?  */
   const char *ctf_parlabel;	  /* Label in parent container (if any).  */
   const char *ctf_parname;	  /* Basename of parent (if any).  */
   char *ctf_dynparname;		  /* Dynamically allocated name of parent.  */
@@ -536,6 +537,7 @@ extern ctf_file_t *ctf_simple_open_internal (const char *, size_t, const char *,
 extern ctf_file_t *ctf_bufopen_internal (const ctf_sect_t *, const ctf_sect_t *,
 					 const ctf_sect_t *, ctf_dynhash_t *,
 					 int, int *);
+extern int ctf_import_unref (ctf_file_t *fp, ctf_file_t *pfp);
 extern int ctf_serialize (ctf_file_t *);
 
 _libctf_malloc_
diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
index c331fde..bcfd256 100644
--- a/libctf/ctf-link.c
+++ b/libctf/ctf-link.c
@@ -222,7 +222,7 @@ ctf_create_per_cu (ctf_file_t *fp, const char *filename, const char *cuname)
       if (ctf_dynhash_insert (fp->ctf_link_outputs, dynname, cu_fp) < 0)
 	goto oom;
 
-      ctf_import (cu_fp, fp);
+      ctf_import_unref (cu_fp, fp);
       ctf_cuname_set (cu_fp, cuname);
       ctf_parent_name_set (cu_fp, _CTF_SECTION);
     }
diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c
index 24899f0..87bff2f 100644
--- a/libctf/ctf-open.c
+++ b/libctf/ctf-open.c
@@ -1657,9 +1657,17 @@ ctf_file_close (ctf_file_t *fp)
       return;
     }
 
+  /* It is possible to recurse back in here, notably if dicts in the
+     ctf_link_inputs or ctf_link_outputs cite this dict as a parent without
+     using ctf_import_unref.  Do nothing in that case.  */
+  if (fp->ctf_refcnt == 0)
+    return;
+
+  fp->ctf_refcnt--;
   free (fp->ctf_dyncuname);
   free (fp->ctf_dynparname);
-  ctf_file_close (fp->ctf_parent);
+  if (fp->ctf_parent && !fp->ctf_parent_unreffed)
+    ctf_file_close (fp->ctf_parent);
 
   for (dtd = ctf_list_next (&fp->ctf_dtdefs); dtd != NULL; dtd = ntd)
     {
@@ -1816,13 +1824,44 @@ ctf_import (ctf_file_t *fp, ctf_file_t *pfp)
   if (pfp != NULL && pfp->ctf_dmodel != fp->ctf_dmodel)
     return (ctf_set_errno (fp, ECTF_DMODEL));
 
-  if (fp->ctf_parent != NULL)
+  if (fp->ctf_parent && !fp->ctf_parent_unreffed)
+    ctf_file_close (fp->ctf_parent);
+  fp->ctf_parent = NULL;
+
+  if (pfp != NULL)
     {
-      fp->ctf_parent->ctf_refcnt--;
-      ctf_file_close (fp->ctf_parent);
-      fp->ctf_parent = NULL;
+      int err;
+
+      if (fp->ctf_parname == NULL)
+	if ((err = ctf_parent_name_set (fp, "PARENT")) < 0)
+	  return err;
+
+      fp->ctf_flags |= LCTF_CHILD;
+      pfp->ctf_refcnt++;
+      fp->ctf_parent_unreffed = 0;
     }
 
+  fp->ctf_parent = pfp;
+  return 0;
+}
+
+/* Like ctf_import, but does not increment the refcount on the imported parent
+   or close it at any point: as a result it can go away at any time and the
+   caller must do all freeing itself.  Used internally to avoid refcount
+   loops.  */
+int
+ctf_import_unref (ctf_file_t *fp, ctf_file_t *pfp)
+{
+  if (fp == NULL || fp == pfp || (pfp != NULL && pfp->ctf_refcnt == 0))
+    return (ctf_set_errno (fp, EINVAL));
+
+  if (pfp != NULL && pfp->ctf_dmodel != fp->ctf_dmodel)
+    return (ctf_set_errno (fp, ECTF_DMODEL));
+
+  if (fp->ctf_parent && !fp->ctf_parent_unreffed)
+    ctf_file_close (fp->ctf_parent);
+  fp->ctf_parent = NULL;
+
   if (pfp != NULL)
     {
       int err;
@@ -1832,7 +1871,7 @@ ctf_import (ctf_file_t *fp, ctf_file_t *pfp)
 	  return err;
 
       fp->ctf_flags |= LCTF_CHILD;
-      pfp->ctf_refcnt++;
+      fp->ctf_parent_unreffed = 1;
     }
 
   fp->ctf_parent = pfp;