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

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

On Wed, Aug 06, 2025 at 05:02:44PM +0100, Alan Maguire wrote:
> On 06/08/2025 14:58, Kris Van Hees wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
> 
> 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?

Tne hashtable is stored as part of the USDT provider, and used by stapsdt
provider probes as well.  The copy_args() function explicitly performs a
lookup for the USDT provider to get access to its private data (the htab).