[binutils-gdb] gdb: don't try to use readline before it's initialized

[binutils-gdb] gdb: don't try to use readline before it's initialized

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=86d77f6a5be904f13c633f10bdf77ff3dd69db94

commit 86d77f6a5be904f13c633f10bdf77ff3dd69db94
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Mar 30 14:49:11 2022 +0100

    gdb: don't try to use readline before it's initialized
    
    While working on a different patch, I triggered an assertion from the
    initialize_current_architecture code, specifically from one of
    the *_gdbarch_init functions in a *-tdep.c file.  This exposes a
    couple of issues with GDB.
    
    This is easy enough to reproduce by adding 'gdb_assert (false)' into a
    suitable function.  For example, I added a line into i386_gdbarch_init
    and can see the following issue.
    
    I start GDB and immediately hit the assert, the output is as you'd
    expect, except for the very last line:
    
      $ ./gdb/gdb --data-directory ./gdb/data-directory/
      ../../src.dev-1/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.
      A problem internal to GDB has been detected,
      further debugging may prove unreliable.
      ----- Backtrace -----
      ... snip ...
      ---------------------
      ../../src.dev-1/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.
      A problem internal to GDB has been detected,
      further debugging may prove unreliable.
      Quit this debugging session? (y or n) ../../src.dev-1/gdb/ser-event.c:212:16: runtime error: member access within null pointer of type 'struct serial'
    
    Something goes wrong when we try to query the user.  Note, I
    configured GDB with --enable-ubsan, I suspect that without this the
    above "error" would actually just be a crash.
    
    The backtrace from ser-event.c:212 looks like this:
    
      (gdb) bt 10
      #0  serial_event_clear (event=0x675c020) at ../../src/gdb/ser-event.c:212
      #1  0x0000000000769456 in invoke_async_signal_handlers () at ../../src/gdb/async-event.c:211
      #2  0x000000000295049b in gdb_do_one_event () at ../../src/gdbsupport/event-loop.cc:194
      #3  0x0000000001f015f8 in gdb_readline_wrapper (
          prompt=0x67135c0 "../../src/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugg"...)
          at ../../src/gdb/top.c:1141
      #4  0x0000000002118b64 in defaulted_query(const char *, char, typedef __va_list_tag __va_list_tag *) (
          ctlstr=0x2e4eb68 "%s\nQuit this debugging session? ", defchar=0 '\000', args=0x7fffffffa6e0)
          at ../../src/gdb/utils.c:934
      #5  0x0000000002118f72 in query (ctlstr=0x2e4eb68 "%s\nQuit this debugging session? ")
          at ../../src/gdb/utils.c:1026
      #6  0x00000000021170f6 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=0x6107bc0 <internal_error_problem>, file=0x2b976c8 "../../src/gdb/i386-tdep.c",
          line=8455, fmt=0x2b96d7f "%s: Assertion `%s' failed.", ap=0x7fffffffa8e8) at ../../src/gdb/utils.c:417
      #7  0x00000000021175a0 in internal_verror (file=0x2b976c8 "../../src/gdb/i386-tdep.c", line=8455,
          fmt=0x2b96d7f "%s: Assertion `%s' failed.", ap=0x7fffffffa8e8) at ../../src/gdb/utils.c:485
      #8  0x00000000029503b3 in internal_error (file=0x2b976c8 "../../src/gdb/i386-tdep.c", line=8455,
          fmt=0x2b96d7f "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:55
      #9  0x000000000122d5b6 in i386_gdbarch_init (info=..., arches=0x0) at ../../src/gdb/i386-tdep.c:8455
      (More stack frames follow...)
    
    It turns out that the problem is that the async event handler
    mechanism has been invoked, but this has not yet been initialized.
    
    If we look at gdb_init (in gdb/top.c) we can indeed see the call to
    gdb_init_signals is after the call to initialize_current_architecture.
    
    If I reorder the calls, moving gdb_init_signals earlier, then the
    initial error is resolved, however, things are still broken.  I now
    see the same "Quit this debugging session? (y or n)" prompt, but when
    I provide an answer and press return GDB immediately crashes.
    
    So what's going on now?  The next problem is that the call_readline
    field within the current_ui structure is not initialized, and this
    callback is invoked to process the reply I entered.
    
    The problem is that call_readline is setup as a result of calling
    set_top_level_interpreter, which is called from captured_main_1.
    Unfortunately, set_top_level_interpreter is called after gdb_init is
    called.
    
    I wondered how to solve this problem for a while, however, I don't
    know if there's an easy "just reorder some lines" solution here.
    Looking through captured_main_1 there seems to be a bunch of
    dependencies between printing various things, parsing config files,
    and setting up the interpreter.  I'm sure there is a solution hiding
    in there somewhere.... I'm just not sure I want to spend any longer
    looking for it.
    
    So.
    
    I propose a simpler solution, more of a hack/work-around.  In utils.c
    we already have a function filtered_printing_initialized, this is
    checked in a few places within internal_vproblem.  In some of these
    cases the call gates whether or not GDB will query the user.
    
    My proposal is to add a new readline_initialized function, which
    checks if the current_ui has had readline initialized yet.  If this is
    not the case then we should not attempt to query the user.
    
    After this change GDB prints the error message, the backtrace, and
    then aborts (including dumping core).  This actually seems pretty sane
    as, if GDB has not yet made it through the initialization then it
    doesn't make much sense to allow the user to say "no, I don't want to
    quit the debug session" (I think).

Diff:
---
 gdb/utils.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 68bf3a67340..62a75d9e37f 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -306,6 +306,16 @@ struct internal_problem
   bool should_print_backtrace;
 };
 
+/* Return true if the readline callbacks have been initialized for UI.
+   This is always true once GDB is fully initialized, but during the early
+   startup phase this is initially false.  */
+
+static bool
+readline_initialized (struct ui *ui)
+{
+  return ui->call_readline != nullptr;
+}
+
 /* Report a problem, internal to GDB, to the user.  Once the problem
    has been reported, and assuming GDB didn't quit, the caller can
    either allow execution to resume or throw an error.  */
@@ -378,6 +388,7 @@ internal_vproblem (struct internal_problem *problem,
   if (problem->should_quit != internal_problem_ask
       || !confirm
       || !filtered_printing_initialized ()
+      || !readline_initialized (current_ui)
       || problem->should_print_backtrace)
     gdb_printf (gdb_stderr, "%s\n", reason.c_str ());
 
@@ -389,7 +400,8 @@ internal_vproblem (struct internal_problem *problem,
       /* Default (yes/batch case) is to quit GDB.  When in batch mode
 	 this lessens the likelihood of GDB going into an infinite
 	 loop.  */
-      if (!confirm || !filtered_printing_initialized ())
+      if (!confirm || !filtered_printing_initialized ()
+	  || !readline_initialized (current_ui))
 	quit_p = 1;
       else
 	quit_p = query (_("%s\nQuit this debugging session? "),
@@ -413,7 +425,8 @@ internal_vproblem (struct internal_problem *problem,
     {
       if (!can_dump_core_warn (LIMIT_MAX, reason.c_str ()))
 	dump_core_p = 0;
-      else if (!filtered_printing_initialized ())
+      else if (!filtered_printing_initialized ()
+	       || !readline_initialized (current_ui))
 	dump_core_p = 1;
       else
 	{