From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0JwXOmojxV+3LAAAWB0awg (envelope-from ) for ; Mon, 30 Nov 2020 11:52:58 -0500 Received: by simark.ca (Postfix, from userid 112) id E733C1F0AC; Mon, 30 Nov 2020 11:52:58 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 8790B1E552 for ; Mon, 30 Nov 2020 11:52:58 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B8DB43898029; Mon, 30 Nov 2020 16:52:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B8DB43898029 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606755176; bh=7RLQ3HlCRKgJkdRx0d0hsxqWjBE1Ju521Feby9wi8Kg=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=UF9tLuUjJNWp1fxAygyl+i75sGfuWZk2M55p68wKKkmGRnattxEjfELuUUs097cmV h+/c4H5tZjvpoy89BNelfdUM9szLgYQrI+baZvghAAkdhE4H7TGcaHagj+a4b7p+rI Un3FqPcJn+S+a8rrOkwCCaUhWnXOw9UmBnp48l94= Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 90A9F3896C30 for ; Mon, 30 Nov 2020 16:52:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 90A9F3896C30 X-ASG-Debug-ID: 1606755172-0c856e6cd5295e90001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id qswUnB8DmyB9JJkl (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 30 Nov 2020 11:52:52 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@efficios.com X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from epycamd.internal.efficios.com (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) by smtp.ebox.ca (Postfix) with ESMTP id 8DE8E441B21; Mon, 30 Nov 2020 11:52:52 -0500 (EST) X-Barracuda-RBL-IP: 192.222.181.218 X-Barracuda-Effective-Source-IP: 192-222-181-218.qc.cable.ebox.net[192.222.181.218] X-Barracuda-Apparent-Source-IP: 192.222.181.218 To: gdb-patches@sourceware.org Subject: [PATCH 2/4] gdb: make remote target clear its async event handler in wait Date: Mon, 30 Nov 2020 11:52:49 -0500 X-ASG-Orig-Subj: [PATCH 2/4] gdb: make remote target clear its async event handler in wait Message-Id: <20201130165251.830482-3-simon.marchi@efficios.com> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20201130165251.830482-1-simon.marchi@efficios.com> References: <20201130165251.830482-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1606755172 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 5055 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.50 X-Barracuda-Spam-Status: No, SCORE=0.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.86230 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M 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: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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 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. */ - 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); } int -- 2.29.2