[RFC,04/15] Fix crash in tstatus after detach

  From: Par Olsson <par.olsson@windriver.com>

When calling tstatus after detaching the process,
gdbserver tries to access inferior memory which
results in a crash.
This changes the behavior of the agent_loaded_p()
to return false if no inferior is loaded.

gdb/ChangeLog:

	* agent.c (agent_loaded_p): Add check that inferior is present.

Signed-off-by: Par Olsson <par.olsson@windriver.com>
Signed-off-by: Henrik Wallin <henrik.wallin@windriver.com>
---
 gdb/common/agent.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

Hi Henrik,

henrik.wallin@windriver.com wrote:
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 5c307290589d..c9b6c41bc4ff 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -73,9 +73,16 @@ static struct ipa_sym_addresses ipa_sym_addrs;
>  
>  static int all_agent_symbols_looked_up = 0;
>  
> +#ifdef GDBSERVER
> +#include <inferiors.h>
> +#endif
>  int
>  agent_loaded_p (void)
>  {
> +#ifdef GDBSERVER
> +  if (current_thread == NULL)
> +    return 0;
> +#endif
>    return all_agent_symbols_looked_up;
>  }
>  

Please don't introduce "#ifdef GDBSERVER" conditionals into common
code, I spent some time removing them last year.  I know I didn't
get them all, but the remaining two are on my hit list :)

Thanks,
Gary
  

On 10/14/2015 12:14 PM, henrik.wallin@windriver.com wrote:
> From: Par Olsson <par.olsson@windriver.com>
> 
> When calling tstatus after detaching the process,
> gdbserver tries to access inferior memory which
> results in a crash.
> This changes the behavior of the agent_loaded_p()
> to return false if no inferior is loaded.

Sounds like it should be easy to cook up a testcase.
Could you do that?

> 
> gdb/ChangeLog:
> 
> 	* agent.c (agent_loaded_p): Add check that inferior is present.
> 
> Signed-off-by: Par Olsson <par.olsson@windriver.com>
> Signed-off-by: Henrik Wallin <henrik.wallin@windriver.com>
> ---
>  gdb/common/agent.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 5c307290589d..c9b6c41bc4ff 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -73,9 +73,16 @@ static struct ipa_sym_addresses ipa_sym_addrs;
>  
>  static int all_agent_symbols_looked_up = 0;
>  
> +#ifdef GDBSERVER
> +#include <inferiors.h>
> +#endif
>  int
>  agent_loaded_p (void)
>  {
> +#ifdef GDBSERVER
> +  if (current_thread == NULL)
> +    return 0;
> +#endif
>    return all_agent_symbols_looked_up;
>  }
>  

Should probably be moved up to the caller.

Thanks,
Pedro Alves
  

2015-10-15 19:22 GMT+02:00 Pedro Alves <palves@redhat.com>:
> On 10/14/2015 12:14 PM, henrik.wallin@windriver.com wrote:
>> From: Par Olsson <par.olsson@windriver.com>
>>
>> When calling tstatus after detaching the process,
>> gdbserver tries to access inferior memory which
>> results in a crash.
>> This changes the behavior of the agent_loaded_p()
>> to return false if no inferior is loaded.
>
> Sounds like it should be easy to cook up a testcase.
> Could you do that?

I will do, after I manage to actually reproduce this one...
The patch was done on 7.6 and then rebased. I will check some more but it looks like this might've been fixed by some other changes between 7.6 and master.

>
>>
>> gdb/ChangeLog:
>>
>>       * agent.c (agent_loaded_p): Add check that inferior is present.
>>
>> Signed-off-by: Par Olsson <par.olsson@windriver.com>
>> Signed-off-by: Henrik Wallin <henrik.wallin@windriver.com>
>> ---
>>  gdb/common/agent.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
>> index 5c307290589d..c9b6c41bc4ff 100644
>> --- a/gdb/common/agent.c
>> +++ b/gdb/common/agent.c
>> @@ -73,9 +73,16 @@ static struct ipa_sym_addresses ipa_sym_addrs;
>>
>>  static int all_agent_symbols_looked_up = 0;
>>
>> +#ifdef GDBSERVER
>> +#include <inferiors.h>
>> +#endif
>>  int
>>  agent_loaded_p (void)
>>  {
>> +#ifdef GDBSERVER
>> +  if (current_thread == NULL)
>> +    return 0;
>> +#endif
>>    return all_agent_symbols_looked_up;
>>  }
>>
>
> Should probably be moved up to the caller.

I will check that.

thanks,
/ Henrik
  

2015-10-14 13:48 GMT+02:00 Gary Benson <gbenson@redhat.com>:
> Hi Henrik,
>
> henrik.wallin@windriver.com wrote:
>> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
>> index 5c307290589d..c9b6c41bc4ff 100644
>> --- a/gdb/common/agent.c
>> +++ b/gdb/common/agent.c
>> @@ -73,9 +73,16 @@ static struct ipa_sym_addresses ipa_sym_addrs;
>>
>>  static int all_agent_symbols_looked_up = 0;
>>
>> +#ifdef GDBSERVER
>> +#include <inferiors.h>
>> +#endif
>>  int
>>  agent_loaded_p (void)
>>  {
>> +#ifdef GDBSERVER
>> +  if (current_thread == NULL)
>> +    return 0;
>> +#endif
>>    return all_agent_symbols_looked_up;
>>  }
>>
>
> Please don't introduce "#ifdef GDBSERVER" conditionals into common
> code, I spent some time removing them last year.  I know I didn't
> get them all, but the remaining two are on my hit list :)

Ok :)
I will check that.

thanks,
/ Henrik
  

Wallin, Henrik wrote:
> 2015-10-14 13:48 GMT+02:00 Gary Benson <gbenson@redhat.com>:
> > henrik.wallin@windriver.com wrote:
> > > diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> > > index 5c307290589d..c9b6c41bc4ff 100644
> > > --- a/gdb/common/agent.c
> > > +++ b/gdb/common/agent.c
> > > @@ -73,9 +73,16 @@ static struct ipa_sym_addresses ipa_sym_addrs;
> > >
> > >  static int all_agent_symbols_looked_up = 0;
> > >
> > > +#ifdef GDBSERVER
> > > +#include <inferiors.h>
> > > +#endif
> > >  int
> > >  agent_loaded_p (void)
> > >  {
> > > +#ifdef GDBSERVER
> > > +  if (current_thread == NULL)
> > > +    return 0;
> > > +#endif
> > >    return all_agent_symbols_looked_up;
> > >  }
> > >
> >
> > Please don't introduce "#ifdef GDBSERVER" conditionals into common
> > code, I spent some time removing them last year.  I know I didn't
> > get them all, but the remaining two are on my hit list :)
> 
> Ok :)
> I will check that.

Thanks Henrik.

Cheers,
Gary