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

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

>>>>> "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
  

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,