Re: [PATCH] gdbserver: don't pick a random thread if the current thread dies

[Pedro Alves <palves@redhat.com>]

On 08/06/2015 03:44 PM, Pedro Alves wrote:
> In all-stop mode, if the current thread disappears while stopping all
> threads, gdbserver calls set_desired_thread(0) ['0' means "I want the
> continue thread"] which just picks the first thread in the list.
> 
> This looks like a dangerous thing to do.  GDBserver continues
> processing whatever it was doing, but to the wrong thread.  If
> debugging more than one process, we may even pick the wrong process.
> Instead, GDBserver should detect the situation and bail out of
> whatever is was doing.
> 
> The backends used to pay attention to the set 'cont_thread' (the Hc
> thread, used in the old way to resume threads, before vCont), but all
> such 'cont_thread' checks have been eliminated meanwhile.  The
> remaining implicit dependencies that I found on there being a selected
> thread in the backends are in the Ctrl-C handling, which some backends
> use as thread to send a signal to.  Even that seems to me to be better
> handled by always using the first thread in the list or by using the
> signal_pid PID.
> 
> In order to make this a systematic approach, I'm making
> set_desired_thread never fallback to a random thread, and instead end
> up with current_thread == NULL, like already done in non-stop mode.
> Then I updated all callers to handle the situation.
> 
> I stumbled on this while fixing other bugs exposed by
> gdb.threads/fork-plus-threads.exp test.  The problems I saw were fixed
> in a different way, but in any case, I think the potential for
> problems is more or less obvious, and the resulting code looks a bit
> less magical to me.
> 
> Tested on x86-64 Fedora 20, w/ native-extended-gdbserver board.
> 
> gdb/gdbserver/ChangeLog:
> 2015-08-06  Pedro Alves  <palves@redhat.com>
> 
> 	* linux-low.c (wait_for_sigstop): Always switch to no thread
> 	selected if the previously current thread dies.
> 	* lynx-low.c (lynx_request_interrupt): Use the first thread's
> 	process instead of the current thread's.
> 	* remote-utils.c (input_interrupt): Don't check if there's no
> 	current thread.
> 	* server.c (gdb_read_memory, gdb_write_memory): If setting the
> 	current thread to the general thread fails, error out.
> 	(handle_qxfer_auxv, handle_qxfer_libraries)
> 	(handle_qxfer_libraries_svr4, handle_qxfer_siginfo)
> 	(handle_qxfer_spu, handle_qxfer_statictrace, handle_qxfer_fdpic)
> 	(handle_query): Check if there's a thread selected instead of
> 	checking whether there's any thread in the thread list.
> 	(handle_qxfer_threads, handle_qxfer_btrace)
> 	(handle_qxfer_btrace_conf): Don't error out early if there's no
> 	thread in the thread list.
> 	(handle_v_cont, myresume): Don't set the current thread to the
> 	continue thread.
> 	(process_serial_event) <Hg handling>: Also set thread_id if the
> 	previous general thread is still alive.
> 	(process_serial_event) <g/G handling>: If setting the current
> 	thread to the general thread fails, error out.
> 	* spu-low.c (spu_resume, spu_request_interrupt): Use the first
> 	thread's lwp instead of the current thread's.
> 	* target.c (set_desired_thread): If the desired thread was not
> 	found, leave the current thread pointing to NULL.  Return an int
> 	(boolean) indicating success.
> 	* target.h (set_desired_thread): Change return type to int.

I pushed this in.

Thanks,
Pedro Alves