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

[Pedro Alves <pedro@palves.net>]

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.

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
>