[patch] Fix crash on NULL rl_prompt

[patch] Fix crash on NULL rl_prompt

[Jan Kratochvil]

Hi,

https://bugzilla.redhat.com/attachment.cgi?id=401527

#1  in xstrdup (s=0x0) at ../../libiberty/xstrdup.c:33
#2  in tui_prep_terminal (notused1=1) at ../../gdb/tui/tui-io.c:292
#3  in _rl_callback_newline () at ../callback.c:82
#4  in gdb_do_one_event (data=0x0) at ../../gdb/event-loop.c:468
#5  in catch_errors (func=0x8177e60 <gdb_do_one_event>, 
#6  in tui_command_loop
#7  in current_interp_command_loop
#8  in captured_command_loop
#9  in catch_errors
#10 in captured_main
#11 in catch_errors
#12 in gdb_main
#13 in main

I have not found how to reproduce it, normally when GDB calls
tui_prep_terminal it has rl_prompt set to non-NULL.  But NULL rl_prompt is
a valid state for readline and GDB itself even sets it temporarily to NULL in
tui_setup_io (1) (just tui_prep_terminal is not called in such case) so I find
it fragile to crash on NULL rl_prompt.  GDB can handle NULL
tui_rl_saved_prompt.

BTW don't you think xstrdup (NULL) should == NULL?  Like xmalloc (0) == NULL.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


2010-03-30  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* tui/tui-io.c (tui_prep_terminal): Permit NULL rl_prompt.

--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -289,7 +289,10 @@ tui_prep_terminal (int notused1)
      (we can't use gdb_prompt() due to secondary prompts and can't use
      rl_prompt because it points to an alloca buffer).  */
   xfree (tui_rl_saved_prompt);
-  tui_rl_saved_prompt = xstrdup (rl_prompt);
+  if (rl_prompt)
+    tui_rl_saved_prompt = xstrdup (rl_prompt);
+  else
+    tui_rl_saved_prompt = NULL;
 }
 
 /* Readline callback to restore the terminal.  It is called once each

Re: [patch] Fix crash on NULL rl_prompt

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> BTW don't you think xstrdup (NULL) should == NULL?  Like xmalloc
Jan> (0) == NULL.

I personally don't care either way.  If you want to do it, that is fine
by me.

Jan> 2010-03-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* tui/tui-io.c (tui_prep_terminal): Permit NULL rl_prompt.

Ok.

Tom

Re: [patch] Fix crash on NULL rl_prompt

[Pedro Alves]

On Tuesday 30 March 2010 16:57:28, Tom Tromey wrote:
> Jan> BTW don't you think xstrdup (NULL) should == NULL?  Like xmalloc
> Jan> (0) == NULL.

While it is implementation defined if malloc(0) returns NULL or
not, xmalloc(0) never returns NULL.  Quite the opposite.
It makes sure to never call malloc with size 0.
I don't think that xstrdup should special case NULL input.
It will tend to mask out bugs.  It isn't clear to me your
patch isn't doing so, FTR.  It could have been a readline
change only visible when using a more recent system
readline, instead of the version bundled with gdb, for
example.

-- 
Pedro Alves

Re: [patch] Fix crash on NULL rl_prompt

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> It isn't clear to me your patch isn't doing so, FTR.  It could
Pedro> have been a readline change only visible when using a more recent
Pedro> system readline, instead of the version bundled with gdb, for
Pedro> example.

The included readline handles rl_prompt == NULL:

int
rl_set_prompt (prompt)
     const char *prompt;
{
  FREE (rl_prompt);
  rl_prompt = prompt ? savestring (prompt) : (char *)NULL;
  rl_display_prompt = rl_prompt ? rl_prompt : "";

  rl_visible_prompt_length = rl_expand_prompt (rl_prompt);
  return 0;
}

Tom

Re: [patch] Fix crash on NULL rl_prompt

[Pedro Alves]

On Tuesday 30 March 2010 17:22:50, Tom Tromey wrote:
> Pedro> It isn't clear to me your patch isn't doing so, FTR.  It could
> Pedro> have been a readline change only visible when using a more recent
> Pedro> system readline, instead of the version bundled with gdb, for
> Pedro> example.
> 
> The included readline handles rl_prompt == NULL:

Yes, and doesn't leave rl_display_promp as NULL.  But,
how did rl_prompt end up NULL in the first place?  Was GDB running in
batch/non-tty stdin, "set editing off", with readline not setup
at all, and tried to enable the TUI/curses anyway?  That's
what I meant by masking out a bug.

-- 
Pedro Alves

Re: [patch] Fix crash on NULL rl_prompt

[Jan Kratochvil]

On Tue, 30 Mar 2010 18:41:20 +0200, Pedro Alves wrote:
> But, how did rl_prompt end up NULL in the first place?

I do not know.  I have spent some time trying to reproduce it reading the
source but gave up after some reasonable time.  Bugreport comes from an
automated crash reporter (ABRT) where the person only sometimes can/gives more
info.  Asked now for a reproducer.


> Was GDB running in batch/non-tty stdin, "set editing off", with readline not
> setup at all, and tried to enable the TUI/curses anyway?

Tried also this one with no luck (=with no crash).
	gdb -nx -ex 'set editing off' -ex 'tui reg general';echo $?


> That's what I meant by masking out a bug.

An existing bug a user cannot notice is ... no longer a bug, isn't it?


Thanks,
Jan

Re: [patch] Fix crash on NULL rl_prompt

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Pedro> But, how did rl_prompt end up NULL in the first place?

Jan> I do not know.

Yeah, it is not great that we don't know how to reproduce it.

But, given that part of the readline contract is that rl_prompt==NULL is
a valid state, I think tui_prep_terminal ought to cope with that as
well.  Maybe there is also some other bug somewhere else, but that
doesn't affect the correctness of this particular patch.

Pedro> That's what I meant by masking out a bug.

Jan> An existing bug a user cannot notice is ... no longer a bug, isn't it?

I'm sympathetic to this, but I would assume that however the user got
gdb into this weird state could also hypothetically reveal other latent
bugs.  I still think your fix is ok, but without a reproducer we can't
really be sure we've fixed the problem.

Tom

Re: [patch] Fix crash on NULL rl_prompt

[Pedro Alves]

On Tuesday 30 March 2010 18:04:57, Jan Kratochvil wrote:
> On Tue, 30 Mar 2010 18:41:20 +0200, Pedro Alves wrote:
> > But, how did rl_prompt end up NULL in the first place?
> 
> I do not know.  I have spent some time trying to reproduce it reading the
> source but gave up after some reasonable time.  Bugreport comes from an
> automated crash reporter (ABRT) where the person only sometimes can/gives more
> info.  Asked now for a reproducer.

My guess is, either readline wasn't setup proper at all, or,
the prompts stack got busted (get_prompt/set_prompt/PROMPT), which
I've seen happen before with target-async mode.

> An existing bug a user cannot notice is ... no longer a bug, isn't it?

Err, whatever.

(Nowhere in this thread have I seen mentioned that GDB (or its
prompt) doesn't get busted further down the road, mind you.)

On Tuesday 30 March 2010 18:13:20, Tom Tromey wrote:
> Jan> I do not know.
> 
> Yeah, it is not great that we don't know how to reproduce it.
> 
> But, given that part of the readline contract is that rl_prompt==NULL is
> a valid state, I think tui_prep_terminal ought to cope with that as
> well>.  Maybe there is also some other bug somewhere else, but that
> doesn't affect the correctness of this particular patch.

But isn't `rl_prompt' always built from input feed to readline?
GDB always gives readline a non-NULL prompt, from what I've seen.
Hence, I wouldn't be so fast in calling it correct, but I'm
not going to spend more time on this.  My intention was mainly
to comment on the xstrdup/xmalloc remark.

-- 
Pedro Alves

Re: [patch] Fix crash on NULL rl_prompt

[Jan Kratochvil]

On Tue, 30 Mar 2010 19:25:18 +0200, Pedro Alves wrote:
> GDB always gives readline a non-NULL prompt, from what I've seen.

gdb/tui/tui-io.c
tui_setup_io (int mode)
  if (mode)
      rl_prompt = 0;


Thanks,
Jan

Re: [patch] Fix crash on NULL rl_prompt

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> But isn't `rl_prompt' always built from input feed to readline?

Yes.

Pedro> GDB always gives readline a non-NULL prompt, from what I've seen.
Pedro> Hence, I wouldn't be so fast in calling it correct, but I'm
Pedro> not going to spend more time on this.

I see it as defensive programming.

We can always un-approve it, if you would rather wait to see if a
reproducer is available.

Tom

Re: [patch] Fix crash on NULL rl_prompt

[Pedro Alves]

On Tuesday 30 March 2010 18:29:40, Jan Kratochvil wrote:
> On Tue, 30 Mar 2010 19:25:18 +0200, Pedro Alves wrote:
> > GDB always gives readline a non-NULL prompt, from what I've seen.
> 
> gdb/tui/tui-io.c
> tui_setup_io (int mode)
>   if (mode)
>       rl_prompt = 0;
> 

Yes, but rl_set_prompt should be called shortly after, and before
tui_prep_terminal, so TUI displays a prompt as well.  Try
setting a watchpoint on rl_prompt, a break on tui_prep_terminal
and switching to the TUI and back.

On Tuesday 30 March 2010 18:30:48, Tom Tromey wrote:
> I see it as defensive programming.

Yes, of course.

> We can always un-approve it, if you would rather wait to see if a
> reproducer is available.

I'll leave it up to you to decide.  But if this goes in,
perhaps a comment saying this is being defensive would be good.

-- 
Pedro Alves

Re: [patch] Fix crash on NULL rl_prompt

[Pedro Alves]

On Tuesday 30 March 2010 18:25:18, Pedro Alves wrote:
> On Tuesday 30 March 2010 18:04:57, Jan Kratochvil wrote:
> > On Tue, 30 Mar 2010 18:41:20 +0200, Pedro Alves wrote:
> > > But, how did rl_prompt end up NULL in the first place?
> > 
> > I do not know.  I have spent some time trying to reproduce it reading the
> > source but gave up after some reasonable time.  Bugreport comes from an
> > automated crash reporter (ABRT) where the person only sometimes can/gives more
> > info.  Asked now for a reproducer.
> 
> My guess is, either readline wasn't setup proper at all, or,
> the prompts stack got busted (get_prompt/set_prompt/PROMPT), which
> I've seen happen before with target-async mode.
> 

Maybe this patch helps your crash as well.

Running a TUI command when the top level interpreter isn't TUI,
segfaults, though somewhere else.

./gdb -i=mi -ex "layout next"

#0  0x000000000046968c in gdb_flush (file=0x0) at ../../src/gdb/ui-file.c:173
#1  0x0000000000586739 in gdb_wait_for_event (block=0) at ../../src/gdb/event-loop.c:831
#2  0x0000000000585e58 in gdb_do_one_event (data=0x0) at ../../src/gdb/event-loop.c:432
#3  0x00000000005803c7 in catch_errors (func=0x585dfd <gdb_do_one_event>, func_args=0x0, errstring=0x7c48a8 "",
    mask=6) at ../../src/gdb/exceptions.c:510
#4  0x0000000000585ef2 in start_event_loop () at ../../src/gdb/event-loop.c:482
#5  0x00000000004e340c in mi_command_loop (mi_version=2) at ../../src/gdb/mi/mi-interp.c:292

It doesn't seem useful to be able to run TUI commands
while GDB is being controlled by MI.

I'm also disabling TUI command if we're not outputting to a tty.
It doesn't sound useful, and is broken.  Example, try this, and see
that foo.txt end up full of term control characters:
 
$ ./gdb -nx --batch -ex "refresh" > foo.txt
$ od -c foo.txt  | less

0000000 033   [   ?   1   0   4   9   h 033   [   1   ;   3   1   r 033
0000020   [   m 033   (   B 033   [   4   l 033   [   ?   7   h 033   [
0000040   ?   1   h 033   = 033   [   H 033   [   2   J 033   [   2   1
0000060   d 033   [   0   ;   7   m 033   (   B   N   o   n   e       N
0000100   o       p   r   o   c   e   s   s       I   n   :
...

Remove --batch, and GDB hangs, no ammount of ctrl-c seems to
get it back.

-- 
Pedro Alves

2010-03-30  Pedro Alves  <pedro@codesourcery.com>

	* tui/tui-interp.c (tui_is_toplevel): New.
	(tui_init): Set it.
	(tui_allowed_p): New.
	* tui/tui.c (tui_enable): Check if the TUI is allowed before
	enabling it.
	* tui/tui.h (tui_allowed_p): Declare.

---
 gdb/tui/tui-interp.c |   17 +++++++++++++++++
 gdb/tui/tui.c        |    3 +++
 gdb/tui/tui.h        |    4 ++++
 3 files changed, 24 insertions(+)

Index: src/gdb/tui/tui-interp.c
===================================================================
--- src.orig/gdb/tui/tui-interp.c	2010-03-30 21:14:17.000000000 +0100
+++ src/gdb/tui/tui-interp.c	2010-03-30 21:34:23.000000000 +0100
@@ -45,6 +45,8 @@ tui_exit (void)
   tui_disable ();
 }
 
+static int tui_is_toplevel = 0;
+
 /* These implement the TUI interpreter.  */
 
 static void *
@@ -60,9 +62,24 @@ tui_init (int top_level)
   if (ui_file_isatty (gdb_stdout))
     tui_initialize_readline ();
 
+  if (top_level)
+    tui_is_toplevel = 1;
+
   return NULL;
 }
 
+/* True if enabling the TUI is allowed.  Example, if the top level
+   interpreter is MI, enabling curses will certainly lose.  */
+
+int
+tui_allowed_p (void)
+{
+  /* Only if TUI is the top level interpreter.  Also don't try to
+     setup curses (and print funny control characters if we're not
+     outputting to a terminal.  */
+  return tui_is_toplevel && ui_file_isatty (gdb_stdout);
+}
+
 static int
 tui_resume (void *data)
 {
Index: src/gdb/tui/tui.c
===================================================================
--- src.orig/gdb/tui/tui.c	2010-03-30 21:05:07.000000000 +0100
+++ src/gdb/tui/tui.c	2010-03-30 21:38:13.000000000 +0100
@@ -364,6 +364,9 @@ tui_initialize_readline (void)
 void
 tui_enable (void)
 {
+  if (!tui_allowed_p ())
+    error (_("TUI mode not allowed"));
+
   if (tui_active)
     return;
 
Index: src/gdb/tui/tui.h
===================================================================
--- src.orig/gdb/tui/tui.h	2010-03-30 21:14:23.000000000 +0100
+++ src/gdb/tui/tui.h	2010-03-30 21:20:51.000000000 +0100
@@ -65,6 +65,10 @@ extern int tui_get_command_dimension (un
    key shortcut.  */
 extern void tui_initialize_readline (void);
 
+/* True if enabling the TUI is allowed.  Example, if the top level
+   interpreter is MI, enabling curses will certainly lose.  */
+extern int tui_allowed_p (void);
+
 /* Enter in the tui mode (curses).  */
 extern void tui_enable (void);
 

[cancel] Re: [patch] Fix crash on NULL rl_prompt

[Jan Kratochvil]

On Tue, 30 Mar 2010 19:55:08 +0200, Pedro Alves wrote:
> On Tuesday 30 March 2010 18:30:48, Tom Tromey wrote:
> > We can always un-approve it, if you would rather wait to see if a
> > reproducer is available.
> 
> I'll leave it up to you to decide.  But if this goes in,
> perhaps a comment saying this is being defensive would be good.

Found the problem was most probably due to a Fedora patch posted here without
the FSF GDB acceptance intention:
	[gdb FYI-patch] callback-mode readline-6.0 regression
	http://sourceware.org/ml/gdb-patches/2009-11/msg00596.html

That patch is needed only with readline-6.0 and neither with 5.1 nor 6.1.
That patch accidentally did rl_set_prompt (NULL).

(FSF GDB should import readline-6.1 but that is offtopic in this thread.)

Therefore not going to check-in this patch.


Thanks,
Jan