[v2] Avoid crash when calling warning too early

  I noticed that if you pass the name of an existing file (not a
directory) as the argument to --data-directory, gdb will crash:

    $ ./gdb -nx  --data-directory  ./gdb
    ../../binutils-gdb/gdb/target.c:590:56: runtime error: member call on null pointer of type 'struct target_ops'

This was later reported as PR gdb/23838.

This happens because warning ends up calling
target_supports_terminal_ours, which calls current_top_target, which
returns nullptr this early.

This fixes the problem by handling this case specially in
target_supports_terminal_ours.  I also changed
target_supports_terminal_ours to return bool.

gdb/ChangeLog
2018-10-31  Tom Tromey  <tom@tromey.com>

	PR gdb/23838:
	* target.h (target_supports_terminal_ours): Return bool.
	* target.c (target_supports_terminal_ours): Handle case where
	current_top_target returns nullptr.  Return bool.

gdb/testsuite/ChangeLog
2018-10-31  Tom Tromey  <tom@tromey.com>

	PR gdb/23838:
	* gdb.base/warning.exp: New file.
---
 gdb/ChangeLog                      |  7 ++++++
 gdb/target.c                       | 10 +++++++--
 gdb/target.h                       |  2 +-
 gdb/testsuite/ChangeLog            |  5 +++++
 gdb/testsuite/gdb.base/warning.exp | 36 ++++++++++++++++++++++++++++++
 5 files changed, 57 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/warning.exp
  

On Wednesday, October 31 2018, Tom Tromey wrote:

> I noticed that if you pass the name of an existing file (not a
> directory) as the argument to --data-directory, gdb will crash:
>
>     $ ./gdb -nx  --data-directory  ./gdb
>     ../../binutils-gdb/gdb/target.c:590:56: runtime error: member call on null pointer of type 'struct target_ops'
>
> This was later reported as PR gdb/23838.

Thanks for the patch, Tom.

> This happens because warning ends up calling
> target_supports_terminal_ours, which calls current_top_target, which
> returns nullptr this early.
>
> This fixes the problem by handling this case specially in
> target_supports_terminal_ours.  I also changed
> target_supports_terminal_ours to return bool.

This seems OK to me, and was also what part of what I was planning to do
to fix:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23555

Could you please also mention this bug on the ChangeLog?  Your patch
doesn't fully fix the issue, but it avoids the segfault at least.

Thanks,

> gdb/ChangeLog
> 2018-10-31  Tom Tromey  <tom@tromey.com>
>
> 	PR gdb/23838:
> 	* target.h (target_supports_terminal_ours): Return bool.
> 	* target.c (target_supports_terminal_ours): Handle case where
> 	current_top_target returns nullptr.  Return bool.
>
> gdb/testsuite/ChangeLog
> 2018-10-31  Tom Tromey  <tom@tromey.com>
>
> 	PR gdb/23838:
> 	* gdb.base/warning.exp: New file.
> ---
>  gdb/ChangeLog                      |  7 ++++++
>  gdb/target.c                       | 10 +++++++--
>  gdb/target.h                       |  2 +-
>  gdb/testsuite/ChangeLog            |  5 +++++
>  gdb/testsuite/gdb.base/warning.exp | 36 ++++++++++++++++++++++++++++++
>  5 files changed, 57 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/warning.exp
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 2d98954b54..9404025104 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -584,10 +584,16 @@ target_terminal::info (const char *arg, int from_tty)
>  
>  /* See target.h.  */
>  
> -int
> +bool
>  target_supports_terminal_ours (void)
>  {
> -  return current_top_target ()->supports_terminal_ours ();
> +  /* This can be called before there is any target, so we must check
> +     for nullptr here.  */
> +  target_ops *top = current_top_target ();
> +
> +  if (top == nullptr)
> +    return false;
> +  return top->supports_terminal_ours ();
>  }
>  
>  static void
> diff --git a/gdb/target.h b/gdb/target.h
> index a3000c80c6..6f4b73bf00 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1577,7 +1577,7 @@ extern int target_remove_breakpoint (struct gdbarch *gdbarch,
>  /* Return true if the target stack has a non-default
>    "terminal_ours" method.  */
>  
> -extern int target_supports_terminal_ours (void);
> +extern bool target_supports_terminal_ours (void);
>  
>  /* Kill the inferior process.   Make it go away.  */
>  
> diff --git a/gdb/testsuite/gdb.base/warning.exp b/gdb/testsuite/gdb.base/warning.exp
> new file mode 100644
> index 0000000000..53067f48d5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/warning.exp
> @@ -0,0 +1,36 @@
> +# Copyright 2018 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/>.
> +
> +# Test that an early warning does not cause a crash.
> +
> +if {[is_remote host]} {
> +    unsupported "warning.exp can only run on local host"
> +    return
> +}
> +
> +set tname [standard_temp_file warning]
> +set fd [open $tname w]
> +puts $fd "anything"
> +close $fd
> +
> +set save $INTERNAL_GDBFLAGS
> +set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
> +
> +gdb_start
> +
> +# Make sure gdb started up.
> +gdb_test "echo 23\\n" "23"
> +
> +set INTERNAL_GDBFLAGS $save
> -- 
> 2.17.1
  

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

Sergio> This seems OK to me, and was also what part of what I was planning to do
Sergio> to fix:
Sergio>   https://sourceware.org/bugzilla/show_bug.cgi?id=23555
Sergio> Could you please also mention this bug on the ChangeLog?  Your patch
Sergio> doesn't fully fix the issue, but it avoids the segfault at least.

If you want, I can just drop this and you can take over.

Tom
  

On Wednesday, October 31 2018, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> This seems OK to me, and was also what part of what I was planning to do
> Sergio> to fix:
> Sergio>   https://sourceware.org/bugzilla/show_bug.cgi?id=23555
> Sergio> Could you please also mention this bug on the ChangeLog?  Your patch
> Sergio> doesn't fully fix the issue, but it avoids the segfault at least.
>
> If you want, I can just drop this and you can take over.

Thanks, but I'd prefer if you checked your patch in.  The bug reported
on 23555 is actually composed of two issues: (a) what to do when GDB
can't determine the CWD, and (b) the segmentation fault that happens
when we try to call warning too early in the code.  Your patch fixes
(b), and I still didn't sit down to figure out how to approach (a).  So
it's best if we just go ahead and avoid the segfault for now.

Thanks,
  

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

>> If you want, I can just drop this and you can take over.

Sergio> Thanks, but I'd prefer if you checked your patch in.  The bug reported
Sergio> on 23555 is actually composed of two issues: (a) what to do when GDB
Sergio> can't determine the CWD, and (b) the segmentation fault that happens
Sergio> when we try to call warning too early in the code.  Your patch fixes
Sergio> (b), and I still didn't sit down to figure out how to approach (a).  So
Sergio> it's best if we just go ahead and avoid the segfault for now.

I'm going to check it in now, with the ChangeLog change you suggested.

Tom
  

On 10/31/2018 06:18 PM, Tom Tromey wrote:
> +set save $INTERNAL_GDBFLAGS
> +set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
> +
> +gdb_start
> +
> +# Make sure gdb started up.
> +gdb_test "echo 23\\n" "23"

I guess this could also check that the warning was emitted?

> +
> +set INTERNAL_GDBFLAGS $save

FYI, you can use:

save_vars { INTERNAL_GDBFLAGS } {
   set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
   ...
}

But, note that gdb/testsuite/README says this about
INTERNAL_GDBFLAGS:

 ~~~ 
 The testsuite does not override a value provided by the user.
 ~~~

I think that the test should instead be replacing/sed'ing
the existing -data-directory option, instead of overriding
INTERNAL_GDBFLAGS completely.
That's what gdb.base/gdbinit-history.exp does.

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 10/31/2018 06:18 PM, Tom Tromey wrote:
>> +set save $INTERNAL_GDBFLAGS
>> +set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
>> +
>> +gdb_start
>> +
>> +# Make sure gdb started up.
>> +gdb_test "echo 23\\n" "23"

Pedro> I guess this could also check that the warning was emitted?

I didn't know how to do that.
Modify gdb_start maybe?

Tom