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

[Eugene Loh]

On 04/08/2020 02:30 PM, Kris Van Hees wrote:

> On Wed, Apr 08, 2020 at 01:53:58PM -0700, Eugene Loh wrote:
>> On 04/08/2020 01:40 PM, Kris Van Hees wrote:
>>> On Wed, Apr 08, 2020 at 01:35:55PM -0700, Eugene Loh wrote:
>>>> 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?
>>> If the memory allocation for dtdo_strtab failed, we bail by way of a fatal
>>> error, which means nothing ever gets to use this.  So yes, it is safe.
>> But what I mean is that if the table has 0 size, we don't even try to
>> allocate the table... we just set a NULL pointer, which is a different
>> situation from an allocation failing and indicating an error.  Again, I
>> just don't know if this means there can be a problem.  Maybe nothing to
>> worry about:  if a problem turns up, it'll make itself known!
> Ah ok.  Sorry, you confused me because you wrote "If the memory allocation for
> dtdo_strtab failed" so I focused on the allocation failure case.

???  I believe those were your words.  Anyhow, doesn't matter.

> If we have no strings, dtdo_strlen will be 0, dtdo_strtab will be NULL, and
> that is safe because when there are no strings, there is nothing in the DIFO
> that will reference dtdo_strtab.  If anything referenced a string it would be
> an entry to either the variable table or one of the relocation tables.  And
> in that case those strings would have been added, and thus dtdo_strlen would
> not be zero.

Cool.  Thanks for the explanation.

>>>>> 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);
>>>>>     	}
>>>>>     
>>>> _______________________________________________
>>>> DTrace-devel mailing list
>>>> DTrace-devel at oss.oracle.com
>>>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel