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

[Kris Van Hees <kris.van.hees@oracle.com>]

On Mon, Oct 28, 2024 at 09:17:54PM +0000, Nick Alcock via DTrace-devel wrote:
> 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@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 *);

I think it would be better to let dt_desc_destroy() take a third arg, to
indicate whether the desc itself should be free'd as well or whether it
should be left alone.

That avoids calling it and then following it up with a dt_free() on the desc.

> +
>  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. */

Why can't it used on other dtrace_probedesc_t that contain references to
memory that needs to be free'd?  I don't think that dtrace_xstr2desc() does
anything to make the desc it populates be special.

> +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
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel