Fix non executable stack handling when calling functions in the inferior.
Commit Message
When gdb creates a dummy frame to execute a function in the inferior,
the process may generate a SIGSEGV, SIGEMT or SIGILL because the stack
is non executable. If the signal handler set in gdb has option print
or stop enabled for these signals gdb handles this correctly.
However, in the case of noprint and nostop the signal is short-circuited
and the inferior process is sent the signal directly. This causes the
inferior to crash because of gdb.
This patch adds a check for SIGSEGV, SIGEMT or SIGILL so that these
signals are sent to gdb rather than short-circuited in the inferior.
gdb then handles them properly and the inferior process does not crash.
Also added a small testcase to test the issue called catch-gdb-caused-signals.
This applies to Linux only, tested on Linux.
gdb/ChangeLog:
PR breakpoints/16812
* linux-nat.c (linux_nat_filter_event): Report SIGILL,SIGSEGV,SIGEMT.
gdb/testsuite/ChangeLog:
PR breakpoints/16812
* gdb.base/catch-gdb-caused-signals.c: New file.
* gdb.base/catch-gdb-caused-signals.exp: New file.
---
gdb/linux-nat.c | 9 +++-
gdb/testsuite/gdb.base/catch-gdb-caused-signals.c | 30 +++++++++++++
.../gdb.base/catch-gdb-caused-signals.exp | 46 ++++++++++++++++++++
3 files changed, 83 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/catch-gdb-caused-signals.c
create mode 100644 gdb/testsuite/gdb.base/catch-gdb-caused-signals.exp
Comments
On 02/12/2015 08:49 PM, Antoine Tremblay wrote:
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 169188a..a0c0e1c 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3070,9 +3070,14 @@ linux_nat_filter_event (int lwpid, int status)
> }
>
> /* When using hardware single-step, we need to report every signal.
> - Otherwise, signals in pass_mask may be short-circuited. */
> + Otherwise, signals in pass_mask may be short-circuited
> + unless these signals are SIGILL, SIGSEGV or SIGEMT.
> + See handle_inferior_event for more information. */
I think it'd be clearer if this mentioned _why_ these signals
are important. Like:
Otherwise, signals in pass_mask may be short-circuited,
except signals that might be caused by a breakpoint. */
> if (!lp->step
> - && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status)))
> + && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status))
> + && signo != GDB_SIGNAL_ILL
> + && signo != GDB_SIGNAL_SEGV
> + && signo != GDB_SIGNAL_EMT)
By the same logic, this should filter SIGTRAP too. Also, no need
to handle SIGEMT on Linux.
It'd be nice to move this to a separate predicate function. We
actually already have one, in gdbserver/linux-low.c's
wstatus_maybe_breakpoint, which would be ideal here. Could you move
that to e.g., nat/linux-ptrace.c, and use it?
While at it, GDBserver should need this fix too, right?
Could you handle that as well?
> +#include <unistd.h>
> +#include <stdio.h>
> +
> +int
> +main (void)
> +{
> + int i = 0;
Empty line after declarations.
> + printf("call main");
Space before parens.
> + i++; /* set dprintf here */
> + return 0;
> +standard_testfile
The test should be skipped on targets that can't do infcalls
(look for gdb,cannot_call_functions).
> +
> +set dp_location [gdb_get_line_number "set dprintf here"]
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> + return -1
> +}
> +
> +if ![runto_main] {
> + fail "Can't run to main to make the tests"
> + return -1
> +}
> +
> +gdb_test "handle SIGSEGV nostop noprint" \
This should also do the same to SIGILL and SIGEMT, to
cover all targets.
> + "Signal\[ \t\]+Stop\[ \t\]+Print\[ \t\]+Pass to program\[ \t\]+Description\r\nSIGSEGV\[ \t\]+No\[ \t\]+No\[ \t\]+Yes\[ \t\].*"
> +
> +gdb_test "call printf(\"test\\n\")" "test.*"
Could you make this part of the test not rely on stdio? Not all
targets support that. Add a dummy function to the program
that is meant to be called from GDB, and then call that. Something
like:
gdb_test "print return_one ()" " = 1"
This part should pass with gdbserver:
make check RUNTESTFLAGS="--target_board=native-gdbserver mytestname.exp"
> +
> +# Clean up the breakpoint state.
> +delete_breakpoints
> +
> +# Also test with dprintf since the original bug was noticed using dprintf.
> +gdb_test "dprintf $dp_location,\"testdprintf\\n\"" "Dprintf .*"
I think you should explicitly set the dprintf-style to call.
As this part of the test can't be tested without stdio
support, skip it with:
if ![target_info exists gdb,noinferiorio] {
+
> +gdb_test "continue" "testdprintf.*"
>
Please set a breakpoint at the end of main, before issuing
that "continue" to avoid problems on targets where
gdb_continue_to_end would need to do something magic.
Thanks,
Pedro Alves
On 02/16/2015 06:36 PM, Pedro Alves wrote:
> On 02/12/2015 08:49 PM, Antoine Tremblay wrote:
>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 169188a..a0c0e1c 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -3070,9 +3070,14 @@ linux_nat_filter_event (int lwpid, int status)
>> }
>>
>> /* When using hardware single-step, we need to report every signal.
>> - Otherwise, signals in pass_mask may be short-circuited. */
>> + Otherwise, signals in pass_mask may be short-circuited
>> + unless these signals are SIGILL, SIGSEGV or SIGEMT.
>> + See handle_inferior_event for more information. */
> I think it'd be clearer if this mentioned _why_ these signals
> are important. Like:
>
> Otherwise, signals in pass_mask may be short-circuited,
> except signals that might be caused by a breakpoint. */
Indeed, fixed.
>
>> if (!lp->step
>> - && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status)))
>> + && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status))
>> + && signo != GDB_SIGNAL_ILL
>> + && signo != GDB_SIGNAL_SEGV
>> + && signo != GDB_SIGNAL_EMT)
> By the same logic, this should filter SIGTRAP too. Also, no need
> to handle SIGEMT on Linux.
>
> It'd be nice to move this to a separate predicate function. We
> actually already have one, in gdbserver/linux-low.c's
> wstatus_maybe_breakpoint, which would be ideal here. Could you move
> that to e.g., nat/linux-ptrace.c, and use it?
>
> While at it, GDBserver should need this fix too, right?
> Could you handle that as well?
Done.
>> +#include <unistd.h>
>> +#include <stdio.h>
>> +
>> +int
>> +main (void)
>> +{
>> + int i = 0;
> Empty line after declarations.
Done
>> + printf("call main");
> Space before parens.
Done, sorry
>> + i++; /* set dprintf here */
>> + return 0;
>
>
>> +standard_testfile
> The test should be skipped on targets that can't do infcalls
> (look for gdb,cannot_call_functions).
Done
>> +
>> +set dp_location [gdb_get_line_number "set dprintf here"]
>> +
>> +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
>> + return -1
>> +}
>> +
>> +if ![runto_main] {
>> + fail "Can't run to main to make the tests"
>> + return -1
>> +}
>> +
>> +gdb_test "handle SIGSEGV nostop noprint" \
> This should also do the same to SIGILL and SIGEMT, to
> cover all targets.
I added SIGILL but not SIGEMT since the problem is fixed in linux only...
>> + "Signal\[ \t\]+Stop\[ \t\]+Print\[ \t\]+Pass to program\[ \t\]+Description\r\nSIGSEGV\[ \t\]+No\[ \t\]+No\[ \t\]+Yes\[ \t\].*"
>> +
>> +gdb_test "call printf(\"test\\n\")" "test.*"
> Could you make this part of the test not rely on stdio? Not all
> targets support that. Add a dummy function to the program
> that is meant to be called from GDB, and then call that. Something
> like:
>
> gdb_test "print return_one ()" " = 1"
>
> This part should pass with gdbserver:
>
> make check RUNTESTFLAGS="--target_board=native-gdbserver mytestname.exp"
Done
>> +
>> +# Clean up the breakpoint state.
>> +delete_breakpoints
>> +
>> +# Also test with dprintf since the original bug was noticed using dprintf.
>> +gdb_test "dprintf $dp_location,\"testdprintf\\n\"" "Dprintf .*"
> I think you should explicitly set the dprintf-style to call.
Indeed I had forgotten that one...
> As this part of the test can't be tested without stdio
> support, skip it with:
>
> if ![target_info exists gdb,noinferiorio] {
Done
> +
>> +gdb_test "continue" "testdprintf.*"
>>
> Please set a breakpoint at the end of main, before issuing
> that "continue" to avoid problems on targets where
> gdb_continue_to_end would need to do something magic.
>
> Thanks,
> Pedro Alves
Done.
Patch v2 follows in the next mail...
Regards,
Antoine
@@ -3070,9 +3070,14 @@ linux_nat_filter_event (int lwpid, int status)
}
/* When using hardware single-step, we need to report every signal.
- Otherwise, signals in pass_mask may be short-circuited. */
+ Otherwise, signals in pass_mask may be short-circuited
+ unless these signals are SIGILL, SIGSEGV or SIGEMT.
+ See handle_inferior_event for more information. */
if (!lp->step
- && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status)))
+ && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status))
+ && signo != GDB_SIGNAL_ILL
+ && signo != GDB_SIGNAL_SEGV
+ && signo != GDB_SIGNAL_EMT)
{
linux_resume_one_lwp (lp, lp->step, signo);
if (debug_linux_nat)
new file mode 100644
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2015 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 a simple dummy program for gdb to read. */
+
+#include <unistd.h>
+#include <stdio.h>
+
+int
+main (void)
+{
+ int i = 0;
+ printf("call main");
+ i++; /* set dprintf here */
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,46 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2015 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 checks if we handle a SIGSEGV caused by gdb in the inferior
+# even if we have noprint,nostop options set in handle SIGSEGV
+# See PR breakpoints/16812
+
+standard_testfile
+
+set dp_location [gdb_get_line_number "set dprintf here"]
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+ return -1
+}
+
+if ![runto_main] {
+ fail "Can't run to main to make the tests"
+ return -1
+}
+
+gdb_test "handle SIGSEGV nostop noprint" \
+ "Signal\[ \t\]+Stop\[ \t\]+Print\[ \t\]+Pass to program\[ \t\]+Description\r\nSIGSEGV\[ \t\]+No\[ \t\]+No\[ \t\]+Yes\[ \t\].*"
+
+gdb_test "call printf(\"test\\n\")" "test.*"
+
+# Clean up the breakpoint state.
+delete_breakpoints
+
+# Also test with dprintf since the original bug was noticed using dprintf.
+gdb_test "dprintf $dp_location,\"testdprintf\\n\"" "Dprintf .*"
+
+gdb_test "continue" "testdprintf.*"