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-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?

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