[DTrace-devel] [PATCH 2/2] Fix data size for value copy in dt_cg_store_var()

[Eugene Loh]

On 9/9/21 1:29 AM, Eugene Loh wrote:

> On 9/8/21 5:07 PM, Kris Van Hees wrote:
>> The size of the data to be copied in dt_cg_store_var() was determined
>> wrongly.  For strings constants we were copying the size of the string
>> constant even if that might be larger than the maximum string size.
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -2245,29 +2246,28 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t 
>> *dlp, dt_regset_t *drp,
>>                 emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
>>                 /*
>>                * Determine the amount of data to be copied.  It is
>> -             * usually the size of the identifier, except for
>> -             * string constants where it is the size of the string
>> -             * constant (adjusted for the variable-width length
>> -             * prefix).  An assignment of a string constant is a
>> -             * store of type 'string' with a RHS of type
>> -             * DT_TOK_STRING.
>> +             * the lesser of the size of the identifier and the
>> +             * size of the data being copied in.
>>                */
>> -            if (dt_node_is_string(src) &&
>> -                src->dn_right->dn_op == DT_TOK_STRING)
>> -                size = dt_node_type_size(src->dn_right) +
>> -                       DT_STRLEN_BYTES;
>> -            else
>> -                size = idp->di_size;
>> +            srcsz = dt_node_type_size(src->dn_right);
>> +            if (dt_node_is_string(src))
>> +                srcsz += DT_STRLEN_BYTES;
>> +            dstsz = idp->di_size;
>> +            if (idp->di_ctfp == DT_STR_CTFP(dtp) &&
>> +                idp->di_type == DT_STR_TYPE(dtp))
>> +                dstsz += DT_STRLEN_BYTES;
>> +            size = MIN(srcsz, dstsz);
>>                 dt_cg_memcpy(dlp, drp, reg, src->dn_reg, size);
>
> I'm not convinced I understand this stuff.  It seems to me:
>
> *)  The offsets in the gvar map for successive strings are STRSIZE 
> apart...  not STRSIZE+DT_STRLEN_BYTES apart.  E.g., examine those 
> idp->offset values.
>
> *)  We copy in stuff that can be STRSIZE+DT_STRLEN_BYTES big. Wouldn't 
> that overwrite another string or even outside the map?

Well, overwrite another gvar, whether or not it's a string.  E.g.,

#pragma D option strsize=8
BEGIN
{
     a = "ab";
     b = 0xdeadbeeffeedface;
     a = "abcdefghij";
     printf("%x\n", b);
     exit(0);
}

producing

CPU     ID                    FUNCTION:NAME
   2      1                           :BEGIN deadbeeffeed6867

The string has overrun its storage by DT_STRLEN_BYTES (four hex digits).

> *)  If we copy from the string table (which also has length prefixes 
> on strings), we copy the length prefix over without editing it, even 
> if we clipped the string.  This means that the string length prefixes 
> in gvar won't be right.
>
> E.g., the BPF verifier seems to complain about:
>
> #pragma D option strsize=5
> BEGIN
>     {
>         x = "abcd";
>         y = "abcd";
>         z = "abcd";
>     }
>
> So far as I can tell, this stuff is buggy.  The best thing to do, if 
> the string length prefixes are going to disappear, is to eradicate 
> them first and then clean this stuff up.