* [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY
@ 2021-12-13 16:20 Lancelot SIX
2021-12-15 11:39 ` Pedro Alves
0 siblings, 1 reply; 4+ messages in thread
From: Lancelot SIX @ 2021-12-13 16:20 UTC (permalink / raw)
To: gdb-patches; +Cc: Lancelot SIX
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.
I did not manage to come with a simple testcase that would reproduce the
original problem using DejaGNU (spawn always provides a tty). Any
feedback on this would be highly appreciated.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26056
Change-Id: I6458acef7b0d9beda2a10715d0345f02361076d9
---
gdb/tui/tui-interp.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
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. */
--
2.17.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY
2021-12-13 16:20 [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY Lancelot SIX
@ 2021-12-15 11:39 ` Pedro Alves
2021-12-15 13:46 ` Six, Lancelot
0 siblings, 1 reply; 4+ messages in thread
From: Pedro Alves @ 2021-12-15 11:39 UTC (permalink / raw)
To: Lancelot SIX, gdb-patches
On 2021-12-13 16:20, Lancelot SIX via Gdb-patches wrote:
> 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.
>
> I did not manage to come with a simple testcase that would reproduce the
> original problem using DejaGNU (spawn always provides a tty). Any
> feedback on this would be highly appreciated.
The patch LGTM. I've come up with a testcase. See below.
From 0d667551faecd603b861b6da82c1395bf85e1e4a Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 14 Dec 2021 19:11:47 +0000
Subject: [PATCH] Testcase for SIGWINCH without readline (PR gdb/26056)
Change-Id: I5b1be87e80bf370a548f18489b2f6503b0ea56a0
---
gdb/testsuite/gdb.base/sigwinch-notty.exp | 70 +++++++++++++++++++++++
gdb/testsuite/lib/gdb.exp | 4 +-
gdb/testsuite/lib/notty-wrap | 24 ++++++++
3 files changed, 97 insertions(+), 1 deletion(-)
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
base-commit: 161e87d12167b1e36193385485c1f6ce92f74f02
--
2.26.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY
2021-12-15 11:39 ` Pedro Alves
@ 2021-12-15 13:46 ` Six, Lancelot
2021-12-15 14:31 ` Pedro Alves
0 siblings, 1 reply; 4+ messages in thread
From: Six, Lancelot @ 2021-12-15 13:46 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
[AMD Official Use Only]
Hi,
Great! Thanks for the testcase!
I will add it to my original patch, with one modification, see below.
+# 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
+ }
+}
If I run this part as is with the faulty GDB (before fix), I have:
Running .../gdb.base/sigwinch-notty.exp ...
ERROR: GDB process no longer exists
=== gdb Summary ===
# of unresolved testcases 1
But I would expect a plain FAIL instead. To me "unresolved testcase" is more about a faulty testcase than a bug caught in GDB.
I propose to change the following addition:
diff --git a/gdb/testsuite/gdb.base/sigwinch-notty.exp b/gdb/testsuite/gdb.base/sigwinch-notty.exp
index 9084c258388..b81a227b284 100644
--- a/gdb/testsuite/gdb.base/sigwinch-notty.exp
+++ b/gdb/testsuite/gdb.base/sigwinch-notty.exp
@@ -59,6 +59,11 @@ 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
}
+ eof {
+ fail $gdb_test_name
+ # The rest of the file cannot execute if this part of the test failed.
+ return
+ }
}
With this, if I run the testsuite with GDB before fix, I have:
Running .../gdb.base/sigwinch-notty.exp ...
FAIL: gdb.base/sigwinch-notty.exp: wait for sleep exit
=== gdb Summary ===
# of unexpected failures 1
WDYT?
Best,
Lancelot.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY
2021-12-15 13:46 ` Six, Lancelot
@ 2021-12-15 14:31 ` Pedro Alves
0 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2021-12-15 14:31 UTC (permalink / raw)
To: Six, Lancelot, gdb-patches
On 2021-12-15 13:46, Six, Lancelot wrote:
> [AMD Official Use Only]
>
> Hi,
>
> Great! Thanks for the testcase!
>
> I will add it to my original patch, with one modification, see below.
>
> +# 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
> + }
> +}
>
> If I run this part as is with the faulty GDB (before fix), I have:
>
> Running .../gdb.base/sigwinch-notty.exp ...
> ERROR: GDB process no longer exists
>
> === gdb Summary ===
>
> # of unresolved testcases 1
>
Yes, I went a bit back and forth on that, but ended with that, note the comment above.
> But I would expect a plain FAIL instead. To me "unresolved testcase" is more about a faulty testcase than a bug caught in GDB.
I don't mind. My thinking was that GDB crashes can happen for any test, and we get an unresolved for them as well. I
wasn't considering this test any special. I.e., I was looking at it from the perspective of assuming a fixed GDB, and
then from that angle, a GDB crash here is just like any other.
>
> I propose to change the following addition:
>
> diff --git a/gdb/testsuite/gdb.base/sigwinch-notty.exp b/gdb/testsuite/gdb.base/sigwinch-notty.exp
> index 9084c258388..b81a227b284 100644
> --- a/gdb/testsuite/gdb.base/sigwinch-notty.exp
> +++ b/gdb/testsuite/gdb.base/sigwinch-notty.exp
> @@ -59,6 +59,11 @@ 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
> }
> + eof {
> + fail $gdb_test_name
> + # The rest of the file cannot execute if this part of the test failed.
> + return
As I said, I don't mind. But please do the wait -i + print status thing here too then. And remember
to update the comment that refers to ERROR/eof.
> + }
> }
>
> With this, if I run the testsuite with GDB before fix, I have:
>
> Running .../gdb.base/sigwinch-notty.exp ...
> FAIL: gdb.base/sigwinch-notty.exp: wait for sleep exit
>
> === gdb Summary ===
>
> # of unexpected failures 1
>
> WDYT?
>
> Best,
> Lancelot.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-15 14:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 16:20 [PATCH] [PR 26056] gdb/tui: install SIGWINCH only when connected to a TTY Lancelot SIX
2021-12-15 11:39 ` Pedro Alves
2021-12-15 13:46 ` Six, Lancelot
2021-12-15 14:31 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).