From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com 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) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1675167938; bh=LteEn9hxfcrXG/5xpdUGPXuEZftpc4gkLmBfIQwDHyc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MjrWQW10790Je46tUoODwUuSuFaLg4aDUa5HfAkPEHFvL+mvFriQXFvaz9L/9tzPj BonSLU/XPJi8CKNOxVaWtX7K0LSGEKlFQQAczUfH5yzPSMOlMEQrcQAIOu9BsGixmQ ZRs1FXXMiqvfOX+iJHgZzuFFCx8MWEqUW97FnkT/6rBiP5dKEaZ6cUeOotX50PS8d4 P1G6OrkrjbssKuoIJlnLKT/MZfOdtIvZ7k6AYX3QThqosb1IoGM/9Hita0zxD1N6Th 3fZ/1cjCmhUrEWbLSg124YH/70JnUzGn70SSfAirgZP3P5j9jeWHi4LwSAvmgtPPTi c3415fHzag7PQ== Date: Tue, 31 Jan 2023 12:25:03 +0000 From: Lancelot SIX 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-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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) > {