Guard against 'current_directory == NULL' on gdb_abspath (PR gdb/23613)

Message ID 20190710234615.14800-1-sergiodj@redhat.com
State New
Headers show
Series
  • Guard against 'current_directory == NULL' on gdb_abspath (PR gdb/23613)
Related show

Commit Message

Sergio Durigan Junior July 10, 2019, 11:46 p.m.
Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1728147
Ref.: https://sourceware.org/bugzilla/show_bug.cgi?id=23613

Hi,

This bug has been reported a few days ago against Fedora GDB.  The
problem reported is that GDB segfaults when the working directory is
deleted.  It's pretty use to reproduce it:

  mkdir bla
  cd bla
  rmdir ../bla
  gdb echo

Debugging the problem is a bit tricky, because, since the current
directory doesn't exist anymore, a corefile cannot be saved there.
After a few attempts, I came up with the following:

  gdb -ex 'shell mkdir bla' -ex 'cd bla' -ex 'shell rmdir ../bla' -ex 'r echo' ./gdb/gdb

This assumes that you're inside a build directory which contains
./gdb/gdb, of course.

After investigating it, I found that the problem happens at
gdb_abspath, where we're dereferencing 'current_directory' without
checking if it's NULL:

    ...
    (concat (current_directory,
	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
	     ? "" : SLASH_STRING,
    ...

So I fixed the problem with the patch below.  The idea is that, if
'current_directory' is NULL, then the final string returned should be
just the "path".

After fixing the bug, I found a similar one reported against our
bugzilla: PR gdb/23613.  The problem is the same, but the reproducer
is a bit different.

I really tried writing a testcase for this, but unfortunately it's
apparently not possible to start GDB inside a non-existent directory
with DejaGNU.

I regression tested this patch on the BuildBot, and no regressions
were found.

gdb/ChangeLog:
2019-07-10  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1728147
	PR gdb/23613
	* gdbsupport/pathstuff.c (gdb_abspath): Guard against
	'current_directory == NULL' case.
---
 gdb/gdbsupport/pathstuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.21.0

Comments

Tom Tromey July 11, 2019, 4:05 p.m. | #1
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:


Sergio> So I fixed the problem with the patch below.  The idea is that, if
Sergio> 'current_directory' is NULL, then the final string returned should be
Sergio> just the "path".

I found other spots relying on current_directory in this way, so I
wonder if this is the best approach.

For example, corelow.c:

  gdb::unique_xmalloc_ptr<char> filename (tilde_expand (arg));
  if (!IS_ABSOLUTE_PATH (filename.get ()))
    filename.reset (concat (current_directory, "/",
			    filename.get (), (char *) NULL));

I guess this should just be converted to gdb_abspath?

... but also code in source.c and some other spots.


If this approach is the right one -- and it seems like it could be --
then the function documentation should at least be updated to reflect
this error case.

Tom
Sergio Durigan Junior July 12, 2019, 1:21 a.m. | #2
Thanks for the review, Tom.

On Thursday, July 11 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>

> Sergio> So I fixed the problem with the patch below.  The idea is that, if

> Sergio> 'current_directory' is NULL, then the final string returned should be

> Sergio> just the "path".

>

> I found other spots relying on current_directory in this way, so I

> wonder if this is the best approach.

>

> For example, corelow.c:

>

>   gdb::unique_xmalloc_ptr<char> filename (tilde_expand (arg));

>   if (!IS_ABSOLUTE_PATH (filename.get ()))

>     filename.reset (concat (current_directory, "/",

> 			    filename.get (), (char *) NULL));

>

> I guess this should just be converted to gdb_abspath?


Yeah, agreed, this is doing the same thing that gdb_abspath does, but
without the proper checks.

> ... but also code in source.c and some other spots.


I will make sure to grep for "current_directory" and adjust the code
where it's necessary.

> If this approach is the right one -- and it seems like it could be --

> then the function documentation should at least be updated to reflect

> this error case.


I'll do that as well, good idea.

As I mentioned on IRC, I think this is the best solution we have for
now.  It's really not possible to do much without a current directory
here.

I'll send v2 soon.  Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Patch

diff --git a/gdb/gdbsupport/pathstuff.c b/gdb/gdbsupport/pathstuff.c
index fafecd543d..aa51e8f36e 100644
--- a/gdb/gdbsupport/pathstuff.c
+++ b/gdb/gdbsupport/pathstuff.c
@@ -134,7 +134,7 @@  gdb_abspath (const char *path)
   if (path[0] == '~')
     return gdb_tilde_expand_up (path);
 
-  if (IS_ABSOLUTE_PATH (path))
+  if (IS_ABSOLUTE_PATH (path) || current_directory == NULL)
     return make_unique_xstrdup (path);
 
   /* Beware the // my son, the Emacs barfs, the botch that catch...  */