[DTrace-devel] [PATCH] Fix size of string data in the trace output buffer

[Eugene Loh]

On 9/8/21 2:30 AM, Kris Van Hees wrote:

> On Wed, Sep 08, 2021 at 12:49:12AM -0400, Eugene Loh wrote:
>> Also, does dt_cg_store_var() have the same issues?
> No, this is only really needed for store_val because of how the consumer
> handles strings.

Let's put it this way:  dt_cg_store_var() has issues.  Perhaps they are 
not the "same" issues since they have nothing to do with the consumer, 
but the fix (replacing a straight copy with a custom prefix+data+NULL 
construction) would be the same.  E.g., you have a test case:
     #pragma D option strsize=5
     BEGIN
     {
         trace(probeprov);
     }
If we modified that to be:
     #pragma D option strsize=5
     BEGIN
     {
         p = "dtrace";
         trace(p);
     }
the BPF verifier would erupt.  I think that's because in 
dt_cg_store_var(), we assume we'll have enough space to copy over the 
string.  We probably need, instead, well, basically the same fix you 
give in this patch.  If you like, I can propose a patch.  Or, we can 
wait... especially if the length prefix is going away anyhow.

>
>> On 9/7/21 8:37 PM, Kris Van Hees wrote:
>>>    		dt_regset_xalloc(drp, BPF_REG_0);
>>>    		emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_probe_read));
>>>    		dt_regset_free_args(drp);
>>> -		dt_regset_free(drp, BPF_REG_0);
>>> +
>>> +		/*
>>> +		 * Write the NUL terminating byte in case we are truncating.
>>> +		 */
>>> +		emit(dlp,  BPF_MOV_REG(BPF_REG_0, BPF_REG_9));
>>> +		emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_0, reg));
>>> +		emit(dlp,  BPF_STORE_IMM(BPF_B, BPF_REG_0, off + DT_STRLEN_BYTES, 0));
>> I sure do not understand the register management here.  Earlier, you use
>> %r0 for writing the length prefix, but never xalloc'ing the register.
>> Here, you xalloc(r0) but you removed the free(r0).  Isn't that wrong?
>> But I tried iterating on this and it appears someone else is freeing the
>> register?  Who?  I'm confused.
> Ah, I forgot to paste the freeing of %r0 apparently - good catch.

Thanks, but this sort of thing should be caught automatically.  I think 
what was happening was that since we were not checking the return from 
dt_regset_xalloc(%r0), we just reused and respilled the same register 
over and over again.  So, we should probably:

1)  Check the return value of regset alloc() (here and wherever else 
that check has been omitted).

2)  Or perhaps just push the longjmp(NOREG) down into dt_regset.c, both 
making sure the error is caught and reducing the boilerplate code of 
checking the return.