Re: [PATCH] Fix various memory leaks related to stapsdt and usdt probes

On 06/08/2025 14:58, Kris Van Hees wrote:
> Signed-off-by: Kris Van Hees <[email protected]>

Reviewed-by: Alan Maguire <[email protected]>
Tested-by: Alan Maguire <[email protected]>

All stapsdt, expected USDT tests pass for me. One question at the end..

> ---
>  libdtrace/dt_pid.c         | 14 ++++++++++----
>  libdtrace/dt_prov_uprobe.c | 13 +++++++++++++
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 42f667fe1..4af9141aa 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -1282,6 +1282,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
>  	int i, err = 0;
>  	int fd = -1;
>  	char *mod;
> +	char *no_fun = "";
>  
>  	fd = open(path, O_RDONLY);
>  	if (fd < 0) {
> @@ -1415,7 +1416,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
>  				       &fun, &sym) == 0)
>  			psp.pps_fun = (char *)fun;
>  		else
> -			psp.pps_fun = "";
> +			psp.pps_fun = no_fun;
>  		psp.pps_dev = pmp->pr_dev;
>  		psp.pps_inum = pmp->pr_inum;
>  		psp.pps_pid = dpr->dpr_pid;
> @@ -1430,6 +1431,9 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
>  		}
>  		if (err == -1)
>  			break;
> +
> +		if (psp.pps_fun != no_fun)
> +			free(psp.pps_fun);
>  	}
>  
>  out:
> @@ -1513,7 +1517,6 @@ dt_pid_create_stapsdt_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_
>  	const dt_provider_t *pvp;
>  	dt_proc_t *dpr = NULL;
>  	const char *pidstr;
> -	char *path = NULL;
>  	pid_t pid;
>  
>  	assert(pcb != NULL);
> @@ -1525,8 +1528,6 @@ dt_pid_create_stapsdt_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_
>  	if (strlen(pidstr) == 0)
>  		return 0;
>  
> -	asprintf(&path, "/proc/%s/maps", pidstr);
> -
>  	pvp = dt_provider_lookup(dtp, "stapsdt");
>  	assert(pvp != NULL);
>  
> @@ -1542,8 +1543,13 @@ dt_pid_create_stapsdt_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_
>  	}
>  	dpr = dt_proc_lookup(dtp, pid);
>  	if (dpr) {
> +		char *path = NULL;
> +
> +		if (asprintf(&path, "/proc/%s/maps", pidstr) == -1)
> +			longjmp(pcb->pcb_jmpbuf, EDT_NOMEM);
>  		dt_pid_create_stapsdt_probes_proc(pdp, dtp, pcb,
>  						  pvp, dpr, path);
> +		free(path);
>  		dt_proc_release_unlock(dtp, pid);
>  	}
>  
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 8c8dcae0c..986ac0ead 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -795,6 +795,10 @@ static int populate_args(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  	char	*nptr = NULL, *xptr = NULL;
>  	size_t	i;
>  
> +	/* Nothing to do if we already populated the arguments. */
> +	if (upp->argc >= 0)
> +		return 0;
> +
>  	upp->argc = psp->pps_xargc;
>  
>  	/* Copy argument value source string data (if any). */
> @@ -941,6 +945,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  		upp->refcntr_off = psp->pps_refcntr_off;
>  		upp->fn = strdup(psp->pps_fn);
>  		upp->func = NULL;
> +		upp->argc = -1;			/* no argument data yet */
>  		upp->tp = dt_tp_alloc(dtp);
>  		if (upp->tp == NULL)
>  			goto fail;
> @@ -1716,6 +1721,7 @@ static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
>  	rc = dprintf(fd, "%c:%s %s\n", flags & PP_IS_RETURN ? 'r' : 'p', name, spec);
>  
>  out:
> +	free(spec);
>  	if (fd != -1)
>  		close(fd);
>  	if (rc < 0) {
> @@ -1975,6 +1981,12 @@ static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
>  	uprobe_delete(upp->dev, upp->inum, upp->off, upp->flags);
>  }
>  vi l
> +/* Clean up the private provider data. */
> +static void destroy(dtrace_hdl_t *dtp, void *arg)
> +{
> +	dt_htab_destroy((dt_htab_t *)arg);
> +}
> +
>  /*
>   * Used for underlying probes (uprobes).
>   */
> @@ -2014,6 +2026,7 @@ dt_provimpl_t	dt_usdt = {
>  	.probe_destroy	= &probe_destroy,
>  	.discover	= &discover,
>  	.add_probe	= &add_probe_usdt,
> +	.destroy	= &destroy,
>  };
>  

Do we need destroy for dt_stapsdt too, since the htab is used by the
trampoline via copy_args() there too?