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

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

[Andrew Burgess]

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

commit 9e6a252c062f9cee69007d9ba19eea5e89e675f4
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Mar 23 19:00:35 2022 +0000

    gdb/mi: fix use after free of frame_info causing spurious notifications
    
    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.

Diff:
---
 gdb/mi/mi-main.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index abd033b22ae..91eb8d1edd8 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1974,27 +1974,51 @@ 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, or
+     no 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