[RFC] Avoid crash when calling warning too early

Message ID 20181006192007.1945-1-tom@tromey.com
State New
Headers show
Series
  • [RFC] Avoid crash when calling warning too early
Related show

Commit Message

Tom Tromey Oct. 6, 2018, 7:20 p.m.
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(+)

-- 
2.17.1

Comments

Sergio Durigan Junior Oct. 6, 2018, 7:32 p.m. | #1
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/
Tom Tromey Oct. 6, 2018, 9:13 p.m. | #2
>>>>> "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

Patch

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