From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 6+ChBJPT5F+MPgAAWB0awg (envelope-from ) for ; Thu, 24 Dec 2020 12:44:51 -0500 Received: by simark.ca (Postfix, from userid 112) id 031111F0AA; Thu, 24 Dec 2020 12:44:51 -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 3DAE51E99A for ; Thu, 24 Dec 2020 12:44:50 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BC0F73938399; Thu, 24 Dec 2020 17:44:49 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BC0F73938399 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1608831889; bh=I21YQ4YrJnOIEm4X58e+gdl6vrlyYgTnqoezi2DEtRk=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=yz340EZbVXcJUyf0VDXf8lbbVQ/K2bPtD3ztPRUpv/6lbq5khxMU80xu1ePeP+0LS OdYzQLru54k0prRJWSH61D8DswzldpaWNX7NXElYfTrlhukBhJOAdM9QsVxITlrT71 o+jwbocJUrVnkdufDPyZZvt8GpOYcwtWqwldOj7E= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 2D83B3938398 for ; Thu, 24 Dec 2020 17:44:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2D83B3938398 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 0BOHieOt010805 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 24 Dec 2020 12:44:44 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 0BOHieOt010805 Received: from [10.0.0.213] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 176411E99A; Thu, 24 Dec 2020 12:44:40 -0500 (EST) Subject: Re: [PATCH 2/4] gdb: make remote target clear its async event handler in wait To: Andrew Burgess , Simon Marchi References: <20201130165251.830482-1-simon.marchi@efficios.com> <20201130165251.830482-3-simon.marchi@efficios.com> <20201224172318.GP2945@embecosm.com> Message-ID: <214e12e4-14cf-9bf6-f54c-128843eb3126@polymtl.ca> Date: Thu, 24 Dec 2020 12:44:39 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20201224172318.GP2945@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 24 Dec 2020 17:44:40 +0000 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: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-12-24 12:23 p.m., Andrew Burgess wrote: > * 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 ...." Fixed. >> @@ -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. */ Done. I also changed "to return here" to "to call us again", because I thought "to return here" was not very clear. > >> - 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. I don't think this is specifically what lead to the use-after-free, it was more that we referred to the remote target after calling fetch_inferior_event. But I agree that since it's now unused, we should not pass it anymore, so fixed. > Otherwise, LGTM. Thanks, I'll wait a bit to see if Pedro also has something to say about it. Simon