[PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications

[Andrew Burgess <aburgess@redhat.com>]

Simon,

Thanks for the feedback.  I believe this addresses all your points.
What do you think?

Thanks,
Andrew

---

In commit:

  commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
  Date:   Wed Mar 16 15:08:22 2022 +0000

      gdb/mi: consistently notify user when GDB/MI client uses -thread-select

Changes were made to GDB to address some inconsistencies in when
notifications are sent from a MI terminal to a CLI terminal (when
multiple terminals are in use, see new-ui command).

Unfortunately, in order to track when the currently selected frame has
changed, that commit grabs a frame_info pointer before and after an MI
command has executed, and compares the pointers to see if the frame
has changed.

This is not safe.

If the frame cache is deleted for any reason then the frame_info
pointer captured before the command started, is no longer valid, and
any comparisons based on that pointer are undefined.

This was leading to random test failures for some folk, see:

  https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html

This commit changes GDB so we no longer hold frame_info pointers, but
instead store the frame_id and frame_level, this is safe even when the
frame cache is flushed.
---
 gdb/mi/mi-main.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index abd033b22ae..28d4afc28c0 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1974,27 +1974,50 @@ struct user_selected_context
 {
   /* Constructor.  */
   user_selected_context ()
-    : m_previous_ptid (inferior_ptid),
-      m_previous_frame (deprecated_safe_get_selected_frame ())
-  { /* Nothing.  */ }
+    : m_previous_ptid (inferior_ptid)
+  {
+    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
+  }
 
   /* Return true if the user selected context has changed since this object
      was created.  */
   bool has_changed () const
   {
-    return ((m_previous_ptid != null_ptid
-	     && inferior_ptid != null_ptid
-	     && m_previous_ptid != inferior_ptid)
-	    || m_previous_frame != deprecated_safe_get_selected_frame ());
+    /* Did the selected thread change?  */
+    if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
+	&& m_previous_ptid != inferior_ptid)
+      return true;
+
+    /* Grab details of the currently selected frame, for comparison.  */
+    frame_id current_frame_id;
+    int current_frame_level;
+    save_selected_frame (&current_frame_id, &current_frame_level);
+
+    /* Did the selected frame level change?  */
+    if (current_frame_level != m_previous_frame_level)
+      return true;
+
+    /* Did the selected frame id change?  If the innermost frame is
+       selected then the level will be -1, and the frame-id will be
+       null_frame_id.  As comparing null_frame_id with itself always
+       reports not-equal, we only do the equality test if we have something
+       other than the innermost frame selected.  */
+    if (current_frame_level != -1
+	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
+      return true;
+
+    /* Nothing changed!  */
+    return false;
   }
 private:
   /* The previously selected thread.  This might be null_ptid if there was
      no previously selected thread.  */
   ptid_t m_previous_ptid;
 
-  /* The previously selected frame.  This might be nullptr if there was no
-     previously selected frame.  */
-  frame_info *m_previous_frame;
+  /* The previously selected frame.  If the innermost frame is selected
+     then the frame_id will be null_frame_id, and the level will be -1.  */
+  frame_id m_previous_frame_id;
+  int m_previous_frame_level;
 };
 
 static void
-- 
2.25.4