From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by sourceware.org (Postfix) with ESMTPS id D83AD3861962 for ; Thu, 9 Jul 2020 11:12:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D83AD3861962 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wm1-f66.google.com with SMTP id a6so6033214wmm.0 for ; Thu, 09 Jul 2020 04:12:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=T+QjV4S8iWzyX9vMbim1Jmfnr8ztDryvBXYqAneX6tQ=; b=kMU+WJXd6GfF6rLWbmGfi/jtIRQcuragm0z9LzpWtVf0i7jnYHEtoBm3qXa8PsgqwJ ggipc1fkx9KgnfSlO8d2Tq3xXsVDnRTs0NUrMRoddj2v5Z3d7qpSkxn4AlSIhmC3JBDN wCYfE1ionW+0NdY0j+6GFBR+RUm573ZEy4GMlGm5R8j8X2XCB00iC1eGoZ6/Ke6fNtl8 ZMwBGMOu1NSOekEAweB0JOD8vMK/LacemRW09jXhVdVhkT8eNi2t/g9cSG6/arLbV7tq smRYm4jBmxT6acfNAlvT7f/aMNkqQyAtVBvpWKEH/1XXKdeK5IKzkVvIl44nJ4n3tyga lnnA== X-Gm-Message-State: AOAM5327CJfaO+Z+nfazQ30DmyFRov7uz2+RrTmMPxBspE4RQBdKy9/d wRlP6YkcrsYHzn7ExcqpaDjFiAHQLsk= X-Google-Smtp-Source: ABdhPJz590Zs2HCgw4ytAHXvVY3JEEnsEUX9Nw42zLnf4SzeLGaJkyPdqWAkAtOi4uk8eJQXn6Rbyg== X-Received: by 2002:a1c:2d0c:: with SMTP id t12mr13401420wmt.43.1594293151938; Thu, 09 Jul 2020 04:12:31 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91a:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f91a:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id x11sm3825478wmc.26.2020.07.09.04.12.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jul 2020 04:12:30 -0700 (PDT) Subject: Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 To: Simon Marchi , gdb-patches@sourceware.org References: <20200708233125.1030-1-pedro@palves.net> <20200708233125.1030-3-pedro@palves.net> From: Pedro Alves Message-ID: <1809ad79-7542-776d-cb1f-8a1ce0ed6825@palves.net> Date: Thu, 9 Jul 2020 12:12:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LOTSOFHASH, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 09 Jul 2020 11:12:36 -0000 On 7/9/20 4:31 AM, Simon Marchi wrote: > On 2020-07-08 7:31 p.m., Pedro Alves wrote: >> Running the testsuite against an Asan-enabled build of GDB makes >> gdb.base/multi-target.exp expose this bug. >> >> scoped_restore_current_thread's ctor calls get_frame_id to record the >> selected frame's ID to restore later. If the frame ID hasn't been >> computed yet, it will be computed on the spot, and that will usually >> require accessing the target's memory and registers. If the remote >> connection closes, while we're computing the frame ID, the remote >> target exits its inferiors, unpushes itself, and throws a >> TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the >> inferior's threads. >> >> scoped_restore_current_thread increments the current thread's refcount >> to prevent the thread from being deleted from under its feet. >> However, the code that does that isn't considering the case of the >> thread being deleted from within get_frame_id. It only increments the >> refcount _after_ get_frame_id returns. So if the current thread is >> indeed deleted, the >> >> tp->incref (); >> >> statement references a stale TP pointer. >> >> Incrementing the refcounts earlier fixes it. >> >> We should probably also let the TARGET_CLOSE_ERROR error propagate in >> this case. That alone would fix it, though it seems better to tweak >> the refcount handling too. > > So, when the target closes while we (scoped_restore_current_thread) own > a reference on the inferior and thread, the inferior and thread are still > destroyed, and so we shouldn't decref them? Aw, no, I got confused and misremembered how exceptions in ctors work. The dtor for scoped_restore_current_thread isn't called, and I assumed it was called. We need to decref the inferior and thread before letting the exception propagate, otherwise we leak them. I'm testing the updated version of the patch below, which does that: /* Better let this propagate. */ if (ex.error == TARGET_CLOSE_ERROR) { m_thread->decref (); m_inf->decref (); throw; } It passes multi-target.exp with Asan-enabled GDB. Running the full testsuite now. >From 1ad36a4b892fc4425d6f24c298713eeafece7b04 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 7 Jul 2020 01:50:10 +0100 Subject: [PATCH] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Running the testsuite against an Asan-enabled build of GDB makes gdb.base/multi-target.exp expose this bug. scoped_restore_current_thread's ctor calls get_frame_id to record the selected frame's ID to restore later. If the frame ID hasn't been computed yet, it will be computed on the spot, and that will usually require accessing the target's memory and registers. If the remote connection closes, while we're computing the frame ID, the remote target exits its inferiors, unpushes itself, and throws a TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the inferior's threads. scoped_restore_current_thread increments the current thread's refcount to prevent the thread from being deleted from under its feet. However, the code that does that isn't considering the case of the thread being deleted from within get_frame_id. It only increments the refcount _after_ get_frame_id returns. So if the current thread is indeed deleted, the tp->incref (); statement references a stale TP pointer. Incrementing the refcounts earlier fixes it. We should probably also let the TARGET_CLOSE_ERROR error propagate in this case. That alone would fix it, though it seems better to tweak the refcount handling too. gdb/ChangeLog: * thread.c (scoped_restore_current_thread::scoped_restore_current_thread): Incref the thread before calling get_frame_id instead of after. Let TARGET_CLOSE_ERROR propagate. --- gdb/thread.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/gdb/thread.c b/gdb/thread.c index f0722d3588..a3c2be7dd0 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1433,15 +1433,17 @@ scoped_restore_current_thread::~scoped_restore_current_thread () scoped_restore_current_thread::scoped_restore_current_thread () { - m_thread = NULL; m_inf = current_inferior (); + m_inf->incref (); if (inferior_ptid != null_ptid) { - thread_info *tp = inferior_thread (); + m_thread = inferior_thread (); + m_thread->incref (); + struct frame_info *frame; - m_was_stopped = tp->state == THREAD_STOPPED; + m_was_stopped = m_thread->state == THREAD_STOPPED; if (m_was_stopped && target_has_registers && target_has_stack @@ -1466,13 +1468,18 @@ scoped_restore_current_thread::scoped_restore_current_thread () { m_selected_frame_id = null_frame_id; m_selected_frame_level = -1; - } - tp->incref (); - m_thread = tp; + /* Better let this propagate. */ + if (ex.error == TARGET_CLOSE_ERROR) + { + m_thread->decref (); + m_inf->decref (); + throw; + } + } } - - m_inf->incref (); + else + m_thread = NULL; } /* See gdbthread.h. */ base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f -- 2.14.5