[binutils-gdb] gdb/python: avoid throwing an exception over libopcodes code

[binutils-gdb] gdb/python: avoid throwing an exception over libopcodes code

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=65639fcc54226c0621d4312efac702c92ddde324

commit 65639fcc54226c0621d4312efac702c92ddde324
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Oct 21 16:20:58 2022 +0100

    gdb/python: avoid throwing an exception over libopcodes code
    
    Bug gdb/29712 identifies a problem with the Python disassembler API.
    In some cases GDB will try to throw an exception through the
    libopcodes disassembler code, however, not all targets include
    exception unwind information when compiling C code, for targets that
    don't include this information GDB will terminate when trying to pass
    the exception through libopcodes.
    
    To explain what GDB is trying to do, consider the following trivial
    use of the Python disassembler API:
    
      class ExampleDisassembler(gdb.disassembler.Disassembler):
    
          class MyInfo(gdb.disassembler.DisassembleInfo):
              def __init__(self, info):
                  super().__init__(info)
    
              def read_memory(self, length, offset):
                  return super().read_memory(length, offset)
    
          def __init__(self):
              super().__init__("ExampleDisassembler")
    
          def __call__(self, info):
              info = self.MyInfo(info)
              return gdb.disassembler.builtin_disassemble(info)
    
    This disassembler doesn't add any value, it defers back to GDB to do
    all the actual work, but it serves to allow us to discuss the problem.
    
    The problem occurs when a Python exception is raised by the
    MyInfo.read_memory method.  The MyInfo.read_memory method is called
    from the C++ function gdbpy_disassembler::read_memory_func.  The C++
    stack at the point this function is called looks like this:
    
      #0  gdbpy_disassembler::read_memory_func (memaddr=4198805, buff=0x7fff9ab9d2a8 "\220ӹ\232\377\177", len=1, info=0x7fff9ab9d558) at ../../src/gdb/python/py-disasm.c:510
      #1  0x000000000104ba06 in fetch_data (info=0x7fff9ab9d558, addr=0x7fff9ab9d2a9 "ӹ\232\377\177") at ../../src/opcodes/i386-dis.c:305
      #2  0x000000000104badb in ckprefix (ins=0x7fff9ab9d100) at ../../src/opcodes/i386-dis.c:8571
      #3  0x000000000104e28e in print_insn (pc=4198805, info=0x7fff9ab9d558, intel_syntax=-1) at ../../src/opcodes/i386-dis.c:9548
      #4  0x000000000104f4d4 in print_insn_i386 (pc=4198805, info=0x7fff9ab9d558) at ../../src/opcodes/i386-dis.c:9949
      #5  0x00000000004fa7ea in default_print_insn (memaddr=4198805, info=0x7fff9ab9d558) at ../../src/gdb/arch-utils.c:1033
      #6  0x000000000094fe5e in i386_print_insn (pc=4198805, info=0x7fff9ab9d558) at ../../src/gdb/i386-tdep.c:4072
      #7  0x0000000000503d49 in gdbarch_print_insn (gdbarch=0x5335560, vma=4198805, info=0x7fff9ab9d558) at ../../src/gdb/gdbarch.c:3351
      #8  0x0000000000bcc8c6 in disasmpy_builtin_disassemble (self=0x7f2ab07f54d0, args=0x7f2ab0789790, kw=0x0) at ../../src/gdb/python/py-disasm.c:324
    
      ### ... snip lots of frames as we pass through Python itself ...
    
      #22 0x0000000000bcd860 in gdbpy_print_insn (gdbarch=0x5335560, memaddr=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/python/py-disasm.c:783
      #23 0x00000000008995a5 in ext_lang_print_insn (gdbarch=0x5335560, address=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/extension.c:939
      #24 0x0000000000741aaa in gdb_print_insn_1 (gdbarch=0x5335560, vma=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/disasm.c:1078
      #25 0x0000000000741bab in gdb_disassembler::print_insn (this=0x7fff9ab9e3c0, memaddr=0x401195, branch_delay_insns=0x0) at ../../src/gdb/disasm.c:1101
    
    So gdbpy_disassembler::read_memory_func is called from the libopcodes
    disassembler to read memory, this C++ function then calls into user
    supplied Python code to do the work.
    
    If the user supplied Python code raises an gdb.MemoryError exception
    indicating the memory read failed, this is fine.  The C++ code
    converts this exception back into a return value that libopcodes can
    understand, and returns to libopcodes.
    
    However, if the user supplied Python code raises some other exception,
    what we want is for this exception to propagate through GDB and appear
    as if raised by the call to gdb.disassembler.builtin_disassemble.  To
    achieve this, when gdbpy_disassembler::read_memory_func spots an
    unknown Python exception, we must pass the information about this
    exception from frame #0 to frame #8 in the above backtrace.  Frame #8
    is the C++ implementation of gdb.disassembler.builtin_disassemble, and
    so it is this function that we want to re-raise the unknown Python
    exception, so the user can, if they want, catch the exception in their
    code.
    
    The previous mechanism by which the exception was passed was to pack
    the details of the Python exception into a C++ exception, then throw
    the exception from frame #0, and catch the exception in frame #8,
    unpack the details of the Python exception, and re-raise it.
    
    However, this relies on the exception passing through frames #1 to #7,
    some of which are in libopcodes, which is C code, and so, might not be
    compiled with exception support.
    
    This commit proposes an alternative solution that does not rely on
    throwing a C++ exception.
    
    When we spot an unhandled Python exception in frame #0, we will store
    the details of this exception within the gdbpy_disassembler object
    currently in use.  Then we return to libopcodes a value indicating
    that the memory_read failed.
    
    libopcodes will now continue to disassemble as though that memory read
    failed (with one special case described below), then, when we
    eventually return to disasmpy_builtin_disassemble we check to see if
    there is an exception stored in the gdbpy_disassembler object.  If
    there is then this exception can immediately be installed, and then we
    return back to Python, when the user will be able to catch the
    exception.
    
    There is one extra change in gdbpy_disassembler::read_memory_func.
    After the first call that results in an exception being stored on the
    gdbpy_disassembler object, any future calls to the ::read_memory_func
    function will immediately return as if the read failed.  This avoids
    any additional calls into user supplied Python code.
    
    My thinking here is that should the first call fail with some unknown
    error, GDB should not keep trying with any additional calls.  This
    maintains the illusion that the exception raised from
    MyInfo.read_memory is immediately raised by
    gdb.disassembler.builtin_disassemble.  I have no tests for this change
    though - to trigger this issue would rely on a libopcodes disassembler
    that will try to read further memory even after the first failed
    read.  I'm not aware of any such disassembler that currently does
    this, but that doesn't mean such a disassembler couldn't exist in the
    future.
    
    With this change in place the gdb.python/py-disasm.exp test should now
    pass on AArch64.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29712
    
    Approved-By: Simon Marchi <simon.marchi@efficios.com>

Diff:
---
 gdb/python/py-disasm.c | 79 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 16 deletions(-)

diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index c37452fcf72..1d460997831 100644
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -122,6 +122,28 @@ struct gdbpy_disassembler : public gdb_printing_disassembler
     return m_string_file.release ();
   }
 
+  /* If there is a Python exception stored in this disassembler then
+     restore it (i.e. set the PyErr_* state), clear the exception within
+     this disassembler, and return true.  There must be no current
+     exception set (i.e. !PyErr_Occurred()) when this function is called,
+     as any such exception might get lost.
+
+     Otherwise, there is no exception stored in this disassembler, return
+     false.  */
+  bool restore_exception ()
+  {
+    gdb_assert (!PyErr_Occurred ());
+    if (m_stored_exception.has_value ())
+      {
+	gdbpy_err_fetch ex = std::move (*m_stored_exception);
+	m_stored_exception.reset ();
+	ex.restore ();
+	return true;
+      }
+
+    return false;
+  }
+
 private:
 
   /* Where the disassembler result is written.  */
@@ -138,6 +160,25 @@ private:
      memory source object then a pointer to the object is placed in here,
      otherwise, this field is nullptr.  */
   PyObject *m_memory_source;
+
+  /* Move the exception EX into this disassembler object.  */
+  void store_exception (gdbpy_err_fetch &&ex)
+  {
+    /* The only calls to store_exception are from read_memory_func, which
+       will return early if there's already an exception stored.  */
+    gdb_assert (!m_stored_exception.has_value ());
+    m_stored_exception.emplace (std::move (ex));
+  }
+
+  /* Return true if there is an exception stored in this disassembler.  */
+  bool has_stored_exception () const
+  {
+    return m_stored_exception.has_value ();
+  }
+
+  /* Store a single exception.  This is used to pass Python exceptions back
+     from ::memory_read to disasmpy_builtin_disassemble.  */
+  gdb::optional<gdbpy_err_fetch> m_stored_exception;
 };
 
 /* Return true if OBJ is still valid, otherwise, return false.  A valid OBJ
@@ -288,20 +329,15 @@ disasmpy_builtin_disassemble (PyObject *self, PyObject *args, PyObject *kw)
      the disassembled instruction, or -1 if there was a memory-error
      encountered while disassembling.  See below more more details on
      handling of -1 return value.  */
-  int length;
-  try
-    {
-      length = gdbarch_print_insn (disasm_info->gdbarch, disasm_info->address,
+  int length = gdbarch_print_insn (disasm_info->gdbarch, disasm_info->address,
 				   disassembler.disasm_info ());
-    }
-  catch (gdbpy_err_fetch &pyerr)
-    {
-      /* Reinstall the Python exception held in PYERR.  This clears to
-	 pointers held in PYERR, hence the need to catch as a non-const
-	 reference.  */
-      pyerr.restore ();
-      return nullptr;
-    }
+
+  /* It is possible that, while calling a user overridden memory read
+     function, a Python exception was raised that couldn't be
+     translated into a standard memory-error.  In this case the first such
+     exception is stored in the disassembler and restored here.  */
+  if (disassembler.restore_exception ())
+    return nullptr;
 
   if (length == -1)
     {
@@ -483,6 +519,14 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, gdb_byte *buff,
     = static_cast<gdbpy_disassembler *> (info->application_data);
   disasm_info_object *obj = dis->py_disasm_info ();
 
+  /* If a previous read attempt resulted in an exception, then we don't
+     allow any further reads to succeed.  We only do this check for the
+     read_memory_func as this is the only one the user can hook into,
+     thus, this check prevents us calling back into user code if a
+     previous call has already thrown an error.  */
+  if (dis->has_stored_exception ())
+    return -1;
+
   /* The DisassembleInfo.read_memory method expects an offset from the
      address stored within the DisassembleInfo object; calculate that
      offset here.  */
@@ -513,7 +557,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, gdb_byte *buff,
 	 exception and throw it, this will then be caught in
 	 disasmpy_builtin_disassemble, at which point the exception will be
 	 restored.  */
-      throw gdbpy_err_fetch ();
+      dis->store_exception (gdbpy_err_fetch ());
+      return -1;
     }
 
   /* Convert the result to a buffer.  */
@@ -523,7 +568,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, gdb_byte *buff,
     {
       PyErr_Format (PyExc_TypeError,
 		    _("Result from read_memory is not a buffer"));
-      throw gdbpy_err_fetch ();
+      dis->store_exception (gdbpy_err_fetch ());
+      return -1;
     }
 
   /* Wrap PY_BUFF so that it is cleaned up correctly at the end of this
@@ -536,7 +582,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, gdb_byte *buff,
       PyErr_Format (PyExc_ValueError,
 		    _("Buffer returned from read_memory is sized %d instead of the expected %d"),
 		    py_buff.len, len);
-      throw gdbpy_err_fetch ();
+      dis->store_exception (gdbpy_err_fetch ());
+      return -1;
     }
 
   /* Copy the data out of the Python buffer and return success.  */