From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 67C0A3858C54 for ; Thu, 24 Mar 2022 20:32:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 67C0A3858C54 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 024E81EDF0; Thu, 24 Mar 2022 16:32:16 -0400 (EDT) Message-ID: <052a55b2-7f6c-61f1-a4d8-a23a0a46126b@simark.ca> Date: Thu, 24 Mar 2022 16:32:16 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications Content-Language: tl To: Andrew Burgess , gdb-patches@sourceware.org References: <20220324185254.2582384-1-aburgess@redhat.com> From: Simon Marchi In-Reply-To: <20220324185254.2582384-1-aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3645.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Mar 2022 20:32:18 -0000 On 2022-03-24 14:52, Andrew Burgess via Gdb-patches wrote: > 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 (¤t_frame_id, ¤t_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. */ I noticed you changed this from "if no frame is currently selected" to "if the innermost frame is selected". I think that both can happen, actually: - if you execute an MI command while the target is running, there is no selected frame - if you execute an MI command while the innermost frame is selected So my understanding is that you could mention both (although it might become a bit redundant with the big comment on top of selected_frame_id, in frame.c). LGTM in any case. Simon