From 1fa7a0c24e78e7f7995a3fa608c9f97a6ccbe1b6 Mon Sep 17 00:00:00 2001 From: Nick Alcock Date: Thu, 4 Jun 2020 17:30:01 +0100 Subject: [PATCH] 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) : 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. --- libctf/ChangeLog | 16 ++++++++++++++ libctf/ctf-create.c | 4 +++- libctf/ctf-impl.h | 2 ++ libctf/ctf-link.c | 2 +- libctf/ctf-open.c | 51 +++++++++++++++++++++++++++++++++++++++------ 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/libctf/ChangeLog b/libctf/ChangeLog index f756fc4eeb7..4830284b14a 100644 --- a/libctf/ChangeLog +++ b/libctf/ChangeLog @@ -1,3 +1,19 @@ +2020-07-22 Nick Alcock + + * ctf-impl.c (ctf_file_t) : 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 * ctf-impl.h (ctf_link_type_mapping_key): Rename to... diff --git a/libctf/ctf-create.c b/libctf/ctf-create.c index a538b2d5603..a964c20b9ed 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 b9d52af9d0e..4c8a37c4c26 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 c331fde35dc..bcfd2564fbc 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 24899f08e20..87bff2f122d 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; -- 2.30.2