Re: [PATCH v2] Avoid crash when calling warning too early

[Sergio Durigan Junior <sergiodj@redhat.com>]

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