From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42560 invoked by alias); 5 Nov 2019 23:52:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42549 invoked by uid 89); 5 Nov 2019 23:52:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham version=3.3.1 spammy=commenting, Junior, 0400, Durigan X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Nov 2019 23:52:25 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id B95B02048C; Tue, 5 Nov 2019 18:52:23 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id 136C7201E0; Tue, 5 Nov 2019 18:52:22 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id DB31625B28; Tue, 5 Nov 2019 18:52:21 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Tue, 05 Nov 2019 23:52:00 -0000 From: "Pedro Alves (Code Review)" To: Sergio Durigan Junior , gdb-patches@sourceware.org Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] Make sure we have a valid target on top when pushing a new target X-Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f X-Gerrit-Change-Number: 483 X-Gerrit-ChangeURL: X-Gerrit-Commit: 9ef2f42f9cbbd0fa45026c061f5bf978f2485e5e In-Reply-To: References: X-Gerrit-Comment-Date: Tue, 5 Nov 2019 18:52:21 -0500 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 Message-Id: <20191105235221.DB31625B28@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-11/txt/msg00137.txt.bz2 Pedro Alves has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483 ...................................................................... Patch Set 1: Code-Review-1 (3 comments) Thanks for the fix. I don't think it's the right one, but the right one is in the area. The test looks good, but I think you should rename the file. More details in the comments. https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1//COMMIT_MSG Commit Message: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1//COMMIT_MSG@7 PS1, Line 7: 2 | Author: Sergio Durigan Junior 3 | AuthorDate: 2019-10-30 13:58:29 -0400 4 | Commit: Sergio Durigan Junior 5 | CommitDate: 2019-11-01 11:53:06 -0400 6 | 7 > Make sure we have a valid target on top when pushing a new target 8 | 9 | Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117 10 | 11 | A segfault can happen in a specific scenario when using TUI + a 12 | corefile, as explained in the bug mentioned above. The problem I'd rename this to "Fix crash with core + TUI + run" The current subject is talking in terms of the fix, but I think the fix isn't the right one. More below. https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/target.c File gdb/target.c: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/target.c@574 PS1, Line 574: 559 | target_stack::push (target_ops *t) | ... 569 | m_stack[stratum] = t; 570 | 571 | /* And close the previous one, if it exists. */ 572 | if (prev != nullptr) 573 | target_close (prev); 574 > 575 | if (m_top < stratum) 576 | m_top = stratum; 577 | } 578 | 579 | /* See target.h. */ This isn't the right fix. The reason we're clearing m_stack[stratum] before calling target_close is so that target method calls within the target_close implementation don't reach the target. There's a comment about it at the end of target_stack::unpush(target_ops *): > /* Finally close the target. Note we do this after unchaining, so > any target method calls from within the target_close > implementation don't end up in T anymore. */ > target_close (t); With your change, we're now reaching the target_close implementation with the new target pushed on the stack, so if the target_close implementation calls any target method, it reaches the new target! I.e., target_shortname inside the target_close call for the core target is returning the shortname of the native target. Imagine if we end up trying to read memory, for example. The real problem is that we're clearing m_stack[stratum], but forget to adjust m_top. target_stack::unpush does exactly that: > /* Unchain the target. */ > m_stack[stratum] = NULL; > > if (m_top == stratum) > m_top = t->beneath ()->stratum (); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > /* Finally close the target. Note we do this after unchaining, so > any target method calls from within the target_close > implementation don't end up in T anymore. */ > target_close (t); ... so the right fix is to just call unpush, like so: if (m_stack[stratum] != NULL) - { - target_ops *prev = m_stack[stratum]; - m_stack[stratum] = NULL; - target_close (prev); - } + unpush (m_stack[stratum]); https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp File gdb/testsuite/gdb.tui/segfault-corefile-run.exp: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp@1 PS1, Line 1: 1 > # Copyright 2019 Free Software Foundation, Inc. 2 | 3 | # This program is free software; you can redistribute it and/or modify 4 | # it under the terms of the GNU General Public License as published by 5 | # the Free Software Foundation; either version 3 of the License, or 6 | # (at your option) any later version. I don't know how to comment on a file name, so I'm commenting here. I'd rather rename the file to "corefile-run.exp". I.e., name it for what it tests, leave the bug description to the comments, not the file name. -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f Gerrit-Change-Number: 483 Gerrit-PatchSet: 1 Gerrit-Owner: Sergio Durigan Junior Gerrit-Reviewer: Pedro Alves Gerrit-Reviewer: Sergio Durigan Junior Gerrit-Comment-Date: Tue, 05 Nov 2019 23:52:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment