[PATCH v2] Avoid crash when calling warning too early

[PATCH v2] Avoid crash when calling warning too early

[Tom Tromey]

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

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

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

[Sergio Durigan Junior]

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/

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

[Tom Tromey]

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

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

[Sergio Durigan Junior]

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

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

[Tom Tromey]

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

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

[Pedro Alves]

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

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

[Tom Tromey]

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

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

[Tom Tromey]

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

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

I didn't address checking the warning output, but here's a patch for the
rest.  Let me know what you think.

Tom

commit 42cbd945dddcc019af24b75aa353fd42d06b17a6
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Nov 15 16:23:25 2018 -0700

    Use save_vars in gdb.base/warning.exp
    
    This changes gdb.base/warning.exp per Pedro's comments: it now uses
    save_vars and it rewrites INTERNAL_GDBFLAGS in place, rather than
    replcing it entirely.
    
    gdb/testsuite/ChangeLog
    2018-11-15  Tom Tromey  <tom@tromey.com>
    
            * gdb.base/warning.exp: Use save_vars.  Rewrite INTERNAL_GDBFLAGS
            rather than replacing it.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 90faeda261..8ad17ca5c5 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-15  Tom Tromey  <tom@tromey.com>
+
+	* gdb.base/warning.exp: Use save_vars.  Rewrite INTERNAL_GDBFLAGS
+	rather than replacing it.
+
 2018-11-12  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* gdb.base/skip.exp: Add standard_testfile.  Add "skip delete"
diff --git a/gdb/testsuite/gdb.base/warning.exp b/gdb/testsuite/gdb.base/warning.exp
index 53067f48d5..975d994e30 100644
--- a/gdb/testsuite/gdb.base/warning.exp
+++ b/gdb/testsuite/gdb.base/warning.exp
@@ -25,12 +25,28 @@ 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
+save_vars { INTERNAL_GDBFLAGS } {
+    set last_was_dd 0
+    set new_list {}
+    foreach opt [split $INTERNAL_GDBFLAGS] {
+	if {$last_was_dd} {
+	    # Just drop it.
+	    set last_was_dd 0
+	} elseif {[regexp -- "^--?data-directory\$" $opt]} {
+	    set last_was_dd 1
+	} elseif {[regexp -- "^--?data-directory=" $opt]} {
+	    # Drop but don't drop the next argument.
+	} else {
+	    lappend new_list $opt
+	}
+    }
+    lappend new_list -data-directory
+    lappend new_list $tname
+
+    set INTERNAL_GDBFLAGS [join $new_list]
+
+    gdb_start
+
+    # Make sure gdb started up.
+    gdb_test "echo 23\\n" "23"
+}

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

[Pedro Alves]

On 11/15/2018 11:15 PM, Tom Tromey wrote:
>>>>>> "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?
Hmm, by using gdb_spawn directly, or better, gdb_spawn_with_cmdline_opts
instead of gdb_start?

See patch below; it works here.

Also, looking further into the INTERNAL_GDBFLAGS issue,
it turns out we don't really need to frob INTERNAL_GDBFLAGS.
Each passed -data-directory option leads to a call to the warning:

 $ ./gdb -data-directory=foo -data-directory=bar
 Warning: foo: No such file or directory.
 Warning: bar: No such file or directory.
 [...]

(And if it didn't, I'd think we'd just have to make sure to put
our -data-directory last, leaving the original one in place.
I.e., just passing it in gdb_spawn_with_cmdline_opts should work
even in that case.)

So, WDYT of this version?

From 7f3e6bf1b75e049c0abbab2a9cbf3d6e28f855ac Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 16 Nov 2018 13:13:56 +0000
Subject: [PATCH] warning

---
 gdb/testsuite/gdb.base/warning.exp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/warning.exp b/gdb/testsuite/gdb.base/warning.exp
index 53067f48d5..134218acb9 100644
--- a/gdb/testsuite/gdb.base/warning.exp
+++ b/gdb/testsuite/gdb.base/warning.exp
@@ -25,12 +25,12 @@ set fd [open $tname w]
 puts $fd "anything"
 close $fd
 
-set save $INTERNAL_GDBFLAGS
-set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
+gdb_spawn_with_cmdline_opts \
+    "-iex \"set pagination off\" -data-directory $tname"
 
-gdb_start
+# Make sure we see the warning.
+gdb_test "" "warning: $tname is not a directory.*" \
+    "got warning"
 
 # Make sure gdb started up.
 gdb_test "echo 23\\n" "23"
-
-set INTERNAL_GDBFLAGS $save
-- 
2.14.4

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

[Tom Tromey]

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

Pedro> Hmm, by using gdb_spawn directly, or better, gdb_spawn_with_cmdline_opts
Pedro> instead of gdb_start?

[...]

Pedro> So, WDYT of this version?

Look good to me, thanks.

Tom

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

[Pedro Alves]

On 11/18/2018 06:46 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Hmm, by using gdb_spawn directly, or better, gdb_spawn_with_cmdline_opts
> Pedro> instead of gdb_start?
> 
> [...]
> 
> Pedro> So, WDYT of this version?
> 
> Look good to me, thanks.
> 
I've pushed it in now.

From 6769f2765db0d94eeb8437b41a925e2bd871f514 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 19 Nov 2018 15:08:46 +0000
Subject: [PATCH] gdb.base/warning.exp tweaks

#1- Check that the warning is emitted.

#2- Avoid overriding INTERNAL_GDBFLAGS, as per documentated in
    gdb/testsuite/README:

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

We don't actually need to tweak INTERNAL_GDBFLAGS, we just need to
append out -data-directory to GDBFLAGS, because each passed
-data-directory option leads to a call to the warning:

 $ ./gdb -data-directory=foo -data-directory=bar
 Warning: foo: No such file or directory.
 Warning: bar: No such file or directory.
 [...]

gdb/ChangeLog
2018-11-19  Pedro Alves  <palves@redhat.com>

	* gdb.base/warning.exp: Don't override INTERNAL_FLAGS.  Use
	gdb_spawn_with_cmdline_opts instead of gdb_start.  Check that we
	see the expected warning.
---
 gdb/testsuite/ChangeLog            |  6 ++++++
 gdb/testsuite/gdb.base/warning.exp | 10 +++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c0de6f2d6c..9acaa79e2b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-11-19  Pedro Alves  <palves@redhat.com>
+
+	* gdb.base/warning.exp: Don't override INTERNAL_FLAGS.  Use
+	gdb_spawn_with_cmdline_opts instead of gdb_start.  Check that we
+	see the expected warning.
+
 2018-11-16  Alan Hayward  <alan.hayward@arm.com>
 
 	PR gdb/22736:
diff --git a/gdb/testsuite/gdb.base/warning.exp b/gdb/testsuite/gdb.base/warning.exp
index 53067f48d5..134218acb9 100644
--- a/gdb/testsuite/gdb.base/warning.exp
+++ b/gdb/testsuite/gdb.base/warning.exp
@@ -25,12 +25,12 @@ set fd [open $tname w]
 puts $fd "anything"
 close $fd
 
-set save $INTERNAL_GDBFLAGS
-set INTERNAL_GDBFLAGS "-nw -nx -data-directory $tname"
+gdb_spawn_with_cmdline_opts \
+    "-iex \"set pagination off\" -data-directory $tname"
 
-gdb_start
+# Make sure we see the warning.
+gdb_test "" "warning: $tname is not a directory.*" \
+    "got warning"
 
 # Make sure gdb started up.
 gdb_test "echo 23\\n" "23"
-
-set INTERNAL_GDBFLAGS $save
-- 
2.14.4