[PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 12.02.2025 07:58, Indu Bhagat wrote:
> On 2/10/25 1:17 AM, Jan Beulich wrote:
>> On 10.02.2025 07:06, Indu Bhagat wrote:
>>> On 2/7/25 3:22 AM, Jan Beulich wrote:
>>>> On 05.02.2025 00:10, Indu Bhagat wrote:
>>>>> +  /* Check roughly for expression of the kind
>>>>> +     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX) */
>>>>> +#define CFI_ESC_NUM_EXP 4
>>>>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>>>>> +  while (e->next)
>>>>> +    {
>>>>> +      e = e->next;
>>>>> +      if (i >= CFI_ESC_NUM_EXP)
>>>>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>>> +      items[i] = e->exp.X_add_number;
>>>>> +      i++;
>>>>> +    }
>>>>> +
>>>>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>>>>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>>
>>>> See the respective comment on sframe_xlate_do_escape_val_offset() below.
>>>>
>>>>> +  cur_fre = xlate_ctx->cur_fre;
>>>>> +  reg = items[0];
>>>>> +#undef CFI_ESC_NUM_EXP
>>>>
>>>> You fetch 4 bytes, then use only the first? Shouldn't you at least check
>>>> expression length and DW_OP_breg<N> (and hence the 2nd register number)
>>>> as well? What about DW_OP_reg<N>, DW_OP_bregx, and DW_OP_regx?
>>>>
>>>
>>> Re:check expression length - We do check for expression length; For
>>> length >= CFI_ESC_NUM_EXP or length <= CFI_ESC_NUM_EXP - 1, we return
>>> SFRAME_XLATE_ERR_NOTREPRESENTED.
>>
>> That's not what I mean though. The length is encoded in the stream of
>> bytes (items[1] iirc), and you don't check that at all. Imo you want to
>> reject anything that isn't valid in the first place, i.e. you probably
>> shouldn't even look past this encoded length if it is, say, zero.
>>
> 
> OK, makes sense.  I now have :
> 
>        if ((i == 2 && items[1] == 0) /* Zero length in DWARF expr.  */
>            || i >= CFI_ESC_NUM_EXP || e->exp.X_op != O_constant)
>          return SFRAME_XLATE_ERR_NOTREPRESENTED;
> 
> inside the while (e->next) loop.

Well, I need to see this in context, in particular with ...

>>> Once the check for target register is done, what follows after is
>>> assumed to of no consequence in terms of how it affects SFrame stack
>>> trace data.  When GAS is in these APIs working out the SFrame
>>> generation, validating .cfi_escape data is not intended.
>>
>> Yet then - why fetch the extra bytes?
> 
> OK. Fixed now as above.

... this further adjustment in place as well. Perhaps best for me to look at
the eventual v3.

Jan