Re: [PATCH v3 05/14] probe: improve dt_probe_lookup2()

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

On Wed, Oct 30, 2024 at 12:12:27PM +0000, Nick Alcock 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>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... modulo copyright year in dt_subr.c (updated in my merge)

> ---
>  libdtrace/dt_impl.h  |  2 ++
>  libdtrace/dt_probe.c | 47 ++++++++++++--------------------------------
>  libdtrace/dt_subr.c  | 15 ++++++++++++++
>  3 files changed, 30 insertions(+), 34 deletions(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 340dc1960c5e..5d34072042e5 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 *, int free_pdp);
> +
>  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..189bf7928e14 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, 0);
>  		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, 0);
>  	return NULL;
>  }
>  
> @@ -386,13 +371,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_free(dtp, (void *)prp->desc);
> -	}
> +	dt_desc_destroy(dtp, (dtrace_probedesc_t *)prp->desc, 1);
>  	dt_free(dtp, prp);
>  }
>  
> diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
> index d6aad7637fb9..afcdb733d26c 100644
> --- a/libdtrace/dt_subr.c
> +++ b/libdtrace/dt_subr.c
> @@ -175,6 +175,21 @@ dtrace_desc2str(const dtrace_probedesc_t *pdp, char *buf, size_t len)
>  	return buf;
>  }
>  
> +void
> +dt_desc_destroy(dtrace_hdl_t *dtp, dtrace_probedesc_t *pdp, int free_pdp)
> +{
> +	if (pdp == NULL)
> +		return;
> +
> +        dt_free(dtp, (void *) pdp->prv);
> +	dt_free(dtp, (void *) pdp->mod);
> +	dt_free(dtp, (void *) pdp->fun);
> +	dt_free(dtp, (void *) pdp->prb);
> +
> +        if (free_pdp)
> +		dt_free(dtp, pdp);
> +}
> +
>  char *
>  dtrace_attr2str(dtrace_attribute_t attr, char *buf, size_t len)
>  {
> -- 
> 2.46.0.278.g36e3a12567
>