[RFC] Avoid crash when calling warning too early

[RFC] Avoid crash when calling warning too early

[Tom Tromey]

I happened to notice 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 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 wasn't sure whether this warranted a test case, hence the RFC.

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

	* target.c (target_supports_terminal_ours): Handle case where
	current_top_target returns nullptr.
---
 gdb/ChangeLog | 5 +++++
 gdb/target.c  | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/gdb/target.c b/gdb/target.c
index 2d98954b54..a261155f29 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -587,6 +587,11 @@ target_terminal::info (const char *arg, int from_tty)
 int
 target_supports_terminal_ours (void)
 {
+  /* 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 current_top_target ()->supports_terminal_ours ();
 }
 
-- 
2.17.1

Re: [RFC] Avoid crash when calling warning too early

[Sergio Durigan Junior]

On Saturday, October 06 2018, Tom Tromey wrote:

> I happened to notice 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 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 wasn't sure whether this warranted a test case, hence the RFC.

Thanks for the patch.  I remember stumbling upon this issue a while ago,
and had a similar patch to fix it, but I think I forgot to submit it.

> gdb/ChangeLog
> 2018-10-06  Tom Tromey  <tom@tromey.com>
>
> 	* target.c (target_supports_terminal_ours): Handle case where
> 	current_top_target returns nullptr.
> ---
>  gdb/ChangeLog | 5 +++++
>  gdb/target.c  | 5 +++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 2d98954b54..a261155f29 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -587,6 +587,11 @@ target_terminal::info (const char *arg, int from_tty)
>  int
>  target_supports_terminal_ours (void)
>  {
> +  /* 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 current_top_target ()->supports_terminal_ours ();
>  }

The patch looks good to me.  My only question is about whether we still
require a newline between variable declarations and the rest of the
code.  I still follow this rule (because I think it improves code
readability), but now with C++11 I'm not sure if it's still being
enforced.

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: [RFC] Avoid crash when calling warning too early

[Tom Tromey]

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

Sergio> The patch looks good to me.  My only question is about whether we still
Sergio> require a newline between variable declarations and the rest of the
Sergio> code.  I still follow this rule (because I think it improves code
Sergio> readability), but now with C++11 I'm not sure if it's still being
Sergio> enforced.

I don't know, but I suppose so, and in any case I went ahead and added
it.

Tom