[DTrace-devel] [PATCH] Do not allocate a 0-size strtab

[Eugene Loh]

On 04/08/2020 08:00 AM, Kris Van Hees wrote:

> If there is no stirng data to be stored in a DIFO, we were allocating
stirng -> string

FWIW, if I look at the code, I typically see dtdo_strtab dereferenced 
without any guards against the scenario dtdo_strtab==NULL.  I don't know 
the code well enough to know whether to worry.  Should I?

> a 0-size memory block.  There is no need for that - we can just assign
> NULL to the dtdo_strtab member.
>
> In the case of the dt_link_stmt() code, we also happened to take the
> dtdo_strlen value from the wrong DIFO (fdp instead of dp) causing a
> 0-size strtab to be allocated into which we then try to write actual
> data.  That caused memory corruption.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cc.c    | 14 +++++++++-----
>   libdtrace/dt_dlibs.c | 23 +++++++++++++----------
>   2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index db009461..fc678692 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -2381,11 +2381,15 @@ dt_link_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
>   	 * Write out the new string table.
>   	 */
>   	dp->dtdo_strlen = dt_strtab_size(stab);
> -	dp->dtdo_strtab = dt_zalloc(dtp, fdp->dtdo_strlen);
> -	if (dp->dtdo_strtab == NULL)
> -		goto fail;
> -	dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> -			dp->dtdo_strtab);
> +	if (dp->dtdo_strlen > 0) {
> +		dp->dtdo_strtab = dt_zalloc(dtp, dp->dtdo_strlen);
> +		if (dp->dtdo_strtab == NULL)
> +			goto fail;
> +		dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> +				dp->dtdo_strtab);
> +	} else
> +		dp->dtdo_strtab = NULL;
> +
>   	dt_strtab_destroy(stab);
>   
>   	/*
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 58e01221..2b8fe21d 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -554,7 +554,6 @@ done:
>   	for (fid = fun0; fid < symc; fid++) {
>   		dtrace_difo_t	*dp;
>   		dof_relodesc_t	*brp;
> -		size_t		slen;
>   
>   		fp = funcs[fid];
>   		dp = fp->difo;
> @@ -583,16 +582,20 @@ done:
>   			rp++;
>   		}
>   
> -		slen = dt_strtab_size(stab);
> -		dp->dtdo_strtab = dt_alloc(dtp, slen);
> -		dp->dtdo_strlen = slen;
> -		if (dp->dtdo_strtab == NULL) {
> -			fprintf(stderr, "Failed to alloc strtab\n");
> -			goto out;
> -		}
> +		dp->dtdo_strlen = dt_strtab_size(stab);
> +		if (dp->dtdo_strlen > 0) {
> +			dp->dtdo_strtab = dt_alloc(dtp, dp->dtdo_strlen);
> +			if (dp->dtdo_strtab == NULL) {
> +				fprintf(stderr, "Failed to alloc strtab\n");
> +				goto out;
> +			}
> +
> +			dt_strtab_write(stab,
> +					(dt_strtab_write_f *)dt_strtab_copystr,
> +					dp->dtdo_strtab);
> +		} else
> +			dp->dtdo_strtab = NULL;
>   
> -		dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> -				dp->dtdo_strtab);
>   		dt_strtab_destroy(stab);
>   	}
>