[binutils-gdb] gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c

[binutils-gdb] gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c

[Andrew Burgess]

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

commit 44d9b0a174b08f283001f01aaf84102ba0a2726a
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Mar 9 10:58:54 2023 +0000

    gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c
    
    This commit copies the pattern that is present in many other py-*.c
    files: having a single macro to check that the Python object is still
    valid.
    
    This cleans up the code a little throughout the py-unwind.c file.
    
    Some of the exception messages will change slightly with this commit,
    though the type of the exceptions is still ValueError in all cases.
    
    I started writing some tests for this change and immediately ran into
    a problem: GDB would crash.  It turns out that the PendingFrame
    objects are not being marked as invalid!
    
    In pyuw_sniffer where the pending frames are created, we make use of a
    scoped_restore to invalidate the pending frame objects.  However, this
    only restores the pending_frame_object::frame_info field to its
    previous value -- and it turns out we never actually give this field
    an initial value, it's left undefined.
    
    So, when the scoped_restore (called invalidate_frame) performs its
    cleanup, it actually restores the frame_info field to an undefined
    value.  If this undefined value is not nullptr then any future
    accesses to the PendingFrame object result in undefined behaviour and
    most likely, a crash.
    
    As part of this commit I now initialize the frame_info field, which
    ensures all the new tests now pass.
    
    Reviewed-By: Tom Tromey <tom@tromey.com>

Diff:
---
 gdb/python/py-unwind.c                 | 53 +++++++++++++++++-----------------
 gdb/testsuite/gdb.python/py-unwind.exp | 19 ++++++++++++
 gdb/testsuite/gdb.python/py-unwind.py  |  7 +++++
 3 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index ee776bf8dea..12b14616363 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -52,6 +52,17 @@ show_pyuw_debug (struct ui_file *file, int from_tty,
 #define PYUW_SCOPED_DEBUG_ENTER_EXIT \
   scoped_debug_enter_exit (pyuw_debug, "py-unwind")
 
+/* Require a valid pending frame.  */
+#define PENDING_FRAMEPY_REQUIRE_VALID(pending_frame)	     \
+  do {							     \
+    if ((pending_frame)->frame_info == nullptr)		     \
+      {							     \
+	PyErr_SetString (PyExc_ValueError,		     \
+			 _("gdb.PendingFrame is invalid.")); \
+	return nullptr;					     \
+      }							     \
+  } while (0)
+
 struct pending_frame_object
 {
   PyObject_HEAD
@@ -215,21 +226,20 @@ unwind_infopy_str (PyObject *self)
 }
 
 /* Create UnwindInfo instance for given PendingFrame and frame ID.
-   Sets Python error and returns NULL on error.  */
+   Sets Python error and returns NULL on error.
+
+   The PYO_PENDING_FRAME object must be valid.  */
 
 static PyObject *
 pyuw_create_unwind_info (PyObject *pyo_pending_frame,
 			 struct frame_id frame_id)
 {
+  gdb_assert (((pending_frame_object *) pyo_pending_frame)->frame_info
+	      != nullptr);
+
   unwind_info_object *unwind_info
-      = PyObject_New (unwind_info_object, &unwind_info_object_type);
+    = PyObject_New (unwind_info_object, &unwind_info_object_type);
 
-  if (((pending_frame_object *) pyo_pending_frame)->frame_info == NULL)
-    {
-      PyErr_SetString (PyExc_ValueError,
-		       "Attempting to use stale PendingFrame");
-      return NULL;
-    }
   unwind_info->frame_id = frame_id;
   Py_INCREF (pyo_pending_frame);
   unwind_info->pending_frame = pyo_pending_frame;
@@ -365,15 +375,11 @@ static PyObject *
 pending_framepy_read_register (PyObject *self, PyObject *args)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
+  PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
   int regnum;
   PyObject *pyo_reg_id;
 
-  if (pending_frame->frame_info == NULL)
-    {
-      PyErr_SetString (PyExc_ValueError,
-		       "Attempting to read register from stale PendingFrame");
-      return NULL;
-    }
   if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
     return NULL;
   if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
@@ -417,6 +423,8 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
   CORE_ADDR pc;
   CORE_ADDR special;
 
+  PENDING_FRAMEPY_REQUIRE_VALID ((pending_frame_object *) self);
+
   if (!PyArg_ParseTuple (args, "O:create_unwind_info", &pyo_frame_id))
       return NULL;
   if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "sp", &sp))
@@ -451,12 +459,8 @@ pending_framepy_architecture (PyObject *self, PyObject *args)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
 
-  if (pending_frame->frame_info == NULL)
-    {
-      PyErr_SetString (PyExc_ValueError,
-		       "Attempting to read register from stale PendingFrame");
-      return NULL;
-    }
+  PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
   return gdbarch_to_arch_object (pending_frame->gdbarch);
 }
 
@@ -467,12 +471,8 @@ pending_framepy_level (PyObject *self, PyObject *args)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
 
-  if (pending_frame->frame_info == NULL)
-    {
-      PyErr_SetString (PyExc_ValueError,
-		       "Attempting to read stack level from stale PendingFrame");
-      return NULL;
-    }
+  PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
   int level = frame_relative_level (pending_frame->frame_info);
   return gdb_py_object_from_longest (level).release ();
 }
@@ -538,6 +538,7 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
       return 0;
     }
   pfo->gdbarch = gdbarch;
+  pfo->frame_info = nullptr;
   scoped_restore invalidate_frame = make_scoped_restore (&pfo->frame_info,
 							 this_frame);
 
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 337e5dc2504..3e214ee0f45 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -142,3 +142,22 @@ gdb_test "python obj = simple_unwinder(True)" \
     [multi_line \
 	 "TypeError: incorrect type for name: <class 'bool'>" \
 	 "Error while executing Python code\\."]
+
+# Now register the simple_unwinder with a valid name, and use the
+# unwinder to capture a PendingFrame object.
+gdb_test_no_output "python obj = simple_unwinder(\"simple\")"
+gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj)"
+check_for_broken_backtrace "backtrace to capture a PendingFrame object"
+
+# Call methods on the captured gdb.PendingFrame and check we see the
+# expected error.
+gdb_test_no_output "python pf = captured_pending_frame"
+foreach cmd {"pf.read_register(\"pc\")" \
+		 "pf.create_unwind_info(None)" \
+		 "pf.architecture()" \
+		 "pf.level()"} {
+    gdb_test "python $cmd" \
+	[multi_line \
+	     "ValueError: gdb\\.PendingFrame is invalid\\." \
+	     "Error while executing Python code\\."]
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index edd2e30eb9b..b30e843e7e5 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -133,12 +133,19 @@ class TestUnwinder(Unwinder):
 global_test_unwinder = TestUnwinder()
 gdb.unwinder.register_unwinder(None, global_test_unwinder, True)
 
+# This is filled in by the simple_unwinder class.
+captured_pending_frame = None
+
 
 class simple_unwinder(Unwinder):
     def __init__(self, name):
         super().__init__(name)
 
     def __call__(self, pending_frame):
+        global captured_pending_frame
+
+        if captured_pending_frame is None:
+            captured_pending_frame = pending_frame
         return None