[DTrace-devel] [PATCH] Fix double-free of dtp->dt_strtab

[Eugene Loh]

lgtm (with the provision that I don't know this part of the code).

On 04/08/2020 08:00 AM, Kris Van Hees wrote:
> The consolidated string table that is used to populate the strtab BPF
> map when we prepare to load and execute BPF programs was being free'd
> twice (first when the DIFOs are destroyed and second when we are closing
> the DTrace consumer handle).  The reason for this is that dtp->dt_strtab
> was being constructed anew for each DIFO, and then assigned to the
> dtdo_strtab member for the DIFO as well.  So, the last DIFO that got
> assembled would have a reference to the same string table as the DTrace
> consumer handle.
>
> When that DIFO got destroyed (dt_difo_free()), the string table would be
> free'd as well.  Then, in dt_close() we would try to free dtp->dt_strtab
> which arready got free'd.
>
> We now (correctly) generate string table data for each DIFO, and assign
> it to dtp->dt_strtab (size is stored in dtp->strlen).  This is sufficient
> because we only care about the last one that get generated (which by
> design contains all strings from all prior DIFOs).  We know that the
> string table will be free'd in dt_difo_free() so we ensure that the
> dtp->dt_strtab and dtp->dt_strlen are cleared when the DIFO referencing
> the same string table as the consumer handle is free'd, and we refrain
> from freeing dtp->dt_strtab altogether.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_as.c   | 32 ++++++++++++++++++++++++--------
>   libdtrace/dt_open.c |  2 --
>   libdtrace/dt_subr.c | 11 +++++++++++
>   3 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
> index 2ddbc886..42c8228a 100644
> --- a/libdtrace/dt_as.c
> +++ b/libdtrace/dt_as.c
> @@ -535,20 +535,36 @@ fail:
>   
>   	/*
>   	 * Allocate memory for the compiled string table and then copy the
> -	 * chunks from the string table into the final string buffer.
> +	 * chunks from the string table into the final string buffer for the
> +	 * DIFO we are constructing.
> +	 *
> +	 * We keep the string table around (dtp->dt_csstab) because further
> +	 * compilations may add more strings.  We will load a consolidated
> +	 * compiled string table into a strtab BPF map so it can be used ny
> +	 * all BPF programs we will be loading.  Since each DIFO will have a
> +	 * string table that comprises the strings of all DIFOs before it along
> +	 * with any new ones, we simply assign the latest (and most complete)
> +	 * string table to dtp->dt_strtab (and the size as dtp->dt_strlen), so
> +	 * that when we are ready to load and execute programs, we know we have
> +	 * the latest and greatest string table to work with.
> +	 *
> +	 * Note that this means that we should *not* free dtp->dt_strtab later
> +	 * because it will be free'd when the DIFO is destroyed.
>   	 */
> -	if ((n = dt_strtab_size(dtp->dt_ccstab)) != 0) {
> -		if ((dtp->dt_strtab = dt_alloc(dtp, n)) == NULL)
> +	dp->dtdo_strlen = dt_strtab_size(dtp->dt_ccstab);
> +	if (dp->dtdo_strlen > 0) {
> +		dp->dtdo_strtab = dt_zalloc(dtp, dp->dtdo_strlen);
> +		if (dp->dtdo_strtab == NULL)
>   			longjmp(pcb->pcb_jmpbuf, EDT_NOMEM);
>   
>   		dt_strtab_write(dtp->dt_ccstab,
>   				(dt_strtab_write_f *)dt_strtab_copystr,
> -				dtp->dt_strtab);
> -		dtp->dt_strlen = (uint32_t)n;
> +				dp->dtdo_strtab);
>   
> -		dp->dtdo_strtab = dtp->dt_strtab;
> -		dp->dtdo_strlen = dtp->dt_strlen;
> -	}
> +		dtp->dt_strtab = dp->dtdo_strtab;
> +		dtp->dt_strlen = dp->dtdo_strlen;
> +	} else
> +		dp->dtdo_strtab = NULL;
>   
>   	/*
>   	 * Fill in the DIFO return type from the type associated with the
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index ab60a803..36a65c88 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -1160,8 +1160,6 @@ dtrace_close(dtrace_hdl_t *dtp)
>   		dt_ident_destroy(idp);
>   	}
>   
> -	if (dtp->dt_strtab != NULL)
> -		dt_free(dtp, dtp->dt_strtab);
>   	if (dtp->dt_ccstab != NULL)
>   		dt_strtab_destroy(dtp->dt_ccstab);
>   	if (dtp->dt_macros != NULL)
> diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
> index 14824578..87db2fb4 100644
> --- a/libdtrace/dt_subr.c
> +++ b/libdtrace/dt_subr.c
> @@ -726,7 +726,18 @@ dt_difo_free(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
>   	if (dp == NULL)
>   		return; /* simplify caller code */
>   
> +	/*
> +	 * If we are destroying the DIFO that holds the final consolidated
> +	 * string table, clear the dt_strtab and dt_strlen members in the
> +	 * consumer handle.
> +	 */
> +	if (dtp->dt_strtab == dp->dtdo_strtab) {
> +		dtp->dt_strtab = NULL;
> +		dtp->dt_strlen = 0;
> +	}
> +
>   	dt_free(dtp, dp->dtdo_buf);
> +	dt_free(dtp, dp->dtdo_strtab);
>   	dt_free(dtp, dp->dtdo_vartab);
>   	dt_free(dtp, dp->dtdo_kreltab);
>   	dt_free(dtp, dp->dtdo_ureltab);