diff mbox series

[PATCHv2,17,2/2] gdb: fix crash from 'shell' when GDB has no terminal

Message ID fcf378fed2f225819f4d6b22db6e5c968267d60e.1765615940.git.aburgess@redhat.com
State New
Headers
Series Fixes for scoped_gdb_ttystate class |

Commit Message

Andrew Burgess Dec. 13, 2025, 8:53 a.m. UTC 
  Bug PR gdb/33716 identifies a problem where GDB will crash when using
the 'shell' command if GDB is not attached to a terminal.  E.g.:

  $ gdb -nw -nh -q -batch -ex 'shell echo hello' </dev/null
  hello

  Fatal signal: Segmentation fault
  ----- Backtrace -----
  ... etc ...

This problem was introduced with commit:

  commit 3a7f7d0be3a2953fd110f084b34586ce9c649d66
  Date:   Fri Jul 25 19:07:59 2025 +0200

      [gdb/tui] Fix shell command terminal settings

This commit introduces 'class scoped_gdb_ttystate', a RAII class which
backs up and restores the current terminal state.  Unfortunately, the
implementation tries to backup and restore the state even when GDB is
not attached to a terminal.

If GDB is not attached to a terminal then serial_get_tty_state
will (on a Unix like system) return a NULL pointer.  Calling
serial_set_tty_state with a NULL state object will trigger the crash
seen above.

Elsewhere in GDB, calling serial_set_tty_state is guarded by calling
gdb_has_a_terminal().  I propose that we only try to backup the
terminal state if GDB is actually connected to a terminal.  The call
to serial_set_tty_state will only be made if the backed up tty state
is not NULL.

With this change in place the crash is resolved.  There's a new test
to cover this case.

While I'm here I added a gdb_assert in ser-unix.c which would have
triggered in this case before we saw the crash.  And I've extended the
comment in serial.h to document that serial_get_tty_state can return
NULL.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33716
---
 gdb/inflow.c                                 |  8 ++-
 gdb/ser-unix.c                               |  1 +
 gdb/serial.h                                 |  3 +-
 gdb/terminal.h                               |  4 +-
 gdb/testsuite/gdb.base/shell-no-terminal.exp | 60 ++++++++++++++++++++
 5 files changed, 72 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/shell-no-terminal.exp

Comments

Simon Marchi Dec. 15, 2025, 9:16 p.m. UTC | #1 
On 12/13/25 3:53 AM, Andrew Burgess wrote:
> Bug PR gdb/33716 identifies a problem where GDB will crash when using
> the 'shell' command if GDB is not attached to a terminal.  E.g.:
> 
>   $ gdb -nw -nh -q -batch -ex 'shell echo hello' </dev/null
>   hello
> 
>   Fatal signal: Segmentation fault
>   ----- Backtrace -----
>   ... etc ...
> 
> This problem was introduced with commit:
> 
>   commit 3a7f7d0be3a2953fd110f084b34586ce9c649d66
>   Date:   Fri Jul 25 19:07:59 2025 +0200
> 
>       [gdb/tui] Fix shell command terminal settings
> 
> This commit introduces 'class scoped_gdb_ttystate', a RAII class which
> backs up and restores the current terminal state.  Unfortunately, the
> implementation tries to backup and restore the state even when GDB is
> not attached to a terminal.
> 
> If GDB is not attached to a terminal then serial_get_tty_state
> will (on a Unix like system) return a NULL pointer.  Calling
> serial_set_tty_state with a NULL state object will trigger the crash
> seen above.
> 
> Elsewhere in GDB, calling serial_set_tty_state is guarded by calling
> gdb_has_a_terminal().  I propose that we only try to backup the
> terminal state if GDB is actually connected to a terminal.  The call
> to serial_set_tty_state will only be made if the backed up tty state
> is not NULL.
> 
> With this change in place the crash is resolved.  There's a new test
> to cover this case.
> 
> While I'm here I added a gdb_assert in ser-unix.c which would have
> triggered in this case before we saw the crash.  And I've extended the
> comment in serial.h to document that serial_get_tty_state can return
> NULL.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33716

Some minor comments below, otherwise LGTM.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index 518b2dcf4e0..0292192ae2e 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -49,6 +49,8 @@
>  
>  static void pass_signal (int);
>  
> +static int gdb_has_a_terminal (void);
> +
>  static void child_terminal_ours_1 (target_terminal_state);
>  
>  /* Record terminal status separately for debugger and inferior.  */
> @@ -59,14 +61,16 @@ static struct serial *stdin_serial;
>  
>  scoped_restore_tty_state::scoped_restore_tty_state ()
>  {
> -  m_ttystate = serial_get_tty_state (stdin_serial);
> +  if (gdb_has_a_terminal ())
> +    m_ttystate = serial_get_tty_state (stdin_serial);

I was wondering if the gdb_has_a_terminal check was really needed here,
or we could let the call to serial_get_tty_state to through and it would
just return nullptr if stdin_serial isn't a tty.  In general I like "ask
for forgiveness rather than permission", because it's less code to keep
in sync.  But here the argument could be made that calling
gdb_has_a_terminal is more efficient than calling serial_get_tty_state,
which would do one XNEW, one syscall and one xfree.  So I guess I
convinced myself that it is fine to have the check here.

> diff --git a/gdb/serial.h b/gdb/serial.h
> index 6bcb5ab6598..a7070dfc8bd 100644
> --- a/gdb/serial.h
> +++ b/gdb/serial.h
> @@ -152,7 +152,8 @@ extern void serial_send_break (struct serial *scb);
>  extern void serial_raw (struct serial *scb);
>  
>  /* Return a pointer to a newly malloc'd ttystate containing the state
> -   of the tty.  */
> +   of the tty.  Can return NULL if the current tty state could not be
> +   read.  */

Could you mention that the case where GDB's stdin is not a terminal is
such a case where that function would return NULL?  Just to make it
clear that it is not only in hypothetical error cases that never happen
in the real world.

Simon
diff mbox series

Patch

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 518b2dcf4e0..0292192ae2e 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -49,6 +49,8 @@ 
 
 static void pass_signal (int);
 
+static int gdb_has_a_terminal (void);
+
 static void child_terminal_ours_1 (target_terminal_state);
 
 /* Record terminal status separately for debugger and inferior.  */
@@ -59,14 +61,16 @@  static struct serial *stdin_serial;
 
 scoped_restore_tty_state::scoped_restore_tty_state ()
 {
-  m_ttystate = serial_get_tty_state (stdin_serial);
+  if (gdb_has_a_terminal ())
+    m_ttystate = serial_get_tty_state (stdin_serial);
 }
 
 /* See terminal.h.  */
 
 scoped_restore_tty_state::~scoped_restore_tty_state ()
 {
-  serial_set_tty_state (stdin_serial, m_ttystate);
+  if (m_ttystate != nullptr)
+    serial_set_tty_state (stdin_serial, m_ttystate);
 }
 
 /* Terminal related info we need to keep track of.  Each inferior
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index f9ef8441629..9d17e1cc3ad 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -152,6 +152,7 @@  hardwire_set_tty_state (struct serial *scb, serial_ttystate ttystate)
   struct hardwire_ttystate *state;
 
   state = (struct hardwire_ttystate *) ttystate;
+  gdb_assert (state != nullptr);
 
   return set_tty_state (scb, state);
 }
diff --git a/gdb/serial.h b/gdb/serial.h
index 6bcb5ab6598..a7070dfc8bd 100644
--- a/gdb/serial.h
+++ b/gdb/serial.h
@@ -152,7 +152,8 @@  extern void serial_send_break (struct serial *scb);
 extern void serial_raw (struct serial *scb);
 
 /* Return a pointer to a newly malloc'd ttystate containing the state
-   of the tty.  */
+   of the tty.  Can return NULL if the current tty state could not be
+   read.  */
 
 extern serial_ttystate serial_get_tty_state (struct serial *scb);
 
diff --git a/gdb/terminal.h b/gdb/terminal.h
index 225554a60c3..892d43113d4 100644
--- a/gdb/terminal.h
+++ b/gdb/terminal.h
@@ -59,7 +59,9 @@  class scoped_restore_tty_state
   DISABLE_COPY_AND_ASSIGN (scoped_restore_tty_state);
 
 private:
-  serial_ttystate m_ttystate;
+  /* The saved tty state.  This can remain NULL even after the constructor
+     has run if serial_get_tty_state fails to fetch the tty state.  */
+  serial_ttystate m_ttystate = nullptr;
 };
 
 #ifdef USE_WIN32API
diff --git a/gdb/testsuite/gdb.base/shell-no-terminal.exp b/gdb/testsuite/gdb.base/shell-no-terminal.exp
new file mode 100644
index 00000000000..2a630fb1541
--- /dev/null
+++ b/gdb/testsuite/gdb.base/shell-no-terminal.exp
@@ -0,0 +1,60 @@ 
+# Copyright 2025 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/>.
+
+# Run GDB in batch mode, with stdin attached to a non-pty.  Use the
+# 'shell' command from the GDB command line.  Check that GDB doesn't
+# crash.  This checks for bug PR gdb/33716.
+
+# Remote boards override the 'remote_spawn' mechanism, and don't
+# support the 'readonly' argument that this test relies on.  Just
+# running this test on local hosts should be fine.
+require {!is_remote host}
+
+gdb_exit
+
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -batch -ex \"shell echo first\" -ex \"shell echo second\" </dev/null"
+
+    # Inlined default_gdb_spawn.
+    verbose -log "Spawning $GDB $INTERNAL_GDBFLAGS $GDBFLAGS"
+    gdb_write_cmd_file "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS"
+
+    set use_gdb_stub [use_gdb_stub]
+    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS [host_info gdb_opts] $GDBFLAGS" "readonly"]
+    if { $res < 0 || $res == "" } {
+	perror "Spawning $GDB failed."
+	return
+    }
+    set gdb_spawn_id $res
+}
+
+# Capture the output of the GDB process.  The above GDB is spawned
+# without a pty (so that we can replace its stdin), and so we don't
+# use '\r\n' as the end of line sequence, instead we expect '\n'.
+set saw_first false
+set saw_second false
+gdb_test_multiple "" "check shell command output" {
+    -re "^first\n" {
+	set saw_first true
+	exp_continue
+    }
+    -re "^second\n" {
+	set saw_second true
+	exp_continue
+    }
+    eof {
+	gdb_assert { $saw_first && $saw_second } $gdb_test_name
+    }
+}