[rfa/gdbserver] Fix crash in thread_db_get_tls_address

[rfa/gdbserver] Fix crash in thread_db_get_tls_address

[Ulrich Weigand]

Hello,

when debugging remotely using a GDB with private modifcations, I'm running
into a crash in gdbserver, which I believe to be a real bug (even if latent
with mainline GDB).

The problem occurs when the thread_db_get_tls_address routine is invoked
(as a result of processing a qGetTLSAddr: query) on an inferior that
actually has no threads (or where the thread layer is not initialized yet).

This is caused by thread_db_get_tls_address calling find_one_thread,
which in the end calls down into the libthread_db td_ta_map_lwp2thr
routine -- at a time libthread_db is not yet initialized, and in fact
the "thread_agent" handle passed to td_ta_map_lwp2thr was not yet
set up.  This results in a segfault within libthread_db.

Now I guess it is debatable whether or not sending a qGetTLSAddr:
query in this situation is a useful thing, but it seems to me that
gdbserver shouldn't just *crash* ...

The following patch fixes this by returning failure from
thread_db_get_tls_address if called before the thread layer
is properly initialized.


Tested on powerpc64-linux (64-bit / 32-bit) using local gdbserver.

OK for mainline?

Bye,
Ulrich


ChangeLog:

	* thread-db.c (thread_db_get_tls_address): Do not crash if
	called when thread layer is not yet initialized.


Index: src/gdb/gdbserver/thread-db.c
===================================================================
--- src.orig/gdb/gdbserver/thread-db.c
+++ src/gdb/gdbserver/thread-db.c
@@ -388,6 +388,10 @@ thread_db_get_tls_address (struct thread
   td_err_e err;
   struct process_info *process;
 
+  /* If the thread layer is not (yet) initialized, fail.  */
+  if (!all_symbols_looked_up)
+    return -1;
+
   process = get_thread_process (thread);
   if (!process->thread_known)
     find_one_thread (process->lwpid);
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfa/gdbserver] Fix crash in thread_db_get_tls_address

[Doug Evans]

On Wed, Jan 21, 2009 at 2:57 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> when debugging remotely using a GDB with private modifcations, I'm running
> into a crash in gdbserver, which I believe to be a real bug (even if latent
> with mainline GDB).
>
> The problem occurs when the thread_db_get_tls_address routine is invoked
> (as a result of processing a qGetTLSAddr: query) on an inferior that
> actually has no threads (or where the thread layer is not initialized yet).
>
> This is caused by thread_db_get_tls_address calling find_one_thread,
> which in the end calls down into the libthread_db td_ta_map_lwp2thr
> routine -- at a time libthread_db is not yet initialized, and in fact
> the "thread_agent" handle passed to td_ta_map_lwp2thr was not yet
> set up.  This results in a segfault within libthread_db.
>
> Now I guess it is debatable whether or not sending a qGetTLSAddr:
> query in this situation is a useful thing, but it seems to me that
> gdbserver shouldn't just *crash* ...
>
> The following patch fixes this by returning failure from
> thread_db_get_tls_address if called before the thread layer
> is properly initialized.
>
>
> Tested on powerpc64-linux (64-bit / 32-bit) using local gdbserver.
>
> OK for mainline?
>
> Bye,
> Ulrich
>
>
> ChangeLog:
>
>        * thread-db.c (thread_db_get_tls_address): Do not crash if
>        called when thread layer is not yet initialized.
>
>
> Index: src/gdb/gdbserver/thread-db.c
> ===================================================================
> --- src.orig/gdb/gdbserver/thread-db.c
> +++ src/gdb/gdbserver/thread-db.c
> @@ -388,6 +388,10 @@ thread_db_get_tls_address (struct thread
>   td_err_e err;
>   struct process_info *process;
>
> +  /* If the thread layer is not (yet) initialized, fail.  */
> +  if (!all_symbols_looked_up)
> +    return -1;
> +
>   process = get_thread_process (thread);
>   if (!process->thread_known)
>     find_one_thread (process->lwpid);
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>

Hi.  I've run into similar situations with the thread layer not yet
initialized.  One aspect of this patch is a bit confusing.  Maybe a
comment is warranted.

Returning -1 will cause server.c:handle_query to mark the packet as
unknown which will in turn cause remote.c:packet_ok to mark the packet
as disabled (on the gdb side).  How does the packet get re-enabled if
the thread layer is later initialized?

Re: [rfa/gdbserver] Fix crash in thread_db_get_tls_address

[Ulrich Weigand]

Doug Evans wrote:

> Hi.  I've run into similar situations with the thread layer not yet
> initialized.  One aspect of this patch is a bit confusing.  Maybe a
> comment is warranted.
> 
> Returning -1 will cause server.c:handle_query to mark the packet as
> unknown which will in turn cause remote.c:packet_ok to mark the packet
> as disabled (on the gdb side).  How does the packet get re-enabled if
> the thread layer is later initialized?

You're right -- I missed that.  I guess we need to report an error
instead of marking the packet as unknown.

The following patch is changed to use TD_ERR ("generic error" seems to
be the best response -- I don't see a more specific code that would be
appropriate here).

Retested on powerpc64-linux (64-bit / 32-bit) with local gdbserver.

Bye,
Ulrich


ChangeLog:

	* thread-db.c (thread_db_get_tls_address): Do not crash if
	called when thread layer is not yet initialized.


Index: src/gdb/gdbserver/thread-db.c
===================================================================
--- src.orig/gdb/gdbserver/thread-db.c
+++ src/gdb/gdbserver/thread-db.c
@@ -388,6 +388,10 @@ thread_db_get_tls_address (struct thread
   td_err_e err;
   struct process_info *process;
 
+  /* If the thread layer is not (yet) initialized, fail.  */
+  if (!all_symbols_looked_up)
+    return TD_ERR;
+
   process = get_thread_process (thread);
   if (!process->thread_known)
     find_one_thread (process->lwpid);


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfa/gdbserver] Fix crash in thread_db_get_tls_address

[Doug Evans]

On Thu, Jan 22, 2009 at 7:05 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Doug Evans wrote:
>
>> Hi.  I've run into similar situations with the thread layer not yet
>> initialized.  One aspect of this patch is a bit confusing.  Maybe a
>> comment is warranted.
>>
>> Returning -1 will cause server.c:handle_query to mark the packet as
>> unknown which will in turn cause remote.c:packet_ok to mark the packet
>> as disabled (on the gdb side).  How does the packet get re-enabled if
>> the thread layer is later initialized?
>
> You're right -- I missed that.  I guess we need to report an error
> instead of marking the packet as unknown.
>
> The following patch is changed to use TD_ERR ("generic error" seems to
> be the best response -- I don't see a more specific code that would be
> appropriate here).
>
> Retested on powerpc64-linux (64-bit / 32-bit) with local gdbserver.

I don't know if there's a better value to use here either.  Maybe
TD_TLSDEFER, but I'm just guessing (and I don't know how portable it
is).

>
> Bye,
> Ulrich
>
>
> ChangeLog:
>
>        * thread-db.c (thread_db_get_tls_address): Do not crash if
>        called when thread layer is not yet initialized.
>
>
> Index: src/gdb/gdbserver/thread-db.c
> ===================================================================
> --- src.orig/gdb/gdbserver/thread-db.c
> +++ src/gdb/gdbserver/thread-db.c
> @@ -388,6 +388,10 @@ thread_db_get_tls_address (struct thread
>   td_err_e err;
>   struct process_info *process;
>
> +  /* If the thread layer is not (yet) initialized, fail.  */
> +  if (!all_symbols_looked_up)
> +    return TD_ERR;
> +
>   process = get_thread_process (thread);
>   if (!process->thread_known)
>     find_one_thread (process->lwpid);
>
>
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>

[rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Ulrich Weigand]

Doug Evans wrote:
> On Thu, Jan 22, 2009 at 7:05 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > Doug Evans wrote:
> >
> >> Hi.  I've run into similar situations with the thread layer not yet
> >> initialized.  One aspect of this patch is a bit confusing.  Maybe a
> >> comment is warranted.
> >>
> >> Returning -1 will cause server.c:handle_query to mark the packet as
> >> unknown which will in turn cause remote.c:packet_ok to mark the packet
> >> as disabled (on the gdb side).  How does the packet get re-enabled if
> >> the thread layer is later initialized?
> >
> > You're right -- I missed that.  I guess we need to report an error
> > instead of marking the packet as unknown.
> >
> > The following patch is changed to use TD_ERR ("generic error" seems to
> > be the best response -- I don't see a more specific code that would be
> > appropriate here).
> >
> > Retested on powerpc64-linux (64-bit / 32-bit) with local gdbserver.
> 
> I don't know if there's a better value to use here either.  Maybe
> TD_TLSDEFER, but I'm just guessing (and I don't know how portable it
> is).

TD_TLSDEFER doesn't seem to be available everywhere, and has a somewhat
different meaning, I think.  In any case, it doesn't really matter, as GDB
will currently throw a TLS_GENERIC_ERROR in remote.c no matter what error
code is returned ...

I've updated the patch to account for multi-process changes.
Retested on powerpc64-linux (64-bit / 32-bit) with local gdbserver.

Dan, is this OK for mainline?

Bye,
Ulrich


ChangeLog:

	* thread-db.c (thread_db_get_tls_address): Do not crash if
	called when thread layer is not yet initialized.


Index: src/gdb/gdbserver/thread-db.c
===================================================================
--- src.orig/gdb/gdbserver/thread-db.c
+++ src/gdb/gdbserver/thread-db.c
@@ -382,6 +382,10 @@ thread_db_get_tls_address (struct thread
   struct lwp_info *lwp;
   struct thread_info *saved_inferior;
 
+  /* If the thread layer is not (yet) initialized, fail.  */
+  if (!current_process()->all_symbols_looked_up)
+    return TD_ERR;
+
   lwp = get_thread_lwp (thread);
   if (!lwp->thread_known)
     find_one_thread (lwp->head.id);


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Pedro Alves]

On Friday 03 April 2009 19:06:55, Ulrich Weigand wrote:
> Index: src/gdb/gdbserver/thread-db.c
> ===================================================================
> --- src.orig/gdb/gdbserver/thread-db.c
> +++ src/gdb/gdbserver/thread-db.c
> @@ -382,6 +382,10 @@ thread_db_get_tls_address (struct thread
>    struct lwp_info *lwp;
>    struct thread_info *saved_inferior;
>  
> +  /* If the thread layer is not (yet) initialized, fail.  */
> +  if (!current_process()->all_symbols_looked_up)
> +    return TD_ERR;
> +

(note the missing space after current_process)

The qGetTLSAddr packet takes an explicit thread id, so in this
case, it may be that the current process isn't the
correct one.  I think in this case the best would be to
inferior.c:get_thread_process and use that, like:

  if (!get_thread_process (thread)->all_symbols_looked_up)
    return TD_ERR;

Alternatively you could make sure you call current_process (),
after temporarily   having switched the current inferior, like
we do a bit below.

Sorry for the extra work...

-- 
Pedro Alves

Re: [rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Ulrich Weigand]

Pedro Alves wrote:

> The qGetTLSAddr packet takes an explicit thread id, so in this
> case, it may be that the current process isn't the
> correct one.  I think in this case the best would be to
> inferior.c:get_thread_process and use that, like:
> 
>   if (!get_thread_process (thread)->all_symbols_looked_up)
>     return TD_ERR;

That function is currently static to inferior.c; I guess it
should be exported?
 
> Alternatively you could make sure you call current_process (),
> after temporarily   having switched the current inferior, like
> we do a bit below.

Hmm, I want to guard against find_one_thread blowing up due to
a NULL proc->thread_agent -- but "proc" is always refering to
current_process () as well.  This is probably incorrect too,
and find_one_thread ought to use get_thread_process?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Pedro Alves]

A Friday 03 April 2009 20:20:09, Ulrich Weigand escreveu:
> Pedro Alves wrote:
> 
> > The qGetTLSAddr packet takes an explicit thread id, so in this
> > case, it may be that the current process isn't the
> > correct one.  I think in this case the best would be to
> > inferior.c:get_thread_process and use that, like:

   ^ export

> > 
> >   if (!get_thread_process (thread)->all_symbols_looked_up)
> >     return TD_ERR;
> 
> That function is currently static to inferior.c; I guess it
> should be exported?

Yes.  That's what I meant, but fingers slipped.  :-)

> > Alternatively you could make sure you call current_process (),
> > after temporarily   having switched the current inferior, like
> > we do a bit below.
> 
> Hmm, I want to guard against find_one_thread blowing up due to
> a NULL proc->thread_agent -- but "proc" is always refering to
> current_process () as well.  This is probably incorrect too,
> and find_one_thread ought to use get_thread_process?

Indeed.

-- 
Pedro Alves

Re: [rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Ulrich Weigand]

Pedro Alves wrote:
> A Friday 03 April 2009 20:20:09, Ulrich Weigand escreveu:
> > Pedro Alves wrote:
> > 
> > > The qGetTLSAddr packet takes an explicit thread id, so in this
> > > case, it may be that the current process isn't the
> > > correct one.  I think in this case the best would be to
> > > inferior.c:get_thread_process and use that, like:
> 
>    ^ export
> 
> > > 
> > >   if (!get_thread_process (thread)->all_symbols_looked_up)
> > >     return TD_ERR;
> > 
> > That function is currently static to inferior.c; I guess it
> > should be exported?
> 
> Yes.  That's what I meant, but fingers slipped.  :-)
> 
> > > Alternatively you could make sure you call current_process (),
> > > after temporarily   having switched the current inferior, like
> > > we do a bit below.
> > 
> > Hmm, I want to guard against find_one_thread blowing up due to
> > a NULL proc->thread_agent -- but "proc" is always refering to
> > current_process () as well.  This is probably incorrect too,
> > and find_one_thread ought to use get_thread_process?
> 
> Indeed.

OK, here's the version I've just committed.  Retested on powerpc64-linux
(64-bit and 32-bit) with local gdbserver.

Thanks,
Ulrich

ChangeLog:

	* inferiors.c (get_thread_process): Make global.
	* server.h (get_thread_process): Add prototype.
	* thread-db.c (find_one_thread): Use get_thread_process
	instead of current_process.
	(thread_db_get_tls_address): Do not crash if called when
	thread layer is not yet initialized.


Index: gdb/gdbserver/inferiors.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/inferiors.c,v
retrieving revision 1.19
diff -u -p -r1.19 inferiors.c
--- gdb/gdbserver/inferiors.c	1 Apr 2009 22:50:24 -0000	1.19
+++ gdb/gdbserver/inferiors.c	3 Apr 2009 19:47:11 -0000
@@ -442,7 +442,7 @@ find_process_pid (int pid)
     find_inferior_id (&all_processes, pid_to_ptid (pid));
 }
 
-static struct process_info *
+struct process_info *
 get_thread_process (struct thread_info *thread)
 {
   int pid = ptid_get_pid (thread->entry.id);
Index: gdb/gdbserver/server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.55
diff -u -p -r1.55 server.h
--- gdb/gdbserver/server.h	1 Apr 2009 22:50:24 -0000	1.55
+++ gdb/gdbserver/server.h	3 Apr 2009 19:47:11 -0000
@@ -201,6 +201,7 @@ struct process_info
    no current thread selected.  */
 
 struct process_info *current_process (void);
+struct process_info *get_thread_process (struct thread_info *);
 
 #include "regcache.h"
 #include "gdb/signals.h"
Index: gdb/gdbserver/thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/thread-db.c,v
retrieving revision 1.22
diff -u -p -r1.22 thread-db.c
--- gdb/gdbserver/thread-db.c	1 Apr 2009 22:50:24 -0000	1.22
+++ gdb/gdbserver/thread-db.c	3 Apr 2009 19:47:11 -0000
@@ -233,7 +233,7 @@ find_one_thread (ptid_t ptid)
   td_err_e err;
   struct thread_info *inferior;
   struct lwp_info *lwp;
-  struct process_info_private *proc = current_process()->private;
+  struct process_info_private *proc;
   int lwpid = ptid_get_lwp (ptid);
 
   inferior = (struct thread_info *) find_inferior_id (&all_threads, ptid);
@@ -242,6 +242,7 @@ find_one_thread (ptid_t ptid)
     return 1;
 
   /* Get information about this thread.  */
+  proc = get_thread_process (inferior)->private;
   err = td_ta_map_lwp2thr (proc->thread_agent, lwpid, &th);
   if (err != TD_OK)
     error ("Cannot get thread handle for LWP %d: %s",
@@ -382,6 +383,10 @@ thread_db_get_tls_address (struct thread
   struct lwp_info *lwp;
   struct thread_info *saved_inferior;
 
+  /* If the thread layer is not (yet) initialized, fail.  */
+  if (!get_thread_process (thread)->all_symbols_looked_up)
+    return TD_ERR;
+
   lwp = get_thread_lwp (thread);
   if (!lwp->thread_known)
     find_one_thread (lwp->head.id);


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfa/gdbserver] Updated: Fix crash in thread_db_get_tls_address

[Daniel Jacobowitz]

On Fri, Apr 03, 2009 at 08:06:55PM +0200, Ulrich Weigand wrote:
> Dan, is this OK for mainline?

Yes, this looks fine to me.

-- 
Daniel Jacobowitz
CodeSourcery