[PATCH v2] gdb/tui: install SIGWINCH only when connected to a TTY

[PATCH v2] gdb/tui: install SIGWINCH only when connected to a TTY

[Lancelot SIX]

Hi,

here is a V2 for
https://sourceware.org/pipermail/gdb-patches/2021-December/184457.html.

Noticeable changes since V1:
  - Integrated the test case provided by Pedro Alves.

Best,
Lancelot.

---

PR26056 reports that when GDB is connected to non-TTY stdin/stdout, it
crashes when it receives a SIGWINCH signal.

This can be reproduced as follows:

    $ gdb/gdb -nx -batch -ex 'run' --args sleep 60 </dev/null 2>&1 | cat

    # from another terminal:
    $ kill -WINCH %(pidof gdb)

When doing so, the process crashes in a call to rl_resize_terminal:

    void
    rl_resize_terminal (void)
    {
      _rl_get_screen_size (fileno (rl_instream), 1);
      ...
    }

The problem is that at this point rl_instream has the value NULL.

The rl_instream variable is supposed to be initialized during a call to
readline_initialize_everything, which in a normal startup sequence is
called under this call chain:

    tui_interp::init
      tui_ensure_readline_initialized
        rl_initialize
          readline_initialize_everything

In tui_interp::init, we have the following sequence:

    tui_initialize_io ();
    tui_initialize_win ();                // <- Installs SIGWINCH
    if (gdb_stdout->isatty ())
      tui_ensure_readline_initialized (); // <- Initializes rl_instream

This function unconditionally installs the SIGWINCH signal handler (this
is done by tui_initialize_win), and then if gdb_stdout is a TTY it
initializes readline.  Therefore, if stdout is not a TTY, SIGWINCH is
installed but readline is not initialized.  In such situation
rl_instream stays NULL, and when GDB receives a SIGWINCH it calls its
handler and in fine tries to access rl_instream leading to the crash.

This patch proposes to fix this issue by installing the SIGWINCH signal
handler only if GDB is connected to a TTY.  Given that this
initialization it the only task of tui_initialize_win, this patch moves
tui_initialize_win just after the call to
tui_ensure_readline_initialized.

Tested on x86_64-linux.

Co-authored-by: Pedro Alves <pedro@palves.net>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26056
Change-Id: I6458acef7b0d9beda2a10715d0345f02361076d9
---
 gdb/testsuite/gdb.base/sigwinch-notty.exp | 70 +++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                 |  4 +-
 gdb/testsuite/lib/notty-wrap              | 24 ++++++++
 gdb/tui/tui-interp.c                      | 10 +++-
 4 files changed, 105 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/sigwinch-notty.exp
 create mode 100755 gdb/testsuite/lib/notty-wrap

diff --git a/gdb/testsuite/gdb.base/sigwinch-notty.exp b/gdb/testsuite/gdb.base/sigwinch-notty.exp
new file mode 100644
index 00000000000..9084c258388
--- /dev/null
+++ b/gdb/testsuite/gdb.base/sigwinch-notty.exp
@@ -0,0 +1,70 @@
+# Copyright 2021 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 GDB does not crash when it is started without a terminal /
+# without readline, and, it receives a SIGWINCH.  Regression test for
+# PR gdb/26056.
+
+if [target_info exists gdb,nosignals] {
+    verbose "Skipping $subdir/$gdb_test_file_name.exp because of nosignals."
+    continue
+}
+
+# The testfile relies on "run" from the command line, so only works
+# with "target native".
+if { [target_info gdb_protocol] != "" } {
+    continue
+}
+
+gdb_exit
+
+# Start GDB without a terminal, running sleep for a while.  Before the
+# sleep exits, we'll send a SIGWINCH.  "show editing" to double check
+# that readline is disabled.
+save_vars { GDB GDBFLAGS } {
+    set GDB "$srcdir/lib/notty-wrap $GDB"
+    set GDBFLAGS "$GDBFLAGS -ex \"show editing\" -ex run --args sleep 3"
+
+    gdb_spawn
+}
+
+set gdb_pid [exp_pid -i $gdb_spawn_id]
+
+verbose -log "gdb_spawn_id=$gdb_spawn_id"
+verbose -log "gdb_pid=$gdb_pid"
+
+after 1000 {
+    # Note, GDB is started under a shell, so PID is actually the
+    # shell's pid, not GDB's.  Use "-PID" to send the signal to the
+    # whole process group and reach GDB, instead of just to the shell.
+    remote_exec host "kill -SIGWINCH -${gdb_pid}"
+}
+
+# If GDB mishandles the SIGWINCH and crashes, that happens before we
+# see the "inferior exited normally" message, so this will ERROR with
+# eof.
+gdb_test_multiple "" "wait for sleep exit" {
+    -re "Editing of command lines as they are typed is off.*$inferior_exited_re normally.*$gdb_prompt " {
+	pass $gdb_test_name
+    }
+}
+
+gdb_test_multiple "" "wait for gdb exit" {
+    eof {
+	set wait_status [wait -i $gdb_spawn_id]
+	verbose -log "GDB process exited with wait status $wait_status"
+	pass $gdb_test_name
+    }
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 95220f6fc8d..b952c93c0d5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2096,7 +2096,9 @@ proc default_gdb_spawn { } {
 	    exit 1
 	}
     }
-    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
+
+    # Put GDBFLAGS last so that tests can put "--args ..." in it.
+    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS [host_info gdb_opts] $GDBFLAGS"]
     if { $res < 0 || $res == "" } {
 	perror "Spawning $GDB failed."
 	return 1
diff --git a/gdb/testsuite/lib/notty-wrap b/gdb/testsuite/lib/notty-wrap
new file mode 100755
index 00000000000..899c2ddfdd5
--- /dev/null
+++ b/gdb/testsuite/lib/notty-wrap
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+# Copyright (C) 2021 Free Software Foundation, Inc.
+#
+# This file is part of GDB.
+#
+# 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/>.
+
+# Wrap any passed-in program and args in a pipe, so that the program
+# is started without a terminal.
+
+exec "$@" </dev/null 2>&1 | cat
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 0cd5b125919..16859cb5745 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -242,9 +242,15 @@ tui_interp::init (bool top_level)
   atexit (tui_exit);
 
   tui_initialize_io ();
-  tui_initialize_win ();
   if (gdb_stdout->isatty ())
-    tui_ensure_readline_initialized ();
+    {
+      tui_ensure_readline_initialized ();
+
+      /* This installs the SIGWINCH signal handler.  The handler needs to do
+	 readline calls (to rl_resize_terminal), so it must not be installed
+	 unless readline is properly initialized.  */
+      tui_initialize_win ();
+    }
 }
 
 /* Used as the command handler for the tui.  */

base-commit: a2b1ea81bacc044a721092484c7cdcb600bcf9ce
-- 
2.25.1

Re: [PATCH v2] gdb/tui: install SIGWINCH only when connected to a TTY

[Pedro Alves]

On 2021-12-16 10:19, Lancelot SIX wrote:
> Hi,
> 
> here is a V2 for
> https://sourceware.org/pipermail/gdb-patches/2021-December/184457.html.
> 
> Noticeable changes since V1:
>   - Integrated the test case provided by Pedro Alves.
> 

OK, thanks.

RE: [PATCH v2] gdb/tui: install SIGWINCH only when connected to a TTY

[Six, Lancelot]

[AMD Official Use Only]

Hi,

Thanks.  I just have pushed this.

Best,
Lancelot.

-----Original Message-----
From: Pedro Alves <pedro@palves.net> 
Sent: 16 December 2021 16:03
To: Six, Lancelot <Lancelot.Six@amd.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH v2] gdb/tui: install SIGWINCH only when connected to a TTY

[CAUTION: External Email]

On 2021-12-16 10:19, Lancelot SIX wrote:
> Hi,
>
> here is a V2 for
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourceware.org%2Fpipermail%2Fgdb-patches%2F2021-December%2F184457.html&amp;data=04%7C01%7Clancelot.six%40amd.com%7Cc846c05f758849ce57d008d9c0ad9730%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637752674873034311%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=xr%2FigJ1KUAoH5SfdF2KiCzOBNVzzLDnZw3H%2FAY2R4Ag%3D&amp;reserved=0.
>
> Noticeable changes since V1:
>   - Integrated the test case provided by Pedro Alves.
>

OK, thanks.