diff mbox series

[2/3] gdb/python: fix use of frame_info_ptr within pending_frame_object

Message ID cc0b6f8646c10a9e50b5980635f3dd605764cc61.1778839043.git.aburgess@redhat.com
State New
Headers
Series Use C++ type traits check to catch bugs in Python API |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Patch failed to apply

Commit Message

Andrew Burgess May 15, 2026, 10 a.m. UTC 
  The previous commit added a type trait which identifies types that
should not be used within Python objects, that is, types that are not
trivially default constructible.  As a result of this, it was
discovered that pending_frame_object includes a frame_info_ptr field.

The problem with frame_info_ptr is that its constructor registers the
new frame_info_ptr with the global frame_list.  It is by this
registration that invalidation of frame_info_ptr objects is performed.

As Python is written in C, C++ constructors are not called, so when a
pending_frame_object is created the constructor for the nested
frame_info_ptr field is never run, and the frame_info_ptr is never
registered with the global frame_list.  As a result the frame_info_ptr
will never be invalidated if the frame cache is flushed, this can then
lead to problems where we make use of the 'frame_info *' within the
frame_info_ptr, even though it is no longer valid.

In this commit I change the frame_info_ptr within pending_frame_object
to a 'frame_info_ptr *' and allocate the frame_info_ptr object on the
heap, releasing the object, and resetting the point to NULL, when we
are done with it.  As the pending_frame_object only needs to remain
valid for the duration of frame_unwind_python::sniff, the 'new' and
'delete' both performed within the function.

We can now check that a pending_frame_object is valid by checking if
the 'frame_info_ptr *' is NULL or not.  As the frame_info_ptr is
created in a valid state, and the point is set back to NULL when we
are done with it, we no longer need to compare the frame_info_ptr
object itself against NULL.

The remaining changes in this patch are to dereference the
'frame_info_ptr *' in places where we need the actual object.  In some
cases I need to move the dereference later within a function, after a
validity check, in order to avoid dereferencing a NULL pointer.

Finally, I can add the static_assert that guarantees that
pending_frame_object is now safe for allocation by Python.

I discovered this bug while looking at PR gdb/32120.  That bug is
about a user's custom frame unwinder that triggers a flush of the
frame cache during the sniffer phase (the
RemoteTargetConnection.send_packet call switches thread, which
triggers the frame cache flush).  While looking at that bug I noticed
that the frame_info_ptr within the pending_frame_object wasn't being
reset when the frame cache was flushed.  Fixing this does not resolve
the user's issue, but I thought it was still worth tagging this commit
with the bug link.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32120
---
 gdb/python/py-unwind.c | 53 ++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 20 deletions(-)

Comments

Tom Tromey May 15, 2026, 5:22 p.m. UTC | #1 
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The problem with frame_info_ptr is that its constructor registers the
Andrew> new frame_info_ptr with the global frame_list.  It is by this
Andrew> registration that invalidation of frame_info_ptr objects is performed.

Thanks for finding & fixing this.

Andrew> +  frame_info_ptr frame (*pending_frame->frame_info);
Andrew>    return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
Andrew>  			       gdbpy_py_obj_tp_name (self),
Andrew>  			       frame_relative_level (frame),

This could be 'const frame_info_ptr &frame (...)' and avoid some updates
to globals, since no copy will be done.

I didn't look closely but perhaps other spots could do the same.

It may not matter so if you want to ignore this that's fine by me.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Andrew Burgess May 16, 2026, 12:28 p.m. UTC | #2 
Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The problem with frame_info_ptr is that its constructor registers the
> Andrew> new frame_info_ptr with the global frame_list.  It is by this
> Andrew> registration that invalidation of frame_info_ptr objects is performed.
>
> Thanks for finding & fixing this.
>
> Andrew> +  frame_info_ptr frame (*pending_frame->frame_info);
> Andrew>    return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
> Andrew>  			       gdbpy_py_obj_tp_name (self),
> Andrew>  			       frame_relative_level (frame),
>
> This could be 'const frame_info_ptr &frame (...)' and avoid some updates
> to globals, since no copy will be done.
>
> I didn't look closely but perhaps other spots could do the same.

In fact they all can change!  I made this update (and retested) before
pushing this patch.

Thanks,
Andrew

>
> It may not matter so if you want to ignore this that's fine by me.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom
diff mbox series

Patch

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index ed0ba89d267..fd305cb3316 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -68,13 +68,17 @@  show_pyuw_debug (struct ui_file *file, int from_tty,
 
 struct pending_frame_object : public PyObject
 {
-  /* Frame we are unwinding.  */
-  frame_info_ptr frame_info;
+  /* Frame we are unwinding.  We cannot place a frame_info_ptr
+     directly within this struct as it is not trivially default
+     constructable.  */
+  frame_info_ptr *frame_info;
 
   /* Its architecture, passed by the sniffer caller.  */
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<pending_frame_object>);
+
 /* Saved registers array item.  */
 
 struct saved_reg
@@ -245,9 +249,8 @@  unwind_infopy_repr (PyObject *self)
   unwind_info_object *unwind_info = (unwind_info_object *) self;
   pending_frame_object *pending_frame
     = (pending_frame_object *) (unwind_info->pending_frame);
-  frame_info_ptr frame = pending_frame->frame_info;
 
-  if (frame == nullptr)
+  if (pending_frame->frame_info == nullptr)
     return PyUnicode_FromFormat ("<%s for an invalid frame>",
 				 gdbpy_py_obj_tp_name (self));
 
@@ -263,6 +266,7 @@  unwind_infopy_repr (PyObject *self)
 	saved_reg_names = (saved_reg_names + ", ") + name;
     }
 
+  frame_info_ptr frame (*pending_frame->frame_info);
   return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
 			       gdbpy_py_obj_tp_name (self),
 			       frame_relative_level (frame),
@@ -331,7 +335,7 @@  unwind_infopy_add_saved_register (PyObject *self, PyObject *args, PyObject *kw)
   if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
     {
       struct value *user_reg_value
-	= value_of_user_reg (regnum, pending_frame->frame_info);
+	= value_of_user_reg (regnum, *pending_frame->frame_info);
       if (user_reg_value->lval () == lval_register)
 	regnum = user_reg_value->regnum ();
       if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
@@ -414,14 +418,15 @@  unwind_infopy_dealloc (PyObject *self)
 static PyObject *
 pending_framepy_str (PyObject *self)
 {
-  frame_info_ptr frame = ((pending_frame_object *) self)->frame_info;
+  pending_frame_object *pending_frame = (pending_frame_object *) self;
   const char *sp_str = NULL;
   const char *pc_str = NULL;
 
-  if (frame == NULL)
+  if (pending_frame->frame_info == nullptr)
     return PyUnicode_FromString ("Stale PendingFrame instance");
   try
     {
+      frame_info_ptr frame (*pending_frame->frame_info);
       sp_str = core_addr_to_string_nz (get_frame_sp (frame));
       pc_str = core_addr_to_string_nz (get_frame_pc (frame));
     }
@@ -439,14 +444,15 @@  static PyObject *
 pending_framepy_repr (PyObject *self)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
-  frame_info_ptr frame = pending_frame->frame_info;
 
-  if (frame == nullptr)
+  if (pending_frame->frame_info == nullptr)
     return gdb_py_invalid_object_repr (self);
 
   const char *sp_str = nullptr;
   const char *pc_str = nullptr;
 
+  frame_info_ptr frame (*pending_frame->frame_info);
+
   try
     {
       sp_str = core_addr_to_string_nz (get_frame_sp (frame));
@@ -493,7 +499,7 @@  pending_framepy_read_register (PyObject *self, PyObject *args, PyObject *kw)
 	 get_frame_register_value() was used here, which did not
 	 handle the user register case.  */
       value *val = value_of_register
-	(regnum, get_next_frame_sentinel_okay (pending_frame->frame_info));
+	(regnum, get_next_frame_sentinel_okay (*pending_frame->frame_info));
       if (val == NULL)
 	PyErr_Format (PyExc_ValueError,
 		      "Cannot read register %d from frame.",
@@ -520,6 +526,10 @@  pending_framepy_is_valid (PyObject *self, PyObject *args)
   if (pending_frame->frame_info == nullptr)
     Py_RETURN_FALSE;
 
+  /* The frame_info field should never point at an uninitialized
+     object.  */
+  gdb_assert (*pending_frame->frame_info != nullptr);
+
   Py_RETURN_TRUE;
 }
 
@@ -538,7 +548,7 @@  pending_framepy_name (PyObject *self, PyObject *args)
   try
     {
       enum language lang;
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       name = find_frame_funname (frame, &lang, nullptr);
     }
@@ -568,7 +578,7 @@  pending_framepy_pc (PyObject *self, PyObject *args)
 
   try
     {
-      pc = get_frame_pc (pending_frame->frame_info);
+      pc = get_frame_pc (*pending_frame->frame_info);
     }
   catch (const gdb_exception &except)
     {
@@ -590,7 +600,7 @@  pending_framepy_language (PyObject *self, PyObject *args)
 
   try
     {
-      frame_info_ptr fi = pending_frame->frame_info;
+      frame_info_ptr fi = *pending_frame->frame_info;
 
       enum language lang = get_frame_language (fi);
       const language_defn *lang_def = language_def (lang);
@@ -615,7 +625,7 @@  pending_framepy_find_sal (PyObject *self, PyObject *args)
 
   try
     {
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       symtab_and_line sal = find_frame_sal (frame);
       return symtab_and_line_to_sal_object (sal).release ();
@@ -636,7 +646,7 @@  pending_framepy_block (PyObject *self, PyObject *args)
 
   PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
 
-  frame_info_ptr frame = pending_frame->frame_info;
+  frame_info_ptr frame = *pending_frame->frame_info;
   const struct block *block = nullptr, *fn_block;
 
   try
@@ -682,7 +692,7 @@  pending_framepy_function (PyObject *self, PyObject *args)
   try
     {
       enum language funlang;
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       gdb::unique_xmalloc_ptr<char> funname
 	= find_frame_funname (frame, &funlang, &sym);
@@ -774,7 +784,7 @@  pending_framepy_level (PyObject *self, PyObject *args)
 
   PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
 
-  int level = frame_relative_level (pending_frame->frame_info);
+  int level = frame_relative_level (*pending_frame->frame_info);
   return gdb_py_object_from_longest (level).release ();
 }
 
@@ -863,9 +873,12 @@  frame_unwind_python::sniff (const 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);
+  pfo->frame_info = new frame_info_ptr (this_frame);
+  SCOPE_EXIT
+    {
+      delete pfo->frame_info;
+      pfo->frame_info = nullptr;
+    };
 
   /* Run unwinders.  */
   if (gdb_python_module == NULL