[DTrace-devel] [PATCH v2 05/14] probe: improve dt_probe_lookup2()

[Nick Alcock]

This function had numerous problems:

 - it broke _FORTIFY_SOURCE due to using a function that snprintf()ed
   with a size of INT_MAX, into a much smaller alloca()ed buffer
 - it claimed to call down to the dtrace kernel module for a probe
   description (long obsolete)
 - it invariably leaked the probe description it constructed by calling
   dtrace_xstr2desc()
 - it did meaningless errno testing to determine whether to return
   EDT_NOPROBE on error, which more or less led to EDT_NOPROBE never
   being returned even though it should always be in this case (there
   is no other cause for the ID hash lookup failing).

Fix the lot: while we're at it, implement a dt_desc_destroy() to
undo the strdup()s done by dtrace_xstr2desc(), and use it additionally
in dt_probe_destroy() instead of an identical sequence of dt_free()s.

Bug: https://github.com/oracle/dtrace-utils/issues/78
Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
 libdtrace/dt_impl.h  |  2 ++
 libdtrace/dt_probe.c | 44 +++++++++++++-------------------------------
 libdtrace/dt_subr.c  | 10 ++++++++++
 3 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 340dc1960c5e..d11ad2839f8e 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -759,6 +759,8 @@ extern dtrace_difo_t *dt_difo_copy(dtrace_hdl_t *dtp, const dtrace_difo_t *odp);
 extern int dt_consume_init(dtrace_hdl_t *);
 extern void dt_consume_fini(dtrace_hdl_t *);
 
+extern void dt_desc_destroy(dtrace_hdl_t *dtp, dtrace_probedesc_t *);
+
 extern dtrace_datadesc_t *dt_datadesc_hold(dtrace_datadesc_t *ddp);
 extern void dt_datadesc_release(dtrace_hdl_t *, dtrace_datadesc_t *);
 extern dtrace_datadesc_t *dt_datadesc_create(dtrace_hdl_t *);
diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
index 686e2a661253..f84581687caf 100644
--- a/libdtrace/dt_probe.c
+++ b/libdtrace/dt_probe.c
@@ -172,24 +172,9 @@ dt_probe_alloc_args(dt_probe_t *prp, int nargc, int xargc)
 	}
 }
 
-static size_t
-dt_probe_keylen(const dtrace_probedesc_t *pdp)
-{
-	return strlen(pdp->mod) + 1 + strlen(pdp->fun) + 1 +
-		strlen(pdp->prb) + 1;
-}
-
-static char *
-dt_probe_key(const dtrace_probedesc_t *pdp, char *s)
-{
-	snprintf(s, INT_MAX, "%s:%s:%s", pdp->mod, pdp->fun, pdp->prb);
-	return s;
-}
-
 /*
  * Lookup a probe declaration based on a known provider and full or partially
- * specified module, function, and name.  If the probe is not known to us yet,
- * ask dtrace(7D) to match the description and then cache any useful results.
+ * specified module, function, and name.
  */
 dt_probe_t *
 dt_probe_lookup2(dt_provider_t *pvp, const char *s)
@@ -197,28 +182,28 @@ dt_probe_lookup2(dt_provider_t *pvp, const char *s)
 	dtrace_hdl_t *dtp = pvp->pv_hdl;
 	dtrace_probedesc_t pd;
 	dt_ident_t *idp;
-	size_t keylen;
 	char *key;
 
 	if (dtrace_str2desc(dtp, DTRACE_PROBESPEC_NAME, s, &pd) != 0)
 		return NULL; /* dt_errno is set for us */
 
-	keylen = dt_probe_keylen(&pd);
-	key = dt_probe_key(&pd, alloca(keylen));
+	if (asprintf(&key, "%s:%s:%s", pd.mod, pd.fun, pd.prb) == -1) {
+		dt_set_errno(dtp, errno);
+		goto out;
+	}
 
 	/*
 	 * If the probe is already declared, then return the dt_probe_t from
-	 * the existing identifier.  This could come from a static declaration
-	 * or it could have been cached from an earlier call to this function.
+	 * the existing identifier.
 	 */
-	if ((idp = dt_idhash_lookup(pvp->pv_probes, key)) != NULL)
+	if ((idp = dt_idhash_lookup(pvp->pv_probes, key)) != NULL) {
+		dt_desc_destroy(dtp, &pd);
 		return idp->di_data;
+	}
 
-	if (errno == ESRCH || errno == EBADF)
-		dt_set_errno(dtp, EDT_NOPROBE);
-	else
-		dt_set_errno(dtp, errno);
-
+	dt_set_errno(dtp, EDT_NOPROBE);
+ out:
+	dt_desc_destroy(dtp, &pd);
 	return NULL;
 }
 
@@ -387,10 +372,7 @@ dt_probe_destroy(dt_probe_t *prp)
 	dt_free(dtp, prp->mapping);
 	dt_free(dtp, prp->argv);
 	if (prp->desc) {
-		dt_free(dtp, (void *)prp->desc->prv);
-		dt_free(dtp, (void *)prp->desc->mod);
-		dt_free(dtp, (void *)prp->desc->fun);
-		dt_free(dtp, (void *)prp->desc->prb);
+		dt_desc_destroy(dtp, (dtrace_probedesc_t *)prp->desc);
 		dt_free(dtp, (void *)prp->desc);
 	}
 	dt_free(dtp, prp);
diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
index d6aad7637fb9..72631b33a0ad 100644
--- a/libdtrace/dt_subr.c
+++ b/libdtrace/dt_subr.c
@@ -175,6 +175,16 @@ dtrace_desc2str(const dtrace_probedesc_t *pdp, char *buf, size_t len)
 	return buf;
 }
 
+/* Only use on probedescs derived from dtrace_xstr2desc above. */
+void
+dt_desc_destroy(dtrace_hdl_t *dtp, dtrace_probedesc_t *pdp)
+{
+	dt_free(dtp, (void *) pdp->prv);
+	dt_free(dtp, (void *) pdp->mod);
+	dt_free(dtp, (void *) pdp->fun);
+	dt_free(dtp, (void *) pdp->prb);
+}
+
 char *
 dtrace_attr2str(dtrace_attribute_t attr, char *buf, size_t len)
 {
-- 
2.46.0.278.g36e3a12567