gdbserver: Reset current_thread when its process is removed.

Message ID 1444919808-22088-1-git-send-email-aristovski@qnx.com
State New, archived
Headers

Commit Message

Aleksandar Ristovski Oct. 15, 2015, 2:36 p.m. UTC
  In case of running gdbserver with --multi, if a nat file
removes a process, current_thread may remain set to now
freed 'process' entry.

This may lead to wrong operation or a crash.

gdb/gdbserver/ChangeLog:
	* inferiors.c (remove_process): Reset current_thread if its
	associated process gets removed.
---
 gdb/gdbserver/inferiors.c | 5 +++++
 1 file changed, 5 insertions(+)
  

Comments

Pedro Alves Oct. 15, 2015, 4:22 p.m. UTC | #1
On 10/15/2015 03:36 PM, Aleksandar Ristovski wrote:
> In case of running gdbserver with --multi, if a nat file
> removes a process, current_thread may remain set to now
> freed 'process' entry.
> 
> This may lead to wrong operation or a crash.
> 
> gdb/gdbserver/ChangeLog:
> 	* inferiors.c (remove_process): Reset current_thread if its
> 	associated process gets removed.
> ---
>  gdb/gdbserver/inferiors.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index 21f45fa..4688a44 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -291,6 +291,11 @@ remove_process (struct process_info *process)
>  {
>    clear_symbol_cache (&process->symbol_cache);
>    free_all_breakpoints (process);
> +  if (current_thread && get_thread_process (current_thread) == process)
> +    {
> +      remove_thread (current_thread);
> +      current_thread = NULL;
> +    }

This seems very papering-over-something-else.

- I could argue that current_thread = NULL would be better done
inside remove_thread than here, since what you say would happen
as well if the current thread is removed, even without removing
the process.

- And then, if we remove the current thread, why not remove others?

AFAICS, other targets remove threads from within target_mourn, as that
way they have a chance of clearing auxiliary info associated with
the threads (the inferior_target_data()), and then some call
clear_inferiors, which also clears current_thread.

>    remove_inferior (&all_processes, &process->entry);
>    free (process);
>  }
> 

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 21f45fa..4688a44 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -291,6 +291,11 @@  remove_process (struct process_info *process)
 {
   clear_symbol_cache (&process->symbol_cache);
   free_all_breakpoints (process);
+  if (current_thread && get_thread_process (current_thread) == process)
+    {
+      remove_thread (current_thread);
+      current_thread = NULL;
+    }
   remove_inferior (&all_processes, &process->entry);
   free (process);
 }