Re: [PATCH 08/31] Thread options & clone events (core + remote)

[Lancelot SIX <lsix@lancelotsix.com>]

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)
>  {