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.

diff --git a/libctf/ChangeLog b/libctf/ChangeLog
index f756fc4eeb71a9235b174a703e42406b09946059..4830284b14a1477a52dfbcf351873615631f8f6f 100644 (file)
--- a/libctf/ChangeLog
+++ b/libctf/ChangeLog
@@ -1,3 +1,19 @@
+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...

diff --git a/libctf/ctf-create.c b/libctf/ctf-create.c
index a538b2d560347ae0ff4f4b437741c89a1dfca25d..a964c20b9ed223742b81d943b7d0f265f06c69a2 100644 (file)
--- 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 b9d52af9d0e5c1cab5ff6777092a0e4634bad9d2..4c8a37c4c268e20741e70e8963d54fcada904b3d 100644 (file)
--- 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 c331fde35dc0492a78163a86c5c6d4142b31eed1..bcfd2564fbc23921c8484827a713b64cc30342de 100644 (file)
--- 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 24899f08e20fea36a91d5762fb6a82e0993916f7..87bff2f122d31e61641798029cc1698a5fab2fcc 100644 (file)
--- 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;