From patchwork Fri Jul 4 13:48:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 1920 Received: (qmail 29039 invoked by alias); 4 Jul 2014 13:48:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 29017 invoked by uid 89); 4 Jul 2014 13:48:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 04 Jul 2014 13:48:44 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s64Dmgkq001793 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Fri, 4 Jul 2014 09:48:43 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s64Dmeam004926; Fri, 4 Jul 2014 09:48:41 -0400 Message-ID: <53B6B0B8.2050702@redhat.com> Date: Fri, 04 Jul 2014 14:48:40 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Mark Wielaard , Jan Kratochvil CC: gdb-patches@sourceware.org Subject: [PATCH] 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]] References: <1400878753-24688-1-git-send-email-palves@redhat.com> <538739A2.2050105@redhat.com> <20140701162830.GA25877@host2.jankratochvil.net> <1404291574.3766.35.camel@bordewijk.wildebeest.org> <53B3CDCC.9050502@redhat.com> <53B57911.10304@redhat.com> In-Reply-To: <53B57911.10304@redhat.com> 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 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 * 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 * 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 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 (¤t_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 . */ + +/* 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 +#include + +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 . */ + +# 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 +}