[DTrace-devel] [PATCH] usdt parser: fix memory leak in provider and probe htabs

[Kris Van Hees]

The prvmap and prbmap hashtable implementation was lacking code to
ensure that entries are freed when the hashtables are destroyed.  In
addition, the pvp->pmap hashtables were also not cleaned up.

To provide cleaner data structures and to facilitate cleanup, tracepoint
probes are now always removed from prbmap and either added to the
pvp->pmap hashtable or added to the list of tracepoints for a particular
probe.  Therefore, when the prbmap is destroyed it no longer contains
any entries.

So, only pvp->pmap hashtables can contain probes.  If they are function
specific, they represent a tracepoint (and may have additional
tracepoints linked to them).  If they are not function specific, they
provide a probe definition and they will contain probe argument data
that needs freeing.

Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
---
 libcommon/usdt_parser_notes.c | 82 +++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 17 deletions(-)

diff --git a/libcommon/usdt_parser_notes.c b/libcommon/usdt_parser_notes.c
index cdadf4b2..c6fc3476 100644
--- a/libcommon/usdt_parser_notes.c
+++ b/libcommon/usdt_parser_notes.c
@@ -161,7 +161,25 @@ static int prv_cmp(const dt_provider_t *p, const dt_provider_t *q) {
 }
 
 DEFINE_HE_STD_LINK_FUNCS(prv, dt_provider_t, he)
-DEFINE_HTAB_STD_OPS(prv)
+
+static void *
+prv_del_prov(dt_provider_t *head, dt_provider_t *pvp)
+{
+	head = prv_del(head, pvp);
+
+	dt_htab_destroy(pvp->pmap);
+	free(pvp);
+
+	return head;
+}
+
+static dt_htab_ops_t prv_htab_ops = {
+        .hval = (htab_hval_fn)prv_hval,
+        .cmp = (htab_cmp_fn)prv_cmp,
+        .add = (htab_add_fn)prv_add,
+        .del = (htab_del_fn)prv_del_prov,
+        .next = (htab_next_fn)prv_next
+};
 
 static uint32_t prb_hval(const dt_probe_t *prp) {
 	uint32_t	hval;
@@ -205,6 +223,41 @@ static int prb_cmp(const dt_probe_t *p, const dt_probe_t *q) {
 DEFINE_HE_STD_LINK_FUNCS(prb, dt_probe_t, he)
 DEFINE_HTAB_STD_OPS(prb)
 
+static void *
+prb_del_probe(dt_probe_t *head, dt_probe_t *prp)
+{
+	head = prb_del(head, prp);
+
+	/*
+	 * If this is not a function-specific probe (from a prov note), free
+	 * the translated arg data and the probe itself.
+	 * If this is a function-specific probe (from a usdt note), walk the
+	 * list of tracepoint probe, freeing each probe in the list.
+	 */
+	if (prp->fun == NULL) {
+		free(prp->xargs);
+		free(prp->xmap);
+		free(prp);
+	} else {
+		dt_probe_t	*nxt;
+
+		do {
+			nxt = prp->next;
+			free(prp);
+		} while ((prp = nxt) != NULL);
+	}
+
+	return head;
+}
+
+static dt_htab_ops_t pmap_htab_ops = {
+        .hval = (htab_hval_fn)prb_hval,
+        .cmp = (htab_cmp_fn)prb_cmp,
+        .add = (htab_add_fn)prb_add,
+        .del = (htab_del_fn)prb_del_probe,
+        .next = (htab_next_fn)prb_next
+};
+
 /*
  * Return the cummulative string length of 'cnt' consecutive 0-terminated
  * strings.  If skip > 0, it indicates how many extra bytes are to be skipped
@@ -252,7 +305,7 @@ parse_prov_note(int out, dof_helper_t *dhp, usdt_data_t *data,
 		memset(pvp, 0, sizeof(dt_provider_t));
 		pvp->name = prvt.name;
 		dt_htab_insert(prvmap, pvp);
-		pvp->pmap = dt_htab_create(&prb_htab_ops);
+		pvp->pmap = dt_htab_create(&pmap_htab_ops);
 	} else {
 		usdt_error(out, EEXIST, "Duplicate provider: %s", prvt.name);
 		return -1;
@@ -715,30 +768,24 @@ usdt_parse_notes(int out, dof_helper_t *dhp, usdt_data_t *data)
 		 * tracepoint probe to the provider.
 		 * In either cases, argument data is copied.
 		 */
+		dt_htab_delete(prbmap, ptp);
 		if (prp->fun != NULL) {
 			ptp->next = prp->next;
-			ptp->nargc = prp->nargc;
-			ptp->nargs = prp->nargs;
-			ptp->nargsz = prp->nargsz;
-			ptp->xargc = prp->xargc;
-			ptp->xargs = prp->xargs;
-			ptp->xargsz = prp->xargsz;
-			ptp->xmap = prp->xmap;
 			prp->next = ptp;
 			prp->ntp++;
 		} else {
-			dt_htab_delete(prbmap, ptp);
 			dt_htab_insert(pvp->pmap, ptp);
 			ptp->ntp = 1;
-			ptp->nargc = prp->nargc;
-			ptp->nargs = prp->nargs;
-			ptp->nargsz = prp->nargsz;
-			ptp->xargc = prp->xargc;
-			ptp->xargs = prp->xargs;
-			ptp->xargsz = prp->xargsz;
-			ptp->xmap = prp->xmap;
 			pvp->probec++;
 		}
+
+		ptp->nargc = prp->nargc;
+		ptp->nargs = prp->nargs;
+		ptp->nargsz = prp->nargsz;
+		ptp->xargc = prp->xargc;
+		ptp->xargs = prp->xargs;
+		ptp->xargsz = prp->xargsz;
+		ptp->xmap = prp->xmap;
 	}
 
 	/* Emit any provider that has tracepoints. */
@@ -754,6 +801,7 @@ err:
 	rc = -1;
 
 out:
+	assert(dt_htab_entries(prbmap) == 0);
 	dt_htab_destroy(prvmap);
 	dt_htab_destroy(prbmap);
 
-- 
2.43.5