From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id MYIJNJfO5F/OPQAAWB0awg (envelope-from ) for ; Thu, 24 Dec 2020 12:23:35 -0500 Received: by simark.ca (Postfix, from userid 112) id C5AB41F0AA; Thu, 24 Dec 2020 12:23:35 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 778231E99A for ; Thu, 24 Dec 2020 12:23:33 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 278CB3898537; Thu, 24 Dec 2020 17:23:33 +0000 (GMT) Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by sourceware.org (Postfix) with ESMTPS id BD6C13898537 for ; Thu, 24 Dec 2020 17:23:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BD6C13898537 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42a.google.com with SMTP id t30so2718453wrb.0 for ; Thu, 24 Dec 2020 09:23:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=i7U70H+xdE8Matx7FpoAerPRzBMiwBdBRHnsqOt4pNM=; b=V7jxFebVIrjBv293tKgXxdxN5f4gdsJGmhf7vrMJm3VWSsy/vXy0LKPUO6N39nx3UO ZrQP01H1GN9T97Kv3CQhGoJMvvPQH110vgMTuZ0MRvcV99dEnSAPKDXi7sgnTgMy2ovR QNfudy7PXKrMwbnG+nQ3TmJf6eGvZ2jsxs1xs0ENoXTxhZhYInZ6MbWmpV1fyYrHv9l7 qFqy+KCNr/BxIEYBJtSKP5vz8f9CrexrEGCi4OpY2D47VQFdKOtA1Bs46ZStaWWj0r+K uicDpj3QEhRo148I0FHn3kofD+29QPUJJC5rlAW3y7l893RGDR+DdouO8lIJzqHkK0V1 88/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=i7U70H+xdE8Matx7FpoAerPRzBMiwBdBRHnsqOt4pNM=; b=tJT8Q+E2N8mSYx5LevehGw3dbqR6dgb4GUtf6odQSubH72sLeRHXmfgmIfbi0Ys0Ni jowYfwiuzCsqlHSW9Itxo6y8mtgK7ZdiA9Ydh69ORFCUSXRoHDuWP6ml4Ug4eNHPXApC h0/3zA+/837loErRslS3yFCpjNv7uA4De1B1KY1cLINylOaj7bxxsUI120PqIMiLXpTq K7jGQFlyzQB/iroMgt+HdZKJUnGD4utKUXsLRwgIqgH7TAQu/R/LElYVPSB2QwAmExos JgONDXn02NheaAW1IJSmRMMWJolpZQkacE9NPn1JB1pz02w8Y7xN6T/bCJkNQrZRyIju kSeA== X-Gm-Message-State: AOAM532aUlGJ+VyZ7No7oBSc8ZYC2QnHglfxrHuksCqoynZhdn/bBgn4 IFf1YhpONCjNW9VH81J3dkABBjN2E/Go6A== X-Google-Smtp-Source: ABdhPJxnzebbc7iSUfQs5inWFj3R/HWYSWx1bqmXBIp+IjnMhz7Uu2ZzJ6lcqf2as+OfZZAn54/dxA== X-Received: by 2002:a5d:43cc:: with SMTP id v12mr9720957wrr.319.1608830599814; Thu, 24 Dec 2020 09:23:19 -0800 (PST) Received: from localhost (host109-154-20-128.range109-154.btcentralplus.com. [109.154.20.128]) by smtp.gmail.com with ESMTPSA id a14sm39014140wrn.3.2020.12.24.09.23.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Dec 2020 09:23:19 -0800 (PST) Date: Thu, 24 Dec 2020 17:23:18 +0000 From: Andrew Burgess To: Simon Marchi Subject: Re: [PATCH 2/4] gdb: make remote target clear its async event handler in wait Message-ID: <20201224172318.GP2945@embecosm.com> References: <20201130165251.830482-1-simon.marchi@efficios.com> <20201130165251.830482-3-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201130165251.830482-3-simon.marchi@efficios.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 17:17:54 up 15 days, 22:02, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] 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: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" * Simon Marchi via Gdb-patches [2020-11-30 11:52:49 -0500]: > The remote target's remote_async_inferior_event_token field is a flag > that tells when it has an event to report and it wants its wait method > to be called so it can report it. The flag in cleared the "The flag is cleared in the ...." > async_event_handler's callback (remote_async_inferior_event_handler), > just before calling inferior_event_handler. However, since > inferior_event_handler may actually call another target's wait method, > we are not sure if the remote target may still have an event to report, > so we need check if we need to re-raise the flag after the > inferior_event_handler call. > > For various reasons, inferior_event_handler can lead to the remote > target getting closed and deleted. This can be because of an inferior > exit, or a serial communication failure. That leads to a > use-after-free: > > static void > remote_async_inferior_event_handler (async_event_handler *handler, > gdb_client_data data) > { > clear_async_event_handler (handler); > inferior_event_handler (INF_REG_EVENT); <== remote_target gets deleted > > remote_target *remote = (remote_target *) data; > remote_state *rs = remote->get_remote_state (); <== dereferencing the deleted object > > This is PR26614. To fix this, move the responsibility of clearing the > remote_async_inferior_event_token flag to remote_target::wait. Upon > return remote_target::wait should now leave the flag cleared or marked, > depending on whether it has subsequent event to report. > > In the case where remote_async_inferior_event_handler gets called but > infrun calls another target's wait method, the flag just naturally > stays marked because nothing touches it. > > Note that this logic is already partially implemented in > remote_target::wait, since the remote target may have multiple events to > report (and it can only report one at the time): > > if (target_is_async_p ()) > { > remote_state *rs = get_remote_state (); > > /* If there are are events left in the queue tell the event loop > to return here. */ > if (!rs->stop_reply_queue.empty ()) > mark_async_event_handler (rs->remote_async_inferior_event_token); > } > > However, the code in remote_async_inferior_event_handler checks for for > pending events in addition to whether the stop reply queue is empty, so > I've made remote_target::wait check for that as well. I'm not > completely sure this is ok, since I don't understand very well yet how > the pending events mechanism works. But I figured it was safer to do > this in order to avoid missing events, worst case it just leads to > unnecessary calls to remote_target::wait. > > gdb/ChangeLog: > > PR gdb/26614 > * remote.c (remote_target::wait): Clear async event handler at > beginning, mark if needed at the end. > (remote_async_inferior_event_handler): Don't set or clear async > event handler. > > Change-Id: I20117f5b5acc8a9972c90f16280249b766c1bf37 > --- > gdb/remote.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index 23c1bab0b27..30694e99b31 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8000,6 +8000,13 @@ ptid_t > remote_target::wait (ptid_t ptid, struct target_waitstatus *status, > target_wait_flags options) > { > + remote_state *rs = get_remote_state (); > + > + /* Start by clearing the flag that asks for our wait method to be called, > + we'll mark it again at the end if needed. */ > + if (target_is_async_p ()) > + clear_async_event_handler (rs->remote_async_inferior_event_token); > + > ptid_t event_ptid; > > if (target_is_non_stop_p ()) > @@ -8009,11 +8016,10 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status, > > if (target_is_async_p ()) > { > - remote_state *rs = get_remote_state (); > - > /* If there are are events left in the queue tell the event loop > to return here. */ It feels like this comment should be updated something like: /* If there are events left in the queue, or unacknowledged notifications, then tell the event loop to return here. */ > - if (!rs->stop_reply_queue.empty ()) > + if (!rs->stop_reply_queue.empty () > + || rs->notif_state->pending_event[notif_client_stop.id] != nullptr) > mark_async_event_handler (rs->remote_async_inferior_event_token); > } > > @@ -14174,21 +14180,7 @@ remote_async_serial_handler (struct serial *scb, void *context) > static void > remote_async_inferior_event_handler (gdb_client_data data) > { > - remote_target *remote = (remote_target *) data; > - remote_state *rs = remote->get_remote_state (); > - clear_async_event_handler (rs->remote_async_inferior_event_token); > - > inferior_event_handler (INF_REG_EVENT); > - > - /* inferior_event_handler may have consumed an event pending on the > - infrun side without calling target_wait on the REMOTE target, or > - may have pulled an event out of a different target. Keep trying > - for this remote target as long it still has either pending events > - or unacknowledged notifications. */ > - > - if (rs->notif_state->pending_event[notif_client_stop.id] != NULL > - || !rs->stop_reply_queue.empty ()) > - mark_async_event_handler (rs->remote_async_inferior_event_token); This code was all from commit 96118d114e3c5. If you check that commit for the file remote.c you'll see one extra hunk: diff --git a/gdb/remote.c b/gdb/remote.c index f7f99dc24fe..59075cb09f2 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -5605,7 +5605,7 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p) /* Register extra event sources in the event loop. */ rs->remote_async_inferior_event_token - = create_async_event_handler (remote_async_inferior_event_handler, NULL); + = create_async_event_handler (remote_async_inferior_event_handler, remote); rs->notif_state = remote_notif_state_allocate (remote); /* Reset the target state; these things will be queried either by I think this should be reverted as part of this commit too. Passing the remote through as the gdb_client_data parameter is bad (i.e. allows for use after free situations), so lets not do that. Otherwise, LGTM. Thanks, Andreq > } > > int > -- > 2.29.2 >