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

[Nick Alcock]

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.

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...

>  	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).

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)