Fix "attach" command vs user input race [Re: Regression for attach from stdin [Re: [pushed] Re: [PATCH v6 0/2] enable target-async by default]]

Message ID 53B6B0B8.2050702@redhat.com
State Superseded
Headers

Commit Message

Pedro Alves July 4, 2014, 1:48 p.m. UTC
  On 07/03/2014 04:38 PM, Pedro Alves wrote:
> On 07/02/2014 10:15 AM, Pedro Alves wrote:
>> On 07/02/2014 09:59 AM, Mark Wielaard wrote:
>>> Thanks for tracking this down Jan. Should this patch be reverted 
>>
>> Unless it turns into a deep rat hole, I'd like to fix the issue
>> instead.  I haven't investigated this particular issue yet, but I've
>> been busy fixing PR17072, and I ended up fixing several issues
>> related to execution commands entered directly in the command line,
>> before GDB's main event loop starts.  This one sounds related.
> 
> Looking at this now.  The other series unfortunately doesn't fix this.
> 

Here's a fix.  Let me know whether it fixes guality testing.

You can also find it at:

 git@github.com:palves/gdb.git palves/attach_from_stdin

I've also pushed it on top of the palves/pr17072_pagination_async
branch, in case the other fixes on that branch (posted yesterday)
are necessary.

Thanks,
Pedro Alves

8<----------

From f93645049c02459bedada7b61fea633f59a47817 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 4 Jul 2014 13:47:53 +0100
Subject: [PATCH] Fix "attach" command vs user input race

On async targets, a synchronous attach is done like this:

   #1 - target_attach is called (PTRACE_ATTACH is issued)
   #2 - a continuation is installed
   #3 - we go back to the event loop
   #4 - target reports stop (SIGSTOP), event loop wakes up, and
        attach continuation is called
   #5 - among other things, the continuation calls
        target_terminal_inferior, which removes stdin from the event
        loop

Note that in #3, GDB is still processing user input.  If the user is
fast enough, e.g., with something like:

  echo -e "attach PID\nset xxx=1" | gdb

... then the "set" command is processed before the attach completes.

We get worse behavior even, if input is a tty and therefore
readline/editing is enabled, with e.g.,:

 (gdb) attach PID\nset xxx=1

we then crash readline/gdb, with:

 Attaching to program: attach-wait-input, process 14537
 readline: readline_callback_read_char() called with no handler!
 Aborted
 $

Fix this by calling target_terminal_inferior before #3 above.

The test covers both scenarios by running with editing/readline forced
to both on and off.

gdb/
2014-07-04  Pedro Alves  <palves@redhat.com>

	* infcmd.c (attach_command_post_wait): Don't call
	target_terminal_inferior here.
	(attach_command): Call it here instead.

gdb/testsuite/
2014-07-04  Pedro Alves  <palves@redhat.com>

	* gdb.base/attach-fg-no-stdin.exp: New file.
	* gdb.base/attach-fg-no-stdin.c: New file.
---
 gdb/infcmd.c                                 |   9 +-
 gdb/testsuite/gdb.base/attach-wait-input.c   |  47 +++++++++++
 gdb/testsuite/gdb.base/attach-wait-input.exp | 119 +++++++++++++++++++++++++++
 3 files changed, 170 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/attach-wait-input.c
 create mode 100644 gdb/testsuite/gdb.base/attach-wait-input.exp
  

Comments

Mark Wielaard July 4, 2014, 9:12 p.m. UTC | #1
On Fri, 2014-07-04 at 14:48 +0100, Pedro Alves wrote:
> Here's a fix.  Let me know whether it fixes guality testing.

Yes, that patch works for me. Now I can run make check-c
RUNTESTFLAGS=guality.exp in gcc without needing any guality.h
workarounds.

Thanks,

Mark
  
Doug Evans July 7, 2014, 4:39 p.m. UTC | #2
Pedro Alves writes:
 > gdb/
 > 2014-07-04  Pedro Alves  <palves@redhat.com>
 > 
 > 	* infcmd.c (attach_command_post_wait): Don't call
 > 	target_terminal_inferior here.
 > 	(attach_command): Call it here instead.
 > [...]
 > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
 > index c4bb401..76ab12b 100644
 > --- a/gdb/infcmd.c
 > +++ b/gdb/infcmd.c
 > @@ -2381,9 +2381,6 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
 > 
 >    post_create_inferior (&current_target, from_tty);
 > 
 > -  /* Install inferior's terminal modes.  */
 > -  target_terminal_inferior ();
 > -
 >    if (async_exec)
 >      {
 >        /* The user requested an `attach&', so be sure to leave threads
 > @@ -2495,9 +2492,11 @@ attach_command (char *args, int from_tty)
 >       shouldn't refer to attach_target again.  */
 >    attach_target = NULL;
 > 
 > -  /* Set up the "saved terminal modes" of the inferior
 > -     based on what modes we are starting it with.  */
 > +  /* Set up the "saved terminal modes" of the inferior based on what
 > +     modes we are starting it with, and remove stdin from the event
 > +     loop.  */
 >    target_terminal_init ();
 > +  target_terminal_inferior ();
 > 
 >    /* Set up execution context to know that we should return from
 >       wait_for_inferior as soon as the target reports a stop.  */

Our nomenclature here is problematic. I always do a double take when
I see attach and target_terminal_foo mentioned in the same sentence.
Bleah.

For the case at hand, and at least until we have something more
readable, can I ask for a slight change here?

target_terminal_inferior can do a lot more than just "remove stdin
from the event loop", thus as a reader I'm left being unsure
if there aren't still more bugs here.

Plus, while I can see how to map the comment to the code in the patch,

"Set up the "saved terminal modes" of the inferior based on what
modes we are starting it with"
-->
  target_terminal_init ();

and

"remove stdin from the event loop"
-->
  target_terminal_inferior ();

If I look at the result after the patch,
combining the two steps into one sentence doesn't help clarify
things given that target_terminal_inferior can do more than just
"remove stdin from the event loop".

E.g., this is a marginal improvement:

  /* Set up the "saved terminal modes" of the inferior based on what
     modes we are starting it with.  */
  target_terminal_init ();
  /* And remove stdin from the event loop.  */
  target_terminal_inferior ();

But I'm hoping for more text in the second comment to explain things
better.
  
Jan Kratochvil July 7, 2014, 5:02 p.m. UTC | #3
On Fri, 04 Jul 2014 15:48:40 +0200, Pedro Alves wrote:
> Here's a fix.  Let me know whether it fixes guality testing.

Mark has already replied the same but I can also confirm it fixes the GCC
testsuite.


Thanks,
Jan
  

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c4bb401..76ab12b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2381,9 +2381,6 @@  attach_command_post_wait (char *args, int from_tty, int async_exec)

   post_create_inferior (&current_target, from_tty);

-  /* Install inferior's terminal modes.  */
-  target_terminal_inferior ();
-
   if (async_exec)
     {
       /* The user requested an `attach&', so be sure to leave threads
@@ -2495,9 +2492,11 @@  attach_command (char *args, int from_tty)
      shouldn't refer to attach_target again.  */
   attach_target = NULL;

-  /* Set up the "saved terminal modes" of the inferior
-     based on what modes we are starting it with.  */
+  /* Set up the "saved terminal modes" of the inferior based on what
+     modes we are starting it with, and remove stdin from the event
+     loop.  */
   target_terminal_init ();
+  target_terminal_inferior ();

   /* Set up execution context to know that we should return from
      wait_for_inferior as soon as the target reports a stop.  */
diff --git a/gdb/testsuite/gdb.base/attach-wait-input.c b/gdb/testsuite/gdb.base/attach-wait-input.c
new file mode 100644
index 0000000..bb99b1f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-wait-input.c
@@ -0,0 +1,47 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+/* This program is intended to be started outside of gdb, and then
+   attached to by gdb.  Thus, it simply spins in a loop.  The loop
+   is exited when & if the variable 'should_exit' is non-zero.  (It
+   is initialized to zero in this program, so the loop will never
+   exit unless/until gdb sets the variable to non-zero.)
+   */
+
+#include <sys/types.h>
+#include <unistd.h>
+
+volatile int should_exit = 0;
+int mypid;
+
+static void
+setup_done (void)
+{
+}
+
+int
+main (void)
+{
+  unsigned int counter = 1;
+
+  mypid = getpid ();
+  setup_done ();
+
+  for (counter = 0; !should_exit && counter < 100; counter++)
+    sleep (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/attach-wait-input.exp b/gdb/testsuite/gdb.base/attach-wait-input.exp
new file mode 100644
index 0000000..3120778
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-wait-input.exp
@@ -0,0 +1,119 @@ 
+# Copyright 1997-2014 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/>.  */
+
+# Verify that GDB waits for the "attach" command to finish before
+# processing the following command.
+#
+# GDB used to have a race where on async targets, in the small window
+# between the attach request and the initial stop for the attach, GDB
+# was still processing user input.
+#
+# The issue was originally detected with:
+#
+#  echo -e "attach PID\nset xxx=1" | gdb
+#
+# In that scenario, stdin is not a tty, which disables readline.
+# Explicitly turning off editing exercises the same code path, and is
+# simpler to do, so we test with both editing on and off.
+
+# The test uses the "attach" command.
+if [target_info exists use_gdb_stub] {
+    return
+}
+
+standard_testfile
+
+if {[build_executable "failed to build" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Start the program running, and return its PID, ready for attaching.
+
+proc start_program {binfile} {
+    global gdb_prompt
+    global decimal
+
+    clean_restart $binfile
+
+    if ![runto setup_done] then {
+	fail "Can't run to setup_done"
+	return 0
+    }
+
+    # Get the PID of the test process.
+    set testpid ""
+    set test "get inferior process ID"
+    gdb_test_multiple "p mypid" $test {
+	-re " = ($decimal)\r\n$gdb_prompt $" {
+	    set testpid $expect_out(1,string)
+	    pass $test
+	}
+    }
+
+    gdb_test "detach" "Detaching from program: .*"
+
+    if {$testpid == ""} {
+	return
+    }
+
+    return $testpid
+}
+
+# Do test proper.  EDITING indicates whether "set editing" is on or
+# off.
+
+proc test { editing } {
+    global gdb_prompt
+    global binfile
+    global decimal
+
+    with_test_prefix "editing $editing" {
+
+	set testpid [start_program $binfile]
+	if {$testpid == ""} {
+	    return
+	}
+
+	# Enable/disable readline.
+	gdb_test_no_output "set editing $editing"
+
+	# Send both commands at once.
+	send_gdb "attach $testpid\nprint should_exit = 1\n"
+
+	# Use gdb_expect directly instead of gdb_test_multiple to
+	# avoid races with the double prompt.
+	set test "attach and print"
+	gdb_expect {
+	    -re "Attaching to program.*process $testpid\r\n.*$gdb_prompt.*$decimal = 1\r\n$gdb_prompt $" {
+		pass "$test"
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	}
+
+	# As we've used attach, on quit, we'll detach from the
+	# program.  Explicitly kill it in case we failed above.
+	gdb_test "kill" \
+	    "" \
+	    "after attach, exit" \
+	    "Kill the program being debugged.*y or n. $" \
+	    "y"
+    }
+}
+
+foreach editing {"on" "off"} {
+    test $editing
+}