[PATCH] Remote debugging without a binary (regression)

[PATCH] Remote debugging without a binary (regression)

[Luis Machado]

cc-ing Gary.

It looks like this is fallout from the changes that were added to make
GDB a bit smarter about locating the binary that is being debugged.

When one attempts to do gdbserver-based debugging in the same
machine/filesystem, there is no problem at all.

If the user wants to have the files transfered over the wire, GDB will
handle it. If the user sets a local sysroot path and doesn't want the
file coming through the wire, GDB will use process information to
attempt to locate the binary in the local filesystem.

Now, considering we have a GDB instance running on a local machine and
a gdbserver instance running on a remote machine with a completely separate
filesystem, having the sysroot set will prevent the file from being
downloaded.

GDB will then attempt to be smart and locate the binary through the path that
is reported by gdbserver. This path is from the remote filesystem though, so
there is a chance this file won't even exist in the local filesystem.

In a normal native session (where we start the process from scratch) this would
result in a "No such file or directory" error. And that is fine, because we
really need a binary to get the process started.

But with a local GDB plus a remote gdbserver on a different filesystem, we will
see the same error and the debugging session will end abruptly, giving the user
no chance of doing some debugging without a symbol file.

--
Remote debugging using some_machine:12345
<some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
--

I tracked this down to remote_add_inferior and its call to (mainly)
exec_file_locate_attach. This specific function will call other functions
that may throw an error, causing everything to stop dead on its tracks.

The following patch guards such a call to prevent those errors from
disrupting a potential debugging session, and display only a warning.

--
Remote debugging using some_machine:12345
warning: <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
--

I tried to come up with a valid testcase that would fail with a local
gdb/gdbserver combination, but it seems GDB is smart enough to recognize
a deleted binary with the help of /proc, thus foiling my attempts.

Any ideas?

gdb/ChangeLog:

2016-02-11  Luis Machado  <lgustavo@codesourcery.com>

	* remote.c (remote_add_inferior): Guard block that can throw
	errors.
---
 gdb/remote.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6d56f19..3b63e9f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1789,10 +1789,32 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
   inf->attach_flag = attached;
   inf->fake_pid_p = fake_pid_p;
 
-  /* If no main executable is currently open then attempt to
-     open the file that was executed to create this inferior.  */
-  if (try_open_exec && get_exec_file (0) == NULL)
-    exec_file_locate_attach (pid, 1);
+
+  /* exec_file_locate_attach may throw an error if the file cannot be
+     opened either locally or remotely.  This happens when, for example,
+     GDB connects to a gdbserver that is running on a different
+     filesystem and the sysroot is set to non-target-based (no "target:").
+
+     Then GDB will neither load the binary from the target nor be able to
+     load a binary from the local filesystem (it may not exist in the local
+     filesystem in the same path as in the remote filesystem).
+
+     Even without a binary, the remote-based debugging session should
+     continue normally instead of ending abruptly.  */
+
+  TRY
+    {
+      /* If no main executable is currently open then attempt to
+	 open the file that was executed to create this inferior.  */
+      if (try_open_exec && get_exec_file (0) == NULL)
+	exec_file_locate_attach (pid, 1);
+    }
+  CATCH (err, RETURN_MASK_ALL)
+    {
+      if (err.message != NULL)
+	warning ("%s", err.message);
+    }
+  END_CATCH
 
   return inf;
 }
-- 
1.9.1

Re: [PATCH] Remote debugging without a binary (regression)

[Gary Benson]

Hi Luis,

Luis Machado wrote:
> cc-ing Gary.
> 
> It looks like this is fallout from the changes that were added to
> make GDB a bit smarter about locating the binary that is being
> debugged.
> 
> When one attempts to do gdbserver-based debugging in the same
> machine/filesystem, there is no problem at all.
> 
> If the user wants to have the files transfered over the wire, GDB
> will handle it. If the user sets a local sysroot path and doesn't
> want the file coming through the wire, GDB will use process
> information to attempt to locate the binary in the local filesystem.
> 
> Now, considering we have a GDB instance running on a local machine
> and a gdbserver instance running on a remote machine with a
> completely separate filesystem, having the sysroot set will prevent
> the file from being downloaded.
> 
> GDB will then attempt to be smart and locate the binary through the
> path that is reported by gdbserver. This path is from the remote
> filesystem though, so there is a chance this file won't even exist
> in the local filesystem.
> 
> In a normal native session (where we start the process from scratch)
> this would result in a "No such file or directory" error. And that
> is fine, because we really need a binary to get the process started.
> 
> But with a local GDB plus a remote gdbserver on a different
> filesystem, we will see the same error and the debugging session
> will end abruptly, giving the user no chance of doing some debugging
> without a symbol file.
> 
> --
> Remote debugging using some_machine:12345
> <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
> --
> 
> I tracked this down to remote_add_inferior and its call to (mainly)
> exec_file_locate_attach. This specific function will call other
> functions that may throw an error, causing everything to stop dead
> on its tracks.
> 
> The following patch guards such a call to prevent those errors from
> disrupting a potential debugging session, and display only a warning.
> 
> --
> Remote debugging using some_machine:12345
> warning: <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
> --
> 
> I tried to come up with a valid testcase that would fail with a
> local gdb/gdbserver combination, but it seems GDB is smart enough to
> recognize a deleted binary with the help of /proc, thus foiling my
> attempts.

I don't have any fundamental objection to your patch but I'm not
really sure I understand what's going on here.  You have the sysroot
set to some path that does not exist?  What are you trying to do and
what are you expecting to be able to do?  What did GDB do before?

Thanks,
Gary

--
http://gbenson.net/

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

On 02/11/2016 02:35 PM, Gary Benson wrote:
> Hi Luis,
>
> Luis Machado wrote:
>> cc-ing Gary.
>>
>> It looks like this is fallout from the changes that were added to
>> make GDB a bit smarter about locating the binary that is being
>> debugged.
>>
>> When one attempts to do gdbserver-based debugging in the same
>> machine/filesystem, there is no problem at all.
>>
>> If the user wants to have the files transfered over the wire, GDB
>> will handle it. If the user sets a local sysroot path and doesn't
>> want the file coming through the wire, GDB will use process
>> information to attempt to locate the binary in the local filesystem.
>>
>> Now, considering we have a GDB instance running on a local machine
>> and a gdbserver instance running on a remote machine with a
>> completely separate filesystem, having the sysroot set will prevent
>> the file from being downloaded.
>>
>> GDB will then attempt to be smart and locate the binary through the
>> path that is reported by gdbserver. This path is from the remote
>> filesystem though, so there is a chance this file won't even exist
>> in the local filesystem.
>>
>> In a normal native session (where we start the process from scratch)
>> this would result in a "No such file or directory" error. And that
>> is fine, because we really need a binary to get the process started.
>>
>> But with a local GDB plus a remote gdbserver on a different
>> filesystem, we will see the same error and the debugging session
>> will end abruptly, giving the user no chance of doing some debugging
>> without a symbol file.
>>
>> --
>> Remote debugging using some_machine:12345
>> <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
>> --
>>
>> I tracked this down to remote_add_inferior and its call to (mainly)
>> exec_file_locate_attach. This specific function will call other
>> functions that may throw an error, causing everything to stop dead
>> on its tracks.
>>
>> The following patch guards such a call to prevent those errors from
>> disrupting a potential debugging session, and display only a warning.
>>
>> --
>> Remote debugging using some_machine:12345
>> warning: <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
>> --
>>
>> I tried to come up with a valid testcase that would fail with a
>> local gdb/gdbserver combination, but it seems GDB is smart enough to
>> recognize a deleted binary with the help of /proc, thus foiling my
>> attempts.
>
> I don't have any fundamental objection to your patch but I'm not
> really sure I understand what's going on here.  You have the sysroot
> set to some path that does not exist?  What are you trying to do and
> what are you expecting to be able to do?  What did GDB do before?

No. The sysroot being anything other than "target:" is needed is order 
to prevent gdbserver from transfering the files over (too slow). Plus, 
i'm not loading any symbol file on GDB's side.

So i'm trying to connect to a gdbserver running on a remote system with 
a separate filesystem. gdbserver will now report the full path to the 
binary on the remote end via the new qXfer:exec-file packet, even if i 
force the sysroot to be empty.

In summary, GDB (running on a local machine) is attempting to use that 
path provided by qXfer:exec-file to open a symbol file that only exists 
on the remote end's filesystem, not in the local filesystem where GDB is 
running.

If GDB fails to locate that file, it will drop the connection due to a 
error that is thrown from within exec_file_locate_attach and its callees.

The correct behavior is for GDB to ignore the lack of a symbol file and 
carry on connecting to the remote target, allowing a symbol-less 
debugging session.

Does that make it clear?

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/11/2016 05:06 PM, Luis Machado wrote:

> Does that make it clear?

Sounds like we're missing a test.  Say, a test in gdb.server/
that:

 #1 - does "set sysroot /dev/null"
 #2 - connects
 #3 - does "disassemble", "si", "info registers", or something.

Sounds like we currently fail step #2.  Steps #3 would be there just
to make sure the session is not semi-borked even if we connected
successfully.

Thanks,
Pedro Alves

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

On 02/11/2016 03:31 PM, Pedro Alves wrote:
> On 02/11/2016 05:06 PM, Luis Machado wrote:
>
>> Does that make it clear?
>
> Sounds like we're missing a test.  Say, a test in gdb.server/
> that:
>
>   #1 - does "set sysroot /dev/null"
>   #2 - connects
>   #3 - does "disassemble", "si", "info registers", or something.
>
> Sounds like we currently fail step #2.  Steps #3 would be there just
> to make sure the session is not semi-borked even if we connected
> successfully.

Yeah. Unfortunately that does not work well because GDB is smart enough 
to try and load the binary gdbserver loaded, which happens to be sitting 
in the same filesystem, so it won't fail.

I also attempted to delete the binary right after firing gdbserver up, 
but it also does not fail, though GDB acknowledges now that the file has 
been deleted.

The problematic scenario appears when one has two distinct filesystems. 
Say, one in each machine. Then GDB won't be able to load the symbol file 
and will error out.

This is usually the case when one is doing Linux cross debugging.

Re: [PATCH] Remote debugging without a binary (regression)

[Gary Benson]

Luis Machado wrote:
> On 02/11/2016 02:35 PM, Gary Benson wrote:
> >Luis Machado wrote:
> > > It looks like this is fallout from the changes that were added to
> > > make GDB a bit smarter about locating the binary that is being
> > > debugged.
> > > 
> > > When one attempts to do gdbserver-based debugging in the same
> > > machine/filesystem, there is no problem at all.
> > > 
> > > If the user wants to have the files transfered over the wire, GDB
> > > will handle it. If the user sets a local sysroot path and doesn't
> > > want the file coming through the wire, GDB will use process
> > > information to attempt to locate the binary in the local filesystem.
> > > 
> > > Now, considering we have a GDB instance running on a local machine
> > > and a gdbserver instance running on a remote machine with a
> > > completely separate filesystem, having the sysroot set will prevent
> > > the file from being downloaded.
> > > 
> > > GDB will then attempt to be smart and locate the binary through the
> > > path that is reported by gdbserver. This path is from the remote
> > > filesystem though, so there is a chance this file won't even exist
> > > in the local filesystem.
> > > 
> > > In a normal native session (where we start the process from scratch)
> > > this would result in a "No such file or directory" error. And that
> > > is fine, because we really need a binary to get the process started.
> > > 
> > > But with a local GDB plus a remote gdbserver on a different
> > > filesystem, we will see the same error and the debugging session
> > > will end abruptly, giving the user no chance of doing some debugging
> > > without a symbol file.
> > > 
> > > --
> > > Remote debugging using some_machine:12345
> > > <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
> > > --
> > > 
> > > I tracked this down to remote_add_inferior and its call to (mainly)
> > > exec_file_locate_attach. This specific function will call other
> > > functions that may throw an error, causing everything to stop dead
> > > on its tracks.
> > > 
> > > The following patch guards such a call to prevent those errors from
> > > disrupting a potential debugging session, and display only a warning.
> > > 
> > > --
> > > Remote debugging using some_machine:12345
> > > warning: <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
> > > --
> > > 
> > > I tried to come up with a valid testcase that would fail with a
> > > local gdb/gdbserver combination, but it seems GDB is smart enough to
> > > recognize a deleted binary with the help of /proc, thus foiling my
> > > attempts.
> >
> > I don't have any fundamental objection to your patch but I'm not
> > really sure I understand what's going on here.  You have the
> > sysroot set to some path that does not exist?  What are you trying
> > to do and what are you expecting to be able to do?  What did GDB
> > do before?
> 
> No. The sysroot being anything other than "target:" is needed is
> order to prevent gdbserver from transfering the files over (too
> slow). Plus, i'm not loading any symbol file on GDB's side.
> 
> So i'm trying to connect to a gdbserver running on a remote system
> with a separate filesystem. gdbserver will now report the full path
> to the binary on the remote end via the new qXfer:exec-file packet,
> even if i force the sysroot to be empty.
> 
> In summary, GDB (running on a local machine) is attempting to use
> that path provided by qXfer:exec-file to open a symbol file that
> only exists on the remote end's filesystem, not in the local
> filesystem where GDB is running.
> 
> If GDB fails to locate that file, it will drop the connection due to
> a error that is thrown from within exec_file_locate_attach and its
> callees.
> 
> The correct behavior is for GDB to ignore the lack of a symbol file
> and carry on connecting to the remote target, allowing a symbol-less
> debugging session.
> 
> Does that make it clear?

I'm getting there, but I have a couple more questions:

1) What exactly are you setting sysroot to?  Is it:
    - the empty string
    - a directory full of shared libraries but not the main executable
    - an empty directory
    - a non-existent directory?

2) What exactly is the error being thrown within exec_file_locate_attach?

FWIW I tried this (both on the same machine):

  gdbserver :9999 /bin/ls
  gdb -q -ex "set sysroot /whatever" -ex "target remote :9999"

and got this:

  Reading symbols from /bin/ls...(no debugging symbols found)...done.

which I think is an error: the sysroot is being ignored.

Once again, I have no fundamental problem with your patch, but I want
to make sure we're not papering over some deeper issue.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

On 02/12/2016 08:31 AM, Gary Benson wrote:
> Luis Machado wrote:
>> On 02/11/2016 02:35 PM, Gary Benson wrote:
>>> Luis Machado wrote:
>>>> It looks like this is fallout from the changes that were added to
>>>> make GDB a bit smarter about locating the binary that is being
>>>> debugged.
>>>>
>>>> When one attempts to do gdbserver-based debugging in the same
>>>> machine/filesystem, there is no problem at all.
>>>>
>>>> If the user wants to have the files transfered over the wire, GDB
>>>> will handle it. If the user sets a local sysroot path and doesn't
>>>> want the file coming through the wire, GDB will use process
>>>> information to attempt to locate the binary in the local filesystem.
>>>>
>>>> Now, considering we have a GDB instance running on a local machine
>>>> and a gdbserver instance running on a remote machine with a
>>>> completely separate filesystem, having the sysroot set will prevent
>>>> the file from being downloaded.
>>>>
>>>> GDB will then attempt to be smart and locate the binary through the
>>>> path that is reported by gdbserver. This path is from the remote
>>>> filesystem though, so there is a chance this file won't even exist
>>>> in the local filesystem.
>>>>
>>>> In a normal native session (where we start the process from scratch)
>>>> this would result in a "No such file or directory" error. And that
>>>> is fine, because we really need a binary to get the process started.
>>>>
>>>> But with a local GDB plus a remote gdbserver on a different
>>>> filesystem, we will see the same error and the debugging session
>>>> will end abruptly, giving the user no chance of doing some debugging
>>>> without a symbol file.
>>>>
>>>> --
>>>> Remote debugging using some_machine:12345
>>>> <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
>>>> --
>>>>
>>>> I tracked this down to remote_add_inferior and its call to (mainly)
>>>> exec_file_locate_attach. This specific function will call other
>>>> functions that may throw an error, causing everything to stop dead
>>>> on its tracks.
>>>>
>>>> The following patch guards such a call to prevent those errors from
>>>> disrupting a potential debugging session, and display only a warning.
>>>>
>>>> --
>>>> Remote debugging using some_machine:12345
>>>> warning: <some_remote_filesystem_path/gdb.d/gdb.base/break: No such file or directory.
>>>> --
>>>>
>>>> I tried to come up with a valid testcase that would fail with a
>>>> local gdb/gdbserver combination, but it seems GDB is smart enough to
>>>> recognize a deleted binary with the help of /proc, thus foiling my
>>>> attempts.
>>>
>>> I don't have any fundamental objection to your patch but I'm not
>>> really sure I understand what's going on here.  You have the
>>> sysroot set to some path that does not exist?  What are you trying
>>> to do and what are you expecting to be able to do?  What did GDB
>>> do before?
>>
>> No. The sysroot being anything other than "target:" is needed is
>> order to prevent gdbserver from transfering the files over (too
>> slow). Plus, i'm not loading any symbol file on GDB's side.
>>
>> So i'm trying to connect to a gdbserver running on a remote system
>> with a separate filesystem. gdbserver will now report the full path
>> to the binary on the remote end via the new qXfer:exec-file packet,
>> even if i force the sysroot to be empty.
>>
>> In summary, GDB (running on a local machine) is attempting to use
>> that path provided by qXfer:exec-file to open a symbol file that
>> only exists on the remote end's filesystem, not in the local
>> filesystem where GDB is running.
>>
>> If GDB fails to locate that file, it will drop the connection due to
>> a error that is thrown from within exec_file_locate_attach and its
>> callees.
>>
>> The correct behavior is for GDB to ignore the lack of a symbol file
>> and carry on connecting to the remote target, allowing a symbol-less
>> debugging session.
>>
>> Does that make it clear?
>
> I'm getting there, but I have a couple more questions:
>
> 1) What exactly are you setting sysroot to?  Is it:
>      - the empty string
>      - a directory full of shared libraries but not the main executable
>      - an empty directory
>      - a non-existent directory?
>

It doesn't matter, as long as it is not "target:", meaning we really 
don't want to load files using the help of gdbserver.

> 2) What exactly is the error being thrown within exec_file_locate_attach?
>

The one i mentioned above:

<some_remote_filesystem_path>/gdb.d/gdb.base/break: No such file or 
directory.

This comes from exec_file_attach. In my specific case above, this is 
thrown from gdb/exec.c:268, a call to perror_with_name.

           if (scratch_chan < 0)
             perror_with_name (filename);

It seems this was introduced with commit 
1b6e6f5c7ffba559a681d11852acf38ef48dceff, with the addition of a call to 
exec_file_locate_attach from within remote_add_inferior.

That unguarded call to exec_file_locate_attach doesn't look safe since 
its callees can throw errors and potentially disrupt a connection attempt.

> FWIW I tried this (both on the same machine):
>

Doing so with both on the same machine/filesystem will not reproduce the 
problem, as i mentioned in my original post. GDB and gdbserver need to 
be on separate filesystems.

>    gdbserver :9999 /bin/ls
>    gdb -q -ex "set sysroot /whatever" -ex "target remote :9999"
>
> and got this:
>
>    Reading symbols from /bin/ls...(no debugging symbols found)...done.
>
> which I think is an error: the sysroot is being ignored.
>

Isn't it being ignored because GDB managed to figure out the path and 
successfully open the symbol file?

If it should honor the sysroot in that case, that looks like a different 
problem than the one i was originally chasing. There may be more bugs. :-)

> Once again, I have no fundamental problem with your patch, but I want
> to make sure we're not papering over some deeper issue.
>
> Thanks,
> Gary
>

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/12/2016 10:31 AM, Gary Benson wrote:

> FWIW I tried this (both on the same machine):
> 
>   gdbserver :9999 /bin/ls
>   gdb -q -ex "set sysroot /whatever" -ex "target remote :9999"
> 
> and got this:
> 
>   Reading symbols from /bin/ls...(no debugging symbols found)...done.
> 
> which I think is an error: the sysroot is being ignored.

I agree.  If you tell gdb about a sysroot, then I can't think why
you'd want it to try opening an absolute filename on the host, outside
the sysroot.

(caching and buildid matching aside)

Thanks,
Pedro Alves

Re: [PATCH] Remote debugging without a binary (regression)

[Gary Benson]

Pedro Alves wrote:
> On 02/12/2016 10:31 AM, Gary Benson wrote:
> > FWIW I tried this (both on the same machine):
> > 
> >   gdbserver :9999 /bin/ls
> >   gdb -q -ex "set sysroot /whatever" -ex "target remote :9999"
> > 
> > and got this:
> > 
> >   Reading symbols from /bin/ls...(no debugging symbols found)...done.
> > 
> > which I think is an error: the sysroot is being ignored.
> 
> I agree.  If you tell gdb about a sysroot, then I can't think why
> you'd want it to try opening an absolute filename on the host,
> outside the sysroot.
> 
> (caching and buildid matching aside)

There isn't any caching or buildid matching, the logic in
exec_file_locate_attach is wrong.  It might even be the cause
of the exception Luis is seeing so with luck we can avoid a
try-catch in remote_add_inferior.  I'm putting together a patch
now.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/17/2016 01:53 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 02/12/2016 10:31 AM, Gary Benson wrote:
>>> FWIW I tried this (both on the same machine):
>>>
>>>   gdbserver :9999 /bin/ls
>>>   gdb -q -ex "set sysroot /whatever" -ex "target remote :9999"
>>>
>>> and got this:
>>>
>>>   Reading symbols from /bin/ls...(no debugging symbols found)...done.
>>>
>>> which I think is an error: the sysroot is being ignored.
>>
>> I agree.  If you tell gdb about a sysroot, then I can't think why
>> you'd want it to try opening an absolute filename on the host,
>> outside the sysroot.
>>
>> (caching and buildid matching aside)
> 
> There isn't any caching or buildid matching, the logic in
> exec_file_locate_attach is wrong.

Yes, what I meant was -- a valid reason I'd see gdb trying to open a file
on the host outside the sysroot would be for smarter buildid matching and
local caching of (hunks of) files.  But we don't have that.

> It might even be the cause
> of the exception Luis is seeing so with luck we can avoid a
> try-catch in remote_add_inferior.  I'm putting together a patch
> now.

Yes, if GDB ends up not trying to open any file, then it'll mask off
the exception Luis is seeing.

However, even if the sysroot points at the location with a
valid copy of the file, if the file is NOT readable, then we'll
still throw and close the connection.

Wait, actually, that's actually a good way to add a test to the testsuite
to cover Luis's use case, even without separate filesystems:

gdbserver:

 $ ./gdbserver/gdbserver :9999 ~/gdb/tests/threads&
 $ chmod 000 ~/gdb/tests/threads

gdb:

 (gdb) tar remote :9999
 Remote debugging using :9999
 Reading /home/pedro/gdb/tests/threads from remote target...
 warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
 "target:/home/pedro/gdb/tests/threads": could not open as an executable file: Permission denied.
 (gdb) maint print target-stack
 The current target stack is:
   - None (None)
 (gdb)

Above we dropped the connection...

Thanks,
Pedro Alves

[OB PATCH] Add missing cleanup in exec_file_locate_attach

[Gary Benson]

Hi all,

exec_file_locate_attach allocates memory for full_exec_path (using
either exec_file_find, source_full_path_of or xstrdup) but this
memory is never freed.  This commit adds the necessary cleanup.

Pushed as obvious.

Cheers,
Gary

--
gdb/ChangeLog:

	* exec.c (exec_file_locate_attach): Add missing cleanup.
---
 gdb/ChangeLog |    4 ++++
 gdb/exec.c    |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 23c89c2..0b8c077 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -141,6 +141,7 @@ void
 exec_file_locate_attach (int pid, int from_tty)
 {
   char *exec_file, *full_exec_path = NULL;
+  struct cleanup *old_chain;
 
   /* Do nothing if we already have an executable filename.  */
   exec_file = (char *) get_exec_file (0);
@@ -170,8 +171,12 @@ exec_file_locate_attach (int pid, int from_tty)
 	full_exec_path = xstrdup (exec_file);
     }
 
+  old_chain = make_cleanup (xfree, full_exec_path);
+
   exec_file_attach (full_exec_path, from_tty);
   symbol_file_add_main (full_exec_path, from_tty);
+
+  do_cleanups (old_chain);
 }
 
 /* Set FILENAME as the new exec file.
-- 
1.7.1

Re: [OB PATCH] Add missing cleanup in exec_file_locate_attach

[Gary Benson]

Gary Benson wrote:
> exec_file_locate_attach allocates memory for full_exec_path (using
> either exec_file_find, source_full_path_of or xstrdup) but this
> memory is never freed.  This commit adds the necessary cleanup.

This isn't the fix I talked about, just something I found on the way.
I didn't mean to copy everyone on this little patch :)  The real fix
is coming tomorrow morning.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [OB PATCH] Add missing cleanup in exec_file_locate_attach

[Luis Machado]

On 02/17/2016 03:02 PM, Gary Benson wrote:
> Hi all,
>
> exec_file_locate_attach allocates memory for full_exec_path (using
> either exec_file_find, source_full_path_of or xstrdup) but this
> memory is never freed.  This commit adds the necessary cleanup.
>
> Pushed as obvious.


Well spotted!

Re: [OB PATCH] Add missing cleanup in exec_file_locate_attach

[Gary Benson]

Luis Machado wrote:
> On 02/17/2016 03:02 PM, Gary Benson wrote:
> > exec_file_locate_attach allocates memory for full_exec_path (using
> > either exec_file_find, source_full_path_of or xstrdup) but this
> > memory is never freed.  This commit adds the necessary cleanup.
> > 
> > Pushed as obvious.
> 
> Well spotted!

Thanks :)

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH] Fix logic in exec_file_locate_attach

[Gary Benson]

Hi all,

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.

Before:
  gdb -q \
    -ex "set sysroot /whatever" \
    -ex "target remote | gdbserver - /bin/ls"
  ...
  Reading symbols from /bin/ls...(no debugging symbols found)...done.

After:
  gdb -q \
    -ex "set sysroot /whatever" \
    -ex "target remote | gdbserver - /bin/ls"
  ...
  /bin/ls: in sysroot "/whatever": No such file or directory.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Cheers,
Gary

--
gdb/ChangeLog:

	* exec.c (exec_file_locate_attach): Throw error if
	exec_file_find fails to locate the main executable.

gdb/testsuite/ChangeLog:

	* gdb.base/attach-bad-sysroot.exp: New file.
	* gdb.base/attach-bad-sysroot.c: Likewise.
---
 gdb/ChangeLog                                 |    5 +++
 gdb/exec.c                                    |   13 +++++++--
 gdb/testsuite/ChangeLog                       |    5 +++
 gdb/testsuite/gdb.base/attach-bad-sysroot.c   |   25 +++++++++++++++++
 gdb/testsuite/gdb.base/attach-bad-sysroot.exp |   36 +++++++++++++++++++++++++
 5 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/attach-bad-sysroot.c
 create mode 100644 gdb/testsuite/gdb.base/attach-bad-sysroot.exp

diff --git a/gdb/exec.c b/gdb/exec.c
index 0b8c077..8f19871 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -156,9 +156,16 @@ exec_file_locate_attach (int pid, int from_tty)
   /* If gdb_sysroot is not empty and the discovered filename
      is absolute then prefix the filename with gdb_sysroot.  */
   if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file))
-    full_exec_path = exec_file_find (exec_file, NULL);
-
-  if (full_exec_path == NULL)
+    {
+      full_exec_path = exec_file_find (exec_file, NULL);
+      if (full_exec_path == NULL)
+	{
+	  error (_("%s: in sysroot \"%s\":"
+		   " No such file or directory."),
+		 exec_file, gdb_sysroot);
+	}
+    }
+  else
     {
       /* It's possible we don't have a full path, but rather just a
 	 filename.  Some targets, such as HP-UX, don't provide the
diff --git a/gdb/testsuite/gdb.base/attach-bad-sysroot.c b/gdb/testsuite/gdb.base/attach-bad-sysroot.c
new file mode 100644
index 0000000..e1c1e70
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-bad-sysroot.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+
+int
+main (void)
+{
+  sleep (600);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/attach-bad-sysroot.exp b/gdb/testsuite/gdb.base/attach-bad-sysroot.exp
new file mode 100644
index 0000000..014da65
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-bad-sysroot.exp
@@ -0,0 +1,36 @@
+# Copyright (C) 2011-2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+standard_testfile .c
+if { [build_executable ${testfile}.exp $binfile "${testfile}.c"] == -1 } {
+    untested ${testfile}.exp
+    return -1
+}
+
+set test_spawn_id [spawn_wait_for_attach $binfile]
+set testpid [spawn_id_get_pid $test_spawn_id]
+
+set outdir [make_gdb_parallel_path outputs $subdir $testfile]
+set sysroot ${outdir}/does-not-exist
+
+gdb_start
+gdb_test_no_output "set sysroot $sysroot"
+gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\."
+
+kill_wait_spawned_process $test_spawn_id
-- 
1.7.1

Re: [PATCH] Fix logic in exec_file_locate_attach

[Pedro Alves]

On 02/18/2016 05:05 PM, Gary Benson wrote:

> 
> 	* exec.c (exec_file_locate_attach): Throw error if
> 	exec_file_find fails to locate the main executable.

This goes back to:
  https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html

Why is this an error, that even makes us stop the attach process
halfway, if the case when we don't know the file name is completely
silent? :

void
exec_file_locate_attach (int pid, int from_tty)
{
...
  /* Try to determine a filename from the process itself.  */
  exec_file = target_pid_to_exec_file (pid);
  if (exec_file == NULL)
    return;

> +
> +set test_spawn_id [spawn_wait_for_attach $binfile]
> +set testpid [spawn_id_get_pid $test_spawn_id]
> +
> +set outdir [make_gdb_parallel_path outputs $subdir $testfile]
> +set sysroot ${outdir}/does-not-exist
> +
> +gdb_start
> +gdb_test_no_output "set sysroot $sysroot"

Does this "$sysroot" expand to an absolute path?  If so,
the test message depends on where in the filesystem you
happen to run the testsuite.  So this needs an explicit
test message, or maybe simply:

  gdb_test_no_output "set sysroot /dev/null"

> +gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\."
> +
> +kill_wait_spawned_process $test_spawn_id
> 

Thanks,
Pedro Alves

Re: [PATCH] Fix logic in exec_file_locate_attach

[Gary Benson]

Pedro Alves wrote:
> On 02/18/2016 05:05 PM, Gary Benson wrote:
> > 	* exec.c (exec_file_locate_attach): Throw error if
> > 	exec_file_find fails to locate the main executable.
> 
> This goes back to:
>   https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html
> 
> Why is this an error, that even makes us stop the attach process
> halfway, if the case when we don't know the file name is completely
> silent? :

Hmmm, I was trying to fix a test failure, but looking at it with fresh
eyes this morning it would've been better to fix up that test as the
resulting state is more usable without a throw.  This is with throw:

  (gdb) set sysroot /whatever
  (gdb) attach 31954
  Attaching to process 31954
  ../attach-bad-sysroot: in sysroot "/whatever": No such file or directory.
  (gdb) bt
  #0  0xb54aca20 in ?? ()
  Backtrace stopped: Cannot access memory at address 0x25ae1df8
  (gdb)

This is without throw:

  (gdb) set sysroot /whatever
  (gdb) attach 31954
  Attaching to process 31954
  warning: Could not load vsyscall page because no executable was specified
  try using the "file" command first.
  0x00000039b54aca20 in ?? ()
  (gdb) bt
  #0  0x00000039b54aca20 in ?? ()
  #1  0x00000039b54ac8b0 in ?? ()
  #2  0x0000000000000000 in ?? ()
  (gdb) 

So without the throw you can backtrace.  Also, not throwing will I
think fix things for Luis without adding TRY..CATCH in remote.c.
I'll rework and resubmit.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Fix logic in exec_file_locate_attach

[Luis Machado]

On 02/19/2016 08:24 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 02/18/2016 05:05 PM, Gary Benson wrote:
>>> 	* exec.c (exec_file_locate_attach): Throw error if
>>> 	exec_file_find fails to locate the main executable.
>>
>> This goes back to:
>>    https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html
>>
>> Why is this an error, that even makes us stop the attach process
>> halfway, if the case when we don't know the file name is completely
>> silent? :
>
> Hmmm, I was trying to fix a test failure, but looking at it with fresh
> eyes this morning it would've been better to fix up that test as the
> resulting state is more usable without a throw.  This is with throw:
>
>    (gdb) set sysroot /whatever
>    (gdb) attach 31954
>    Attaching to process 31954
>    ../attach-bad-sysroot: in sysroot "/whatever": No such file or directory.
>    (gdb) bt
>    #0  0xb54aca20 in ?? ()
>    Backtrace stopped: Cannot access memory at address 0x25ae1df8
>    (gdb)
>
> This is without throw:
>
>    (gdb) set sysroot /whatever
>    (gdb) attach 31954
>    Attaching to process 31954
>    warning: Could not load vsyscall page because no executable was specified
>    try using the "file" command first.
>    0x00000039b54aca20 in ?? ()
>    (gdb) bt
>    #0  0x00000039b54aca20 in ?? ()
>    #1  0x00000039b54ac8b0 in ?? ()
>    #2  0x0000000000000000 in ?? ()
>    (gdb)
>
> So without the throw you can backtrace.  Also, not throwing will I
> think fix things for Luis without adding TRY..CATCH in remote.c.
> I'll rework and resubmit.

For the record, i'm re-working my previous patch to handle the case of 
native attaches too, when the file access problem cuts the attachment 
process halfway through (regardless of sysroot use policies).

I'm playing around with the TRY/CATCH block inside 
exec_file_locate_attach as opposed to taking care of its callers (in 
remote.c and infcmd.c).

Currently i'm checking for regressions and writing a testcase. I just 
want to make sure our changes don't overlap in terms of logic.

[PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 0b8c077..90811c0 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -156,9 +156,12 @@ exec_file_locate_attach (int pid, int from_tty)
   /* If gdb_sysroot is not empty and the discovered filename
      is absolute then prefix the filename with gdb_sysroot.  */
   if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file))
-    full_exec_path = exec_file_find (exec_file, NULL);
-
-  if (full_exec_path == NULL)
+    {
+      full_exec_path = exec_file_find (exec_file, NULL);
+      if (full_exec_path == NULL)
+	return;
+    }
+  else
     {
       /* It's possible we don't have a full path, but rather just a
 	 filename.  Some targets, such as HP-UX, don't provide the
diff --git a/gdb/testsuite/gdb.base/attach-pie-noexec.exp b/gdb/testsuite/gdb.base/attach-pie-noexec.exp
index f3e693a..1a51049 100644
--- a/gdb/testsuite/gdb.base/attach-pie-noexec.exp
+++ b/gdb/testsuite/gdb.base/attach-pie-noexec.exp
@@ -60,7 +60,7 @@ set testpid [spawn_id_get_pid $test_spawn_id]
 
 gdb_start
 file delete -- $binfile
-gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\." "attach"
+gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*" "attach"
 gdb_test "set architecture $arch" "The target architecture is assumed to be $arch"
 gdb_test "info shared" "From\[ \t\]+To\[ \t\]+Syms Read\[ \t\]+Shared Object Library\r\n0x.*"
 
-- 
1.7.1

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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.

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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

-- 
http://gbenson.net/

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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.

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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

-- 
http://gbenson.net/

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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.

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Pedro Alves]

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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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?

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Pedro Alves]

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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Luis Machado]

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.

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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/

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Pedro Alves]

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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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

-- 
http://gbenson.net/

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Pedro Alves]

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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[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

Re: [PATCH v2] Fix logic in exec_file_locate_attach

[Gary Benson]

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

-- 
http://gbenson.net/

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/11/2016 02:19 PM, Luis Machado wrote:

> gdb/ChangeLog:
> 
> 2016-02-11  Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* remote.c (remote_add_inferior): Guard block that can throw
> 	errors.

So the question is: why guard this call, and not the others?

E.g., I'd think that failing to find the executable in the sysroot
shouldn't error out of "attach" either.

Thanks,
Pedro Alves

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

On 02/12/2016 01:29 PM, Pedro Alves wrote:
> On 02/11/2016 02:19 PM, Luis Machado wrote:
>
>> gdb/ChangeLog:
>>
>> 2016-02-11  Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* remote.c (remote_add_inferior): Guard block that can throw
>> 	errors.
>
> So the question is: why guard this call, and not the others?
>
> E.g., I'd think that failing to find the executable in the sysroot
> shouldn't error out of "attach" either.

I did not exercise that, but did not discard it either. I was attempting 
to solve one problem at a time. There may be a failure there too.

If you're attempting to attach to a remote process via a remote 
gdbserver, then GDB is already connected to gdbserver by the time it 
issues a vAttach request, no?

If so, then it is not as problematic as not being able to connect to the 
target at all, but still wrong.

In any case, GDB should honor the user option of not using a symbol file 
at all for debugging processes that are already running.

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/12/2016 04:08 PM, Luis Machado wrote:

> I did not exercise that, but did not discard it either. I was attempting 
> to solve one problem at a time. There may be a failure there too.

I matters to determine where the TRY/CATCH should go, or even whether
the fix should be to not throw in the first place.

Like, with:

(gdb) define foo
>  attach PID
>  info threads
> end
(gdb) foo

should failing to load the executable error out before
reaching "info threads", or continue.

I think it should not error out, like we don't error out
if the target doesn't support target_pid_to_exec_file
at all.

Thanks,
Pedro Alves

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

On 02/12/2016 02:36 PM, Pedro Alves wrote:
> On 02/12/2016 04:08 PM, Luis Machado wrote:
>
>> I did not exercise that, but did not discard it either. I was attempting
>> to solve one problem at a time. There may be a failure there too.
>
> I matters to determine where the TRY/CATCH should go, or even whether
> the fix should be to not throw in the first place.
>

Ok, i crafted a test application that i can attach to via a remote 
gdbserver and through the extended remote protocol.

I see the same error as before. On GDB's side, for example:

--
/lib64/ld-2.22.so: No such file or directory.
(gdb)
--

But it seems we have already attached to the process by the time we 
error out. So we remain attached to it, just not sure if in a sane state:

--
(gdb) info proc
process 5464
warning: target file /proc/5464/cmdline contained unexpected null characters
cmdline = '/lib64/ld.so.1'
cwd = '/home/tester'
exe = '/lib64/ld-2.22.so'
(gdb) i r
           zero       at       v0       v1       a0       a1       a2 
     a3
  R0   000013aa 00000014 00000204 00000000 ffa831f8 ffa831f8 00000000 
00000001
             t0       t1       t2       t3       t4       t5       t6 
     t7
  R8   ffa83060 557da7e0 00000000 015555f7 00000000 00000000 801123d8 
00000000
             s0       s1       s2       s3       s4       s5       s6 
     s7
  R16  ffa83178 00020000 ffa830f8 00000000 00503bd8 00000000 004ee308 
00000000
             t8       t9       k0       k1       gp       sp       s8 
     ra
  R24  00000038 55700e4c 00000000 00000000 557da7e0 ffa83000 ffa83240 
55700ba0
         status       lo       hi badvaddr    cause       pc
       0400a4f3 00000000 00000017 557a20e4 00800020 55700e78
fcsr: 0x0
fir: 0x173890c
restart: 0x13aa
(gdb)
--

> Like, with:
>
> (gdb) define foo
>>   attach PID
>>   info threads
>> end
> (gdb) foo
>
> should failing to load the executable error out before
> reaching "info threads", or continue.
>
> I think it should not error out, like we don't error out
> if the target doesn't support target_pid_to_exec_file
> at all.

Agreed. In this case "info threads" doesn't run at all.

>
> Thanks,
> Pedro Alves
>

Re: [PATCH] Remote debugging without a binary (regression)

[Pedro Alves]

On 02/12/2016 05:30 PM, Luis Machado wrote:
>> > I think it should not error out, like we don't error out
>> > if the target doesn't support target_pid_to_exec_file
>> > at all.
> Agreed. In this case "info threads" doesn't run at all.
> 

That then suggests to me that either the TRY/CATCH should be
somewhere inside exec_file_locate_attach instead of wrapping one
particular call, or, exec_file_locate_attach shouldn't be throwing
in the first place.

Thanks,
Pedro Alves

Re: [PATCH] Remote debugging without a binary (regression)

[Gary Benson]

Hi Luis,

Luis Machado wrote:
> The following patch guards such a call to prevent those errors
> from disrupting a potential debugging session, and display only
> a warning.

After looking into this it seems your patch is the right way to
go.  There is a separate bug in exec_file_locate_attach in that
in some cases the sysroot is ignored, but fixing that will not
remove the need for what you are proposing.

My only nit is that I would prefer the TRY-CATCH block smaller,
covering just exec_file_locate_attach, like so:

  /* If no main executable is currently open then attempt to
     open the file that was executed to create this inferior.  */
  if (try_open_exec && get_exec_file (0) == NULL)
    {
      /* exec_file_locate_attach may throw an error...
      TRY
        {
          exec_file_locate_attach (pid, 1);
        }
      CATCH...

Other than that I am ok with this change.

Thanks for tracking this down.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Remote debugging without a binary (regression)

[Luis Machado]

Hi Gary,

On 02/18/2016 10:30 AM, Gary Benson wrote:
> Hi Luis,
>
> Luis Machado wrote:
>> The following patch guards such a call to prevent those errors
>> from disrupting a potential debugging session, and display only
>> a warning.
>
> After looking into this it seems your patch is the right way to
> go.  There is a separate bug in exec_file_locate_attach in that
> in some cases the sysroot is ignored, but fixing that will not
> remove the need for what you are proposing.
>
> My only nit is that I would prefer the TRY-CATCH block smaller,
> covering just exec_file_locate_attach, like so:
>
>    /* If no main executable is currently open then attempt to
>       open the file that was executed to create this inferior.  */
>    if (try_open_exec && get_exec_file (0) == NULL)
>      {
>        /* exec_file_locate_attach may throw an error...
>        TRY
>          {
>            exec_file_locate_attach (pid, 1);
>          }
>        CATCH...
>
> Other than that I am ok with this change.
>
> Thanks for tracking this down.

No problem. I'll come up with a new version of the patch and, while at 
it, i'll try Pedro's suggestion for the testcase.