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

[Eugene Loh]

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!

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