From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id wGZmDt8I2WPUNiYAWB0awg (envelope-from ) for ; Tue, 31 Jan 2023 07:26:07 -0500 Received: by simark.ca (Postfix, from userid 112) id 372E21E128; Tue, 31 Jan 2023 07:26:07 -0500 (EST) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=Nmv58cyk; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 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 71F471E110 for ; Tue, 31 Jan 2023 07:26:03 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 26D98385700E for ; Tue, 31 Jan 2023 12:26:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 26D98385700E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675167961; bh=E6EMPsg+Lii+a1utwIzT8l91Lsv7KprT7hUHzchx95I=; h=Date:To:Cc:Subject:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Nmv58cykQgj1W3tBQCJyONF9E7XhPcC8c6n8CcRtYrheWCQ8v/f5JvI9t0fs5PYhB I2BNzyUEUFD6Zexz4u3lIE94SzDzoFhONcMWYZV20TsvLWG62W1b1fhuO0QHHEhKM9 E8cGG976bihnJkc3P4LknqM9U7pL1M+4DaqpJeYk= Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 87C2F3858D33 for ; Tue, 31 Jan 2023 12:25:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 87C2F3858D33 Received: from ubuntu.lan (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 04EB580E9E; Tue, 31 Jan 2023 12:25:37 +0000 (UTC) Date: Tue, 31 Jan 2023 12:25:03 +0000 To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 08/31] Thread options & clone events (core + remote) Message-ID: <20230131122503.x6aameca5zjuu7kp@ubuntu.lan> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-9-pedro@palves.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20221212203101.1034916-9-pedro@palves.net> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 31 Jan 2023 12:25:38 +0000 (UTC) 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: Lancelot SIX via Gdb-patches Reply-To: Lancelot SIX Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi, > diff --git a/gdb/remote.c b/gdb/remote.c > index 41348a65dc4..9de8ed8a068 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -14534,6 +14601,77 @@ remote_target::thread_events (int enable) > } > } > > +/* Implementation of the supports_set_thread_options target > + method. */ > + > +bool > +remote_target::supports_set_thread_options (gdb_thread_options options) > +{ > + remote_state *rs = get_remote_state (); > + return (packet_support (PACKET_QThreadOptions) == PACKET_ENABLE > + && (rs->supported_thread_options & options) == options); > +} > + > +/* For coalescing reasons, actually sending the options to the target > + happens at resume time, via this function. See target_resume for > + all-stop, and target_commit_resumed for non-stop. */ > + > +void > +remote_target::commit_requested_thread_options () > +{ > + struct remote_state *rs = get_remote_state (); > + > + if (packet_support (PACKET_QThreadOptions) != PACKET_ENABLE) > + return; > + > + char *p = rs->buf.data (); > + char *endp = p + get_remote_packet_size (); > + > + /* Clear options for all threads by default. Note that unlike > + vCont, the rightmost options that match a thread apply, so we > + don't have to worry about whether we can use wildcard ptids. */ > + strcpy (p, "QThreadOptions;0"); > + p += strlen (p); > + > + /* Now set non-zero options for threads that need them. We don't > + bother with the case of all threads of a process wanting the same > + non-zero options as that's not an expected scenario. */ > + for (thread_info *tp : all_non_exited_threads (this)) > + { > + gdb_thread_options options = tp->thread_options (); > + > + if (options == 0) > + continue; > + > + *p++ = ';'; > + p += xsnprintf (p, endp - p, "%s", phex_nz (options, sizeof (options))); I am not super familiar with how big the buffer is guaranteed to be. Can we imagine a situation where the number of thread and options to send exceed the packet size capacity? If so, this seems dangerous. 'p' would be incremented by the size which would have been necessary to do the print, so it means it could now point past the end of the buffer. Even the `*p++'= ';'` above and similar `*p++ =` below are subject to overflow if the number of options to encode grow too high. See man vsnprintf(3) which is used by xsnprintf: The functions snprintf() and vsnprintf() do not write more than size bytes[...]. If the output was truncated due to this limit, then the return value is the number of characters [...] which would have been written to the final string if enough space had been available. As I do not feel that we can have a guaranty regarding the maximum number of non exited threads with non-0 options (I might be wrong, but the set of options can be extended so this can show in the future), I would check the returned value of xsnprintf before adding it to p (the same might apply to remote_target::write_ptid, and other increments to p). Did I miss some precondition which guarantee the buffer to be big enough? Best, Lancelot. > + if (tp->ptid != magic_null_ptid) > + { > + *p++ = ':'; > + p = write_ptid (p, endp, tp->ptid); > + } > + } > + > + *p++ = '\0'; > + > + putpkt (rs->buf); > + getpkt (&rs->buf, 0); > + > + switch (packet_ok (rs->buf, > + &remote_protocol_packets[PACKET_QThreadOptions])) > + { > + case PACKET_OK: > + if (strcmp (rs->buf.data (), "OK") != 0) > + error (_("Remote refused setting thread options: %s"), rs->buf.data ()); > + break; > + case PACKET_ERROR: > + error (_("Remote failure reply: %s"), rs->buf.data ()); > + case PACKET_UNKNOWN: > + gdb_assert_not_reached ("PACKET_UNKNOWN"); > + break; > + } > +} > + > static void > show_remote_cmd (const char *args, int from_tty) > {