[DTrace-devel] [PATCH 6/8] btf: fix memory leaks

[Kris Van Hees]

On Thu, Apr 04, 2024 at 08:58:36PM +0100, Nick Alcock wrote:
> On 3 Apr 2024, Kris Van Hees via DTrace-devel said:
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> A bit more to do :(
> 
> > ---
> >  libdtrace/dt_btf.c    | 21 ++++++++++++++++-----
> >  libdtrace/dt_btf.h    |  1 +
> >  libdtrace/dt_module.c |  2 +-
> >  libdtrace/dt_open.c   |  3 +++
> >  4 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
> > index 3f713632..020c7877 100644
> > --- a/libdtrace/dt_btf.c
> > +++ b/libdtrace/dt_btf.c
> > @@ -240,7 +240,7 @@ dt_btf_load(dtrace_hdl_t *dtp, const char *fn)
> >  
> >  	fp = fopen(fn, "rb");
> >  	if (fp == NULL)
> > -		return dt_btf_set_load_errno(dtp,  errno);
> > +		goto err;
> >  
> >  	/* First see whether this might be a file with raw BTF data. */
> >  	if (fread(&hdr, 1, sizeof(hdr), fp) < sizeof(hdr))
> > @@ -317,12 +317,12 @@ elf_fail_no_end:
> >  	dt_btf_error(dtp, 0, "BTF: %s", elf_errmsg(err));
> >  	fclose(fp);
> >  
> > -	return NULL;
> > -
> >  fail:
> >  	dt_free(dtp, data);
> > +
> > +err:
> > +	dt_free(dtp, btf);
> 
> This should be a dt_btf_destroy(), since you might have allocated
> btf->data by the time you enter elf_fail.

In that case the dt_free(dtp, data) got executed already.

> There's another leak above:
> 
> 		data = dt_alloc(dtp, st.st_size);
> 		if (data == NULL)
> 			goto fail;
> 
> 		if (fread(data, 1, st.st_size, fp) < st.st_size)
> 			goto fail;
> 
> That second fail has allocated 'data' but not yet assigned it to
> btf->data, so even if you *do* use dt_btf_destroy() where I flagged, it
> wouldn't free it...

Again, that is why we have the dt_free(dtp, data);

> >  	dt_btf_set_errno(dtp,  err ? err : errno);
> > -	fclose(fp);
> 
> ... this looks like you've turned a leak of btf into an fd leak of fp by
> everything going down the fail path (all of which are entered before the
> fclose() of fp on the non-fail paths above).

Ah yes, I forgot to move the fclose(fp) to the fail: label.

> In summary, error paths are evil!
> 
> 
> 
> > @@ -752,7 +763,7 @@ dt_btf_load_module(dtrace_hdl_t *dtp, dt_module_t *dmp)
> >  	snprintf(fn, sizeof(fn), "/sys/kernel/btf/%s", dmp->dm_name);
> >  	btf = dt_btf_load_file(dtp, fn);
> >  
> > -	if (btf && strcmp(dmp->dm_name, "vmlinux") == 0)
> > +	if (btf && !dtp->dt_shared_btf && strcmp(dmp->dm_name, "vmlinux") == 0)
> >  		dtp->dt_shared_btf = btf;
> 
> Is there some danger that dt_btf_load_module() wil be called on vmlinux
> more than once? That's the only situation in which this change would
> constitute a leak fix.
> 
> > +	if (dtp->dt_shared_btf != NULL)
> > +		dt_btf_destroy(dtp, dtp->dt_shared_btf);
> 
> Unnecessary != NULL check. (It's unnecessary in the CTF case below, too.)
> 
> -- 
> NULL && (void)