[v2] Fix logic in exec_file_locate_attach

  Hi all,

This is an updated version of the patch I posted yesterday.
It fails silently rather than throwing if the executable is
not in the sysroot, which both fixes the sysroot-escape issue
and results in a better GDB session for the user.

Built and regtested on RHEL 6.6 x86_64.

Luis, I think this patch will fix your connection drop without
any further changes.  Could you test it please?

Thanks,
Gary

---
This commit fixes an error in exec_file_locate_attach where
the main executable could be loaded from outside the sysroot
if a nonempty, non-"target:" sysroot was set but the discovered
executable filename did not exist in that sysroot and did exist
on the main filesystem.

gdb/ChangeLog:

	* exec.c (exec_file_locate_attach): Do not attempt to
	locate main executable locally if not found in sysroot.

gdb/testsuite/ChangeLog:

	* gdb.base/attach-pie-noexec.exp: Do not expect an error
	message on attach.
---
 gdb/ChangeLog                                |    5 +++++
 gdb/exec.c                                   |    9 ++++++---
 gdb/testsuite/ChangeLog                      |    5 +++++
 gdb/testsuite/gdb.base/attach-pie-noexec.exp |    2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)
  

Gary,

On 02/19/2016 09:21 AM, Gary Benson wrote:
> Hi all,
>
> This is an updated version of the patch I posted yesterday.
> It fails silently rather than throwing if the executable is
> not in the sysroot, which both fixes the sysroot-escape issue
> and results in a better GDB session for the user.
>
> Built and regtested on RHEL 6.6 x86_64.
>
> Luis, I think this patch will fix your connection drop without
> any further changes.  Could you test it please?

Unfortunately it doesn't completely solve the problem i saw, as
exec_file_find will still potentially throw errors and will disrupt the 
connection attempt or stop execution of a custom sequence of commands 
(as Pedro noted) when "attach" is part of the sequence.

define foo
attach <pid>
 >>>> execution stops here if an error is thrown
info threads
info registers
end

It still looks like a TRY/CATCH block is needed around at least 
exec_file_find.
  

Luis Machado wrote:
> On 02/19/2016 09:21 AM, Gary Benson wrote:
> > This is an updated version of the patch I posted yesterday.
> > It fails silently rather than throwing if the executable is
> > not in the sysroot, which both fixes the sysroot-escape issue
> > and results in a better GDB session for the user.
> > 
> > Built and regtested on RHEL 6.6 x86_64.
> > 
> > Luis, I think this patch will fix your connection drop without
> > any further changes.  Could you test it please?
> 
> Unfortunately it doesn't completely solve the problem i saw, as
> exec_file_find will still potentially throw errors and will disrupt
> the connection attempt or stop execution of a custom sequence of
> commands (as Pedro noted) when "attach" is part of the sequence.
> 
> define foo
> attach <pid>
> >>>> execution stops here if an error is thrown
> info threads
> info registers
> end
> 
> It still looks like a TRY/CATCH block is needed around at least
> exec_file_find.

What is throwing in exec_file_find?  I'm just seeing lots of calls
to gdb_open_cloexec and openp, and I don't think either of those
should throw except for assertion failures or running out of memory.

Cheers,
Gary
  

On 02/22/2016 07:40 AM, Gary Benson wrote:
> Luis Machado wrote:
>> On 02/19/2016 09:21 AM, Gary Benson wrote:
>>> This is an updated version of the patch I posted yesterday.
>>> It fails silently rather than throwing if the executable is
>>> not in the sysroot, which both fixes the sysroot-escape issue
>>> and results in a better GDB session for the user.
>>>
>>> Built and regtested on RHEL 6.6 x86_64.
>>>
>>> Luis, I think this patch will fix your connection drop without
>>> any further changes.  Could you test it please?
>>
>> Unfortunately it doesn't completely solve the problem i saw, as
>> exec_file_find will still potentially throw errors and will disrupt
>> the connection attempt or stop execution of a custom sequence of
>> commands (as Pedro noted) when "attach" is part of the sequence.
>>
>> define foo
>> attach <pid>
>>>>>> execution stops here if an error is thrown
>> info threads
>> info registers
>> end
>>
>> It still looks like a TRY/CATCH block is needed around at least
>> exec_file_find.
>
> What is throwing in exec_file_find?  I'm just seeing lots of calls
> to gdb_open_cloexec and openp, and I don't think either of those
> should throw except for assertion failures or running out of memory.

Not sure why i had exec_file_find in my mind. I meant to say 
exec_file_attach still throws errors, when openp fails and scratch_chan 
< 0. Sorry.

There is a symbol_file_add_main call right after calling 
exec_file_attach in exec_file_locate_attach, but i didn't see any errors 
being thrown from that one.
  

Luis Machado wrote:
> On 02/22/2016 07:40 AM, Gary Benson wrote:
> > Luis Machado wrote:
> > > On 02/19/2016 09:21 AM, Gary Benson wrote:
> > > > This is an updated version of the patch I posted yesterday.
> > > > It fails silently rather than throwing if the executable is
> > > > not in the sysroot, which both fixes the sysroot-escape issue
> > > > and results in a better GDB session for the user.
> > > > 
> > > > Built and regtested on RHEL 6.6 x86_64.
> > > > 
> > > > Luis, I think this patch will fix your connection drop without
> > > > any further changes.  Could you test it please?
> > > 
> > > Unfortunately it doesn't completely solve the problem i saw, as
> > > exec_file_find will still potentially throw errors and will
> > > disrupt the connection attempt or stop execution of a custom
> > > sequence of commands (as Pedro noted) when "attach" is part of
> > > the sequence.
> > > 
> > > define foo
> > > attach <pid>
> > > >>>>execution stops here if an error is thrown
> > > info threads
> > > info registers
> > > end
> > > 
> > > It still looks like a TRY/CATCH block is needed around at least
> > > exec_file_find.
> > 
> > What is throwing in exec_file_find?  I'm just seeing lots of calls
> > to gdb_open_cloexec and openp, and I don't think either of those
> > should throw except for assertion failures or running out of
> > memory.
> 
> Not sure why i had exec_file_find in my mind. I meant to say
> exec_file_attach still throws errors, when openp fails and
> scratch_chan < 0. Sorry.

You shouldn't get that now, the "if (full_exec_path == NULL) return"
should have caught it.  Are you still seeing thrown errors with your
setup?

> There is a symbol_file_add_main call right after calling
> exec_file_attach in exec_file_locate_attach, but i didn't see any
> errors being thrown from that one.

You could probably race it (e.g. by deleting the file between the
calls) but generally symbol_file_add_main won't fail because
exec_file_attach would have failed if the file was missing or
inaccessible.

Cheers,
Gary
  

On 02/22/2016 10:51 AM, Gary Benson wrote:
> Luis Machado wrote:
>> On 02/22/2016 07:40 AM, Gary Benson wrote:
>>> Luis Machado wrote:
>>>> On 02/19/2016 09:21 AM, Gary Benson wrote:
>>>>> This is an updated version of the patch I posted yesterday.
>>>>> It fails silently rather than throwing if the executable is
>>>>> not in the sysroot, which both fixes the sysroot-escape issue
>>>>> and results in a better GDB session for the user.
>>>>>
>>>>> Built and regtested on RHEL 6.6 x86_64.
>>>>>
>>>>> Luis, I think this patch will fix your connection drop without
>>>>> any further changes.  Could you test it please?
>>>>
>>>> Unfortunately it doesn't completely solve the problem i saw, as
>>>> exec_file_find will still potentially throw errors and will
>>>> disrupt the connection attempt or stop execution of a custom
>>>> sequence of commands (as Pedro noted) when "attach" is part of
>>>> the sequence.
>>>>
>>>> define foo
>>>> attach <pid>
>>>>>>>> execution stops here if an error is thrown
>>>> info threads
>>>> info registers
>>>> end
>>>>
>>>> It still looks like a TRY/CATCH block is needed around at least
>>>> exec_file_find.
>>>
>>> What is throwing in exec_file_find?  I'm just seeing lots of calls
>>> to gdb_open_cloexec and openp, and I don't think either of those
>>> should throw except for assertion failures or running out of
>>> memory.
>>
>> Not sure why i had exec_file_find in my mind. I meant to say
>> exec_file_attach still throws errors, when openp fails and
>> scratch_chan < 0. Sorry.
>
> You shouldn't get that now, the "if (full_exec_path == NULL) return"
> should have caught it.  Are you still seeing thrown errors with your
> setup?
>

Yes. With your patch applied, i still see a case where we error out. 
Suppose we have a test binary gdb/test, then:

- chmod -r gdb/test
- Fire up gdbserver with a test binary: ./gdb/gdbserver/gdbserver :2345 
gdb/test
- Fire up GDB: ./gdb/gdb -ex "set sysroot" -ex "tar rem :2345"

You will see something similar to the following:

Sending packet: $qXfer:exec-file:read:3486:0,fff#5f...Packet received: 
l/proc/13446/exe
/proc/13446/exe: Permission denied.
(gdb) i r
The program has no registers now.
(gdb)

This was the testcase suggested by Pedro and it proved to be a good one.

>> There is a symbol_file_add_main call right after calling
>> exec_file_attach in exec_file_locate_attach, but i didn't see any
>> errors being thrown from that one.
>
> You could probably race it (e.g. by deleting the file between the
> calls) but generally symbol_file_add_main won't fail because
> exec_file_attach would have failed if the file was missing or
> inaccessible.

My idea was to guard both exec_file_attach and symbol_file_add_main. We 
can't have anything in that function throwing an error that won't be 
caught, otherwise the above connection attempt will fail.

Luis
  

On 02/22/2016 07:00 PM, Luis Machado wrote:
> On 02/22/2016 10:51 AM, Gary Benson wrote:
>> Luis Machado wrote:
>>> On 02/22/2016 07:40 AM, Gary Benson wrote:
>>>> Luis Machado wrote:
>>>>> On 02/19/2016 09:21 AM, Gary Benson wrote:
>>>>>> This is an updated version of the patch I posted yesterday.
>>>>>> It fails silently rather than throwing if the executable is
>>>>>> not in the sysroot, which both fixes the sysroot-escape issue
>>>>>> and results in a better GDB session for the user.
>>>>>>
>>>>>> Built and regtested on RHEL 6.6 x86_64.
>>>>>>
>>>>>> Luis, I think this patch will fix your connection drop without
>>>>>> any further changes.  Could you test it please?
>>>>>
>>>>> Unfortunately it doesn't completely solve the problem i saw, as
>>>>> exec_file_find will still potentially throw errors and will
>>>>> disrupt the connection attempt or stop execution of a custom
>>>>> sequence of commands (as Pedro noted) when "attach" is part of
>>>>> the sequence.
>>>>>
>>>>> define foo
>>>>> attach <pid>
>>>>>>>>> execution stops here if an error is thrown
>>>>> info threads
>>>>> info registers
>>>>> end
>>>>>
>>>>> It still looks like a TRY/CATCH block is needed around at least
>>>>> exec_file_find.
>>>>
>>>> What is throwing in exec_file_find?  I'm just seeing lots of calls
>>>> to gdb_open_cloexec and openp, and I don't think either of those
>>>> should throw except for assertion failures or running out of
>>>> memory.
>>>
>>> Not sure why i had exec_file_find in my mind. I meant to say
>>> exec_file_attach still throws errors, when openp fails and
>>> scratch_chan < 0. Sorry.
>>
>> You shouldn't get that now, the "if (full_exec_path == NULL) return"
>> should have caught it.  Are you still seeing thrown errors with your
>> setup?
>>
>
> Yes. With your patch applied, i still see a case where we error out.
> Suppose we have a test binary gdb/test, then:
>
> - chmod -r gdb/test
> - Fire up gdbserver with a test binary: ./gdb/gdbserver/gdbserver :2345
> gdb/test
> - Fire up GDB: ./gdb/gdb -ex "set sysroot" -ex "tar rem :2345"
>
> You will see something similar to the following:
>
> Sending packet: $qXfer:exec-file:read:3486:0,fff#5f...Packet received:
> l/proc/13446/exe
> /proc/13446/exe: Permission denied.
> (gdb) i r
> The program has no registers now.
> (gdb)
>
> This was the testcase suggested by Pedro and it proved to be a good one.
>
>>> There is a symbol_file_add_main call right after calling
>>> exec_file_attach in exec_file_locate_attach, but i didn't see any
>>> errors being thrown from that one.
>>
>> You could probably race it (e.g. by deleting the file between the
>> calls) but generally symbol_file_add_main won't fail because
>> exec_file_attach would have failed if the file was missing or
>> inaccessible.
>
> My idea was to guard both exec_file_attach and symbol_file_add_main. We
> can't have anything in that function throwing an error that won't be
> caught, otherwise the above connection attempt will fail.
>
> Luis
>

For the record, you patch does fix the case of native GDB trying to 
attach to a process without pre-loading a symbol file. We get a 
multi-frame backtrace as expected.

It is the gdb/gdbserver case that still seems to be broken.
  

On 02/22/2016 10:49 PM, Luis Machado wrote:
> On 02/22/2016 07:00 PM, Luis Machado wrote:

>> My idea was to guard both exec_file_attach and symbol_file_add_main. We
>> can't have anything in that function throwing an error that won't be
>> caught, otherwise the above connection attempt will fail.
> 
> For the record, you patch does fix the case of native GDB trying to 
> attach to a process without pre-loading a symbol file. We get a 
> multi-frame backtrace as expected.
> 
> It is the gdb/gdbserver case that still seems to be broken.
> 

Native is also broken as well for unexpectedly aborting the attach
sequence midway.  While "bt" doesn't show it, "detach" does trip on it:

$ ~/gdb/tests/threads&
[1] 2984
$ chmod 000 ~/gdb/tests/threads

$ gdb -q
(gdb) attach 2984
Attaching to process 2984
/home/pedro/gdb/tests/threads: Permission denied.
(gdb) detach
/home/pedro/gdb/mygit/src/gdb/thread.c:1010: internal-error: is_executing: Assertion `tp' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

Thanks,
Pedro Alves
  

On 02/22/2016 08:00 PM, Pedro Alves wrote:
> On 02/22/2016 10:49 PM, Luis Machado wrote:
>> On 02/22/2016 07:00 PM, Luis Machado wrote:
>
>>> My idea was to guard both exec_file_attach and symbol_file_add_main. We
>>> can't have anything in that function throwing an error that won't be
>>> caught, otherwise the above connection attempt will fail.
>>
>> For the record, you patch does fix the case of native GDB trying to
>> attach to a process without pre-loading a symbol file. We get a
>> multi-frame backtrace as expected.
>>
>> It is the gdb/gdbserver case that still seems to be broken.
>>
>
> Native is also broken as well for unexpectedly aborting the attach
> sequence midway.  While "bt" doesn't show it, "detach" does trip on it:
>
> $ ~/gdb/tests/threads&
> [1] 2984
> $ chmod 000 ~/gdb/tests/threads
>
> $ gdb -q
> (gdb) attach 2984
> Attaching to process 2984
> /home/pedro/gdb/tests/threads: Permission denied.
> (gdb) detach
> /home/pedro/gdb/mygit/src/gdb/thread.c:1010: internal-error: is_executing: Assertion `tp' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
>
> Thanks,
> Pedro Alves
>

Sounds like we're looking into a try/catch block around such functions then?
  

On 02/23/2016 12:04 AM, Luis Machado wrote:
> On 02/22/2016 08:00 PM, Pedro Alves wrote:
>> On 02/22/2016 10:49 PM, Luis Machado wrote:
>>> On 02/22/2016 07:00 PM, Luis Machado wrote:
>>
>>>> My idea was to guard both exec_file_attach and symbol_file_add_main. We
>>>> can't have anything in that function throwing an error that won't be
>>>> caught, otherwise the above connection attempt will fail.
>>>
>>> For the record, you patch does fix the case of native GDB trying to
>>> attach to a process without pre-loading a symbol file. We get a
>>> multi-frame backtrace as expected.
>>>
>>> It is the gdb/gdbserver case that still seems to be broken.
>>>
>>
>> Native is also broken as well for unexpectedly aborting the attach
>> sequence midway.  While "bt" doesn't show it, "detach" does trip on it:
>>

> Sounds like we're looking into a try/catch block around such functions then?
> 

Yes, I think so.  Probably best to make it two separate try/catches,
in case the file can't be loaded as executable but succeeds as
symbol file, for some reason.

Thanks,
Pedro Alves
  

On 02/22/2016 09:13 PM, Pedro Alves wrote:
> On 02/23/2016 12:04 AM, Luis Machado wrote:
>> On 02/22/2016 08:00 PM, Pedro Alves wrote:
>>> On 02/22/2016 10:49 PM, Luis Machado wrote:
>>>> On 02/22/2016 07:00 PM, Luis Machado wrote:
>>>
>>>>> My idea was to guard both exec_file_attach and symbol_file_add_main. We
>>>>> can't have anything in that function throwing an error that won't be
>>>>> caught, otherwise the above connection attempt will fail.
>>>>
>>>> For the record, you patch does fix the case of native GDB trying to
>>>> attach to a process without pre-loading a symbol file. We get a
>>>> multi-frame backtrace as expected.
>>>>
>>>> It is the gdb/gdbserver case that still seems to be broken.
>>>>
>>>
>>> Native is also broken as well for unexpectedly aborting the attach
>>> sequence midway.  While "bt" doesn't show it, "detach" does trip on it:
>>>
>
>> Sounds like we're looking into a try/catch block around such functions then?
>>
>
> Yes, I think so.  Probably best to make it two separate try/catches,
> in case the file can't be loaded as executable but succeeds as
> symbol file, for some reason.
>
> Thanks,
> Pedro Alves
>

Ok. I have the code in place, but i'm still crafting the testcase for 
both native / remote cases. Should have something later this week.
  

Luis Machado wrote:
> On 02/22/2016 09:13 PM, Pedro Alves wrote:
> > On 02/23/2016 12:04 AM, Luis Machado wrote:
> > >On 02/22/2016 08:00 PM, Pedro Alves wrote:
> > > > On 02/22/2016 10:49 PM, Luis Machado wrote:
> > > > > On 02/22/2016 07:00 PM, Luis Machado wrote:
> > > > > > My idea was to guard both exec_file_attach and
> > > > > > symbol_file_add_main. We can't have anything in that
> > > > > > function throwing an error that won't be caught, otherwise
> > > > > > the above connection attempt will fail.
> > > > >
> > > > > For the record, you patch does fix the case of native GDB
> > > > > trying to attach to a process without pre-loading a symbol
> > > > > file. We get a multi-frame backtrace as expected.
> > > > >
> > > > > It is the gdb/gdbserver case that still seems to be broken.
> > > > 
> > > > Native is also broken as well for unexpectedly aborting the
> > > > attach sequence midway.  While "bt" doesn't show it, "detach"
> > > > does trip on it:
> > > 
> > > Sounds like we're looking into a try/catch block around such
> > > functions then?
> > 
> > Yes, I think so.  Probably best to make it two separate
> > try/catches, in case the file can't be loaded as executable but
> > succeeds as symbol file, for some reason.
> 
> Ok. I have the code in place, but i'm still crafting the testcase
> for both native / remote cases. Should have something later this
> week.

I hadn't considered user interrupts.

Pedro, are you ok if I commit my v2 patch
(https://sourceware.org/ml/gdb-patches/2016-02/msg00587.html)
as a fix for the sysroot-escaping behaviour we saw?

Thanks,
Gary

--
http://gbenson.net/
  

On 02/23/2016 11:27 AM, Gary Benson wrote:

> I hadn't considered user interrupts.

But there's nothing about user interrupts in either of:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00511.html

or:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00671.html

That is about gdb managing to figure out the file name of the running
program, but then trying to open the file, and that failing and throwing.

- The original failure Luis found was triggered when the file didn't
exist at all in the sysroot.

- The failure I shown in the urls above is that the file exists in the
sysroot but is unreadable.

These are both basically the same problem, except the latter is easier
to reproduce.

So trying to open the file _in_ the sysroot may fail and throw, but
that should not abort the remote connection, nor an "attach" (command)
sequence, both of which use the same exec_file_locate_attach routine
-- the remote connection case is really basically doing an attach.

Thanks,
Pedro Alves
  

On 02/19/2016 11:21 AM, Gary Benson wrote:

> ---
> This commit fixes an error in exec_file_locate_attach where
> the main executable could be loaded from outside the sysroot
> if a nonempty, non-"target:" sysroot was set but the discovered
> executable filename did not exist in that sysroot and did exist
> on the main filesystem.
> 
> gdb/ChangeLog:
> 
> 	* exec.c (exec_file_locate_attach): Do not attempt to
> 	locate main executable locally if not found in sysroot.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/attach-pie-noexec.exp: Do not expect an error
> 	message on attach.

OK.

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 02/23/2016 11:27 AM, Gary Benson wrote:
> > I hadn't considered user interrupts.
> 
> But there's nothing about user interrupts in either of:
> 
>  https://sourceware.org/ml/gdb-patches/2016-02/msg00511.html
> 
> or:
> 
>  https://sourceware.org/ml/gdb-patches/2016-02/msg00671.html
> 
> That is about gdb managing to figure out the file name of the
> running program, but then trying to open the file, and that failing
> and throwing.

Sure.  But up until now I've been thinking my way through the cases in
the exec_file_locate_attach in terms of file accessibility checks, and
the possibility of user interrupts makes that irrelevent.

I'm not averse to TRY..CATCH, in fact I think the suggestion of two
separate TRY..CATCH blocks around exec_file_attach the symbol file
one is the correct solution.

But, there is a separate issue, which is that if you run gdbserver on
some executable, and GDB has a sysroot set, and that executable does
not exist in GDB's sysroot but does exist on GDB's root filesystem,
then GDB will open the file from its root filesystem:

  gdb -ex "set sysroot /xxx" -ex "target remote | gdbserver - /bin/ls"
  ...
  Reading symbols from /bin/ls...(no debugging symbols found)...done.
  
My v2 patch stops that.  So... can I commit it?

Cheers,
Gary
  

On 02/23/2016 12:15 PM, Gary Benson wrote:

> But, there is a separate issue, which is that if you run gdbserver on
> some executable, and GDB has a sysroot set, and that executable does
> not exist in GDB's sysroot but does exist on GDB's root filesystem,
> then GDB will open the file from its root filesystem:
> 
>   gdb -ex "set sysroot /xxx" -ex "target remote | gdbserver - /bin/ls"
>   ...
>   Reading symbols from /bin/ls...(no debugging symbols found)...done.
>   

Yes, I had already agreed that gdb should not do that.

> My v2 patch stops that.  So... can I commit it?

I've already approved it.

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 02/19/2016 11:21 AM, Gary Benson wrote:
> > This commit fixes an error in exec_file_locate_attach where
> > the main executable could be loaded from outside the sysroot
> > if a nonempty, non-"target:" sysroot was set but the discovered
> > executable filename did not exist in that sysroot and did exist
> > on the main filesystem.
> > 
> > gdb/ChangeLog:
> > 
> > 	* exec.c (exec_file_locate_attach): Do not attempt to
> > 	locate main executable locally if not found in sysroot.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/attach-pie-noexec.exp: Do not expect an error
> > 	message on attach.
> 
> OK.

Pushed.

Thanks,
Gary