From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 383953858C50 for ; Tue, 29 Mar 2022 10:10:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 383953858C50 Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-649-RviljegxMi2l-Qj4Zy0SpA-1; Tue, 29 Mar 2022 06:10:23 -0400 X-MC-Unique: RviljegxMi2l-Qj4Zy0SpA-1 Received: by mail-wr1-f72.google.com with SMTP id z1-20020adfec81000000b001f1f7e7ec99so4907096wrn.17 for ; Tue, 29 Mar 2022 03:10:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=j/h/Jj1b3bfkeT4R9fWBj29dN0BL2imqmq8zvdV5EQw=; b=TKd3NX6WCh7jSY7xV2bwofbL3M1n+Udu8SpeTFxLKX3oQQ/pneqCeEkwatOvBMf4fr 5xx50A7bZ6OEBElry/F4m6/UQW2aId92btvAe0MhuC2jByKfxaZk0r6GerTw10YBzzbm Dp8O4QekQ7SMPz+Rh9mqbeS4nj+p3DlV6btoMhXqICDRgGghOXhUz6dRTmSSZZDrhXZT qgoePzid5yf1POyUrxx0nIgCSZ/CV24QjIwkBpgBA4sqWfHc4OKjXfhPtTaFjJXtreDa uanao9X2ETmNdvDdly3dMNVxusZ0NDtUEKeyjd4EPYeW1bzJoY1yFd9/KewV5tsEUQbx lWHw== X-Gm-Message-State: AOAM532Cy785jFFYFk0UoIcFhhwJjCYfFIJNLXLwhXEF+3oWXd+xaIpR UX1P0n2Sgexbu2u1KKnqv26GkYwYZjMKyNC542QAaKePVhAKyz1onZwRYStO6yPDrz5Bmo6s418 ax3I1QkQjc+xQqozBGot0ow== X-Received: by 2002:a05:6000:154c:b0:203:d46b:ede4 with SMTP id 12-20020a056000154c00b00203d46bede4mr30376162wry.501.1648548620505; Tue, 29 Mar 2022 03:10:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyZrfWsKKrVgLPgjpj8dzINc5GcPg2PftSgeWVCM/MCGC0r6l1BeA0vQwW25nZ6mVO6uTTOGg== X-Received: by 2002:a05:6000:154c:b0:203:d46b:ede4 with SMTP id 12-20020a056000154c00b00203d46bede4mr30376130wry.501.1648548620229; Tue, 29 Mar 2022 03:10:20 -0700 (PDT) Received: from localhost (host86-169-131-113.range86-169.btcentralplus.com. [86.169.131.113]) by smtp.gmail.com with ESMTPSA id 11-20020a056000156b00b002040674fd13sm17387785wrz.38.2022.03.29.03.10.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Mar 2022 03:10:19 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications In-Reply-To: <052a55b2-7f6c-61f1-a4d8-a23a0a46126b@simark.ca> References: <20220324185254.2582384-1-aburgess@redhat.com> <052a55b2-7f6c-61f1-a4d8-a23a0a46126b@simark.ca> Date: Tue, 29 Mar 2022 11:10:18 +0100 Message-ID: <87tubh9iid.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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: Tue, 29 Mar 2022 10:10:28 -0000 Simon Marchi writes: > 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. I updated the comment as you suggested and pushed this patch to mater and gdb-12-branch. Thanks, Andrew