Re: [PATCHv5 03/11] gdbserver: allow agent expressions to fail with invalid memory access

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2023-07-07 17:28, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>> 
>>> On 2023-03-16 17:36, Andrew Burgess via Gdb-patches wrote:
>>>> This commit extends gdbserver to take account of a failed memory
>>>> access from agent_mem_read, and to return a new eval_result_type
>>>> expr_eval_invalid_memory_access.
>>>>
>>>> I have only updated the agent_mem_read calls related directly to
>>>> reading memory, I have not updated any of the calls related to
>>>> tracepoint data collection.  This is just because I'm not familiar
>>>> with that area of gdb/gdbserver, and I don't want to break anything,
>>>> so leaving the existing behaviour untouched seems like the safest
>>>> approach.
>>>
>>> I think this should update the gdbserver/tracepoint.cc:eval_result_names array at least,
>>> though.  Otherwise I think a memory error during collection is going to trigger undefined
>>> behavior, crash if we're lucky.
>> 
>> Good spot.
>> 
>> What are your thoughts on the patch below that should address this
>> issue?
>
> Hmm.  If we're changing how the array is built completely, you could go a step
> further and convert to a .def file (like e.g., gdb/unwind_stop_reasons.def) that is
> included by both ax.h and tracepoint.cc.  That would keep the enum names and their
> string representations in the same place.  It would also dispense with the
> diagnostics pragmas, as the problem scenario goes away by design.

Will do this next week and repost.

Thanks,
Andrew

>
> Pedro Alves
>
>> 
>> Thanks,
>> Andrew
>> 
>> ---
>> 
>> commit 35e78a7699618f0bb1c111cdc54d9f9bcb0171c9
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Fri Jul 7 17:18:46 2023 +0100
>> 
>>     gdbserver: handle all eval_result_type values in tracepoint.cc
>>     
>>     It was pointed out[1] that after this commit:
>>     
>>       commit 3812b38d8de5804ad3eadd6c7a5d532402ddabab
>>       Date:   Thu Oct 20 11:14:33 2022 +0100
>>     
>>           gdbserver: allow agent expressions to fail with invalid memory access
>>     
>>     Now that agent expressions might fail with the error
>>     expr_eval_invalid_memory_access, we might overflow the
>>     eval_result_names array in tracepoint.cc.  This is because the
>>     eval_result_names array does not include a string for either
>>     expr_eval_invalid_goto or expr_eval_invalid_memory_access.
>>     
>>     I don't know if having expr_eval_invalid_goto missing is also a
>>     problem, but it feels like eval_result_names should just include a
>>     string for every possible error.
>>     
>>     I could just add two more strings into the array, but I figured that,
>>     if I convert eval_result_names into a function containing a switch
>>     then I can make use of DIAGNOSTIC_ERROR_SWITCH to have the compiler
>>     ensure that we always cover every possible error code.
>>     
>>     I know (from the comments) that tracepoint.cc is built both into
>>     gdbserver, and into a shared library that the inferior might load (for
>>     fast tracepoint support), so I did consider whether this change could
>>     break the API of that shared library.  However, eval_result_names is a
>>     static array, so I don't believe that this is part of the external
>>     API, as such, converting this array into a static function shouldn't
>>     impact any users.
>>     
>>     [1] https://inbox.sourceware.org/gdb-patches/01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net/
>> 
>> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
>> index 1f31e0cc05f..879ad734b66 100644
>> --- a/gdbserver/tracepoint.cc
>> +++ b/gdbserver/tracepoint.cc
>> @@ -857,17 +857,43 @@ EXTERN_C_POP
>>  
>>  static struct tracepoint *last_tracepoint;
>>  
>> -static const char * const eval_result_names[] =
>> -  {
>> -    "terror:in the attic",  /* this should never be reported */
>> -    "terror:empty expression",
>> -    "terror:empty stack",
>> -    "terror:stack overflow",
>> -    "terror:stack underflow",
>> -    "terror:unhandled opcode",
>> -    "terror:unrecognized opcode",
>> -    "terror:divide by zero"
>> -  };
>> +/* Return a string representing the error in ERR.  This should not be
>> +   called when ERR is expr_eval_no_error.  */
>> +
>> +static const char * const
>> +eval_result_error_name (enum eval_result_type err)
>> +{
>> +  gdb_assert (err != expr_eval_no_error);
>> +
>> +DIAGNOSTIC_PUSH
>> +DIAGNOSTIC_ERROR_SWITCH
>> +  switch (err)
>> +    {
>> +    case expr_eval_no_error:
>> +      break;
>> +    case expr_eval_empty_expression:
>> +      return "terror:empty expression";
>> +    case expr_eval_empty_stack:
>> +      return "terror:empty stack";
>> +    case expr_eval_stack_overflow:
>> +      return "terror:stack overflow";
>> +    case expr_eval_stack_underflow:
>> +      return "terror:stack underflow";
>> +    case expr_eval_unhandled_opcode:
>> +      return "terror:unhandled opcode";
>> +    case expr_eval_unrecognized_opcode:
>> +      return "terror:unrecognized opcode";
>> +    case expr_eval_divide_by_zero:
>> +      return "terror:divide by zero";
>> +    case expr_eval_invalid_goto:
>> +      return "terror:invalid goto";
>> +    case expr_eval_invalid_memory_access:
>> +      return "terror:invalid memory access";
>> +    }
>> +DIAGNOSTIC_POP
>> +
>> +  gdb_assert_not_reached ("invalid error type");
>> +}
>>  
>>  #endif
>>  
>> @@ -3447,7 +3473,8 @@ stop_tracing (void)
>>    else if (expr_eval_result != expr_eval_no_error)
>>      {
>>        trace_debug ("Stopping the trace because of an expression eval error");
>> -      tracing_stop_reason = eval_result_names[expr_eval_result];
>> +      tracing_stop_reason
>> +	= eval_result_error_name ((enum eval_result_type) expr_eval_result);
>>        tracing_stop_tpnum = error_tracepoint->number;
>>      }
>>  #ifndef IN_PROCESS_AGENT
>>