diff mbox series

[gdb/cli] Don't allow to return from signal trampoline

Message ID 20241212094505.28440-1-tdevries@suse.de
State Superseded
Headers
Series [gdb/cli] Don't allow to return from signal trampoline |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Tom de Vries Dec. 12, 2024, 9:45 a.m. UTC 
  The Linaro CI reported a regression on arm-linux in test-case
gdb.base/sigstep.exp following commit 7b46460a619 ("[gdb/symtab] Apply
workaround for PR gas/31115 a bit more") [1]:
...
(gdb) return^M
Make __default_sa_restorer return now? (y or n) n^M
Not confirmed^M
(gdb) FAIL: $exp: return from handleri: \
  leave signal trampoline (got interactive prompt)
...

After installing package glibc-debuginfo and adding --with-separate-debug-dir
to the configure flags, I managed to reproduce the FAIL.

The regression seems to be a progression in the sense that the function name
for the signal trampoline is found.

After reading up on the signal trampoline [2] and the return command [3], my
understanding is that returning from the signal trampoline is potentially
unsafe, given that for instance the process signal mask won't be restored.

So what the test-case seems to be testing is: using the return command to
cancel safely returning from a signal handler, and instead unsafely returning
from the signal trampoline.

ISTM it's better to simply disallow returning from a signal trampoline.  Using
finish instead is a safe alternative.

Fix this by disallowing returning from a signal trampoline:
...
(gdb) return^M
Can not force return from a signal trampoline.^M
(gdb) PASS: $exp: return from handleri: leave signal trampoline (cannot return)
...

Tested on arm-linux.

[1] https://linaro.atlassian.net/browse/GNU-1459
[2] https://man7.org/linux/man-pages/man2/sigreturn.2.html
[3] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Returning.html
---
 gdb/stack.c                        | 3 +++
 gdb/testsuite/gdb.base/sigstep.exp | 7 +++----
 2 files changed, 6 insertions(+), 4 deletions(-)


base-commit: e3fa5cf492e09dc0e4041263751720c73799a3cd

Comments

Thiago Jung Bauermann Dec. 13, 2024, 1:15 a.m. UTC | #1 
Tom de Vries <tdevries@suse.de> writes:

> Fix this by disallowing returning from a signal trampoline:
> ...
> (gdb) return^M
> Can not force return from a signal trampoline.^M
> (gdb) PASS: $exp: return from handleri: leave signal trampoline (cannot return)
> ...
>
> Tested on arm-linux.

I agree with your conclusion. Thanks for fixing this!

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

--
Thiago
Tom de Vries Dec. 13, 2024, 11:35 a.m. UTC | #2 
On 12/13/24 02:15, Thiago Jung Bauermann wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> Fix this by disallowing returning from a signal trampoline:
>> ...
>> (gdb) return^M
>> Can not force return from a signal trampoline.^M
>> (gdb) PASS: $exp: return from handleri: leave signal trampoline (cannot return)
>> ...
>>
>> Tested on arm-linux.
> 
> I agree with your conclusion. Thanks for fixing this!
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 

Hi Thiago,

thanks for the review.

I'll leave this open for others to comment, since others may prefer the 
allow-but-warn approach.

FWIW, I've also tried using the return command while stopped in a 
syscall, and that didn't go well either, so I've filed a PR about that ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=32455 ).  There I prefer 
a warning because detecting whether we're in a syscall is not guaranteed 
to be 100% accurate.

Thanks,
- Tom
Tom de Vries Dec. 17, 2024, 10:39 a.m. UTC | #3 
On 12/13/24 12:35, Tom de Vries wrote:
> On 12/13/24 02:15, Thiago Jung Bauermann wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> Fix this by disallowing returning from a signal trampoline:
>>> ...
>>> (gdb) return^M
>>> Can not force return from a signal trampoline.^M
>>> (gdb) PASS: $exp: return from handleri: leave signal trampoline 
>>> (cannot return)
>>> ...
>>>
>>> Tested on arm-linux.
>>
>> I agree with your conclusion. Thanks for fixing this!
>>
>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>>
> 
> Hi Thiago,
> 
> thanks for the review.
> 
> I'll leave this open for others to comment, since others may prefer the 
> allow-but-warn approach.
> 

I just came across commit 1bd70cb9f83 ("rs6000, Fix Linux DWARF register 
mapping"), which mentions:
...
The unwinder bug shows up on platforms where the kernel uses a 
trampoline to dispatch "calls to" the signal handler (not just *returns
from* the signal handler).  Many platforms use a trampoline for signal
return, and that is working fine, but the only platform I'm (Ulrich)
aware of that uses a trampoline for signal handler calls is (recent
kernels for) PowerPC.  I believe the rationale for using a trampoline
here is to improve performance by avoiding unbalancing of the branch
predictor's call/return stack.
...

So, it's possible that returning from a signal trampoline prevents 
calling the signal handler.  That indeed may have some use, so I guess 
I've changed my mind, and am now preferring the allow-but-warn approach. 
  I'll submit a v2.

Thanks,
- Tom


> FWIW, I've also tried using the return command while stopped in a 
> syscall, and that didn't go well either, so I've filed a PR about that 
> ( https://sourceware.org/bugzilla/show_bug.cgi?id=32455 ).  There I 
> prefer a warning because detecting whether we're in a syscall is not 
> guaranteed to be 100% accurate.
> 
> Thanks,
> - Tom
Tom de Vries Dec. 19, 2024, 12:16 p.m. UTC | #4 
On 12/17/24 11:39, Tom de Vries wrote:
> On 12/13/24 12:35, Tom de Vries wrote:
>> On 12/13/24 02:15, Thiago Jung Bauermann wrote:
>>> Tom de Vries <tdevries@suse.de> writes:
>>>
>>>> Fix this by disallowing returning from a signal trampoline:
>>>> ...
>>>> (gdb) return^M
>>>> Can not force return from a signal trampoline.^M
>>>> (gdb) PASS: $exp: return from handleri: leave signal trampoline 
>>>> (cannot return)
>>>> ...
>>>>
>>>> Tested on arm-linux.
>>>
>>> I agree with your conclusion. Thanks for fixing this!
>>>
>>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>>>
>>
>> Hi Thiago,
>>
>> thanks for the review.
>>
>> I'll leave this open for others to comment, since others may prefer 
>> the allow-but-warn approach.
>>
> 
> I just came across commit 1bd70cb9f83 ("rs6000, Fix Linux DWARF register 
> mapping"), which mentions:
> ...
> The unwinder bug shows up on platforms where the kernel uses a 
> trampoline to dispatch "calls to" the signal handler (not just *returns
> from* the signal handler).  Many platforms use a trampoline for signal
> return, and that is working fine, but the only platform I'm (Ulrich)
> aware of that uses a trampoline for signal handler calls is (recent
> kernels for) PowerPC.  I believe the rationale for using a trampoline
> here is to improve performance by avoiding unbalancing of the branch
> predictor's call/return stack.
> ...
> 
> So, it's possible that returning from a signal trampoline prevents 
> calling the signal handler.  That indeed may have some use, so I guess 
> I've changed my mind, and am now preferring the allow-but-warn approach. 
>   I'll submit a v2.
> 

I've submitted this patch ( 
https://sourceware.org/pipermail/gdb-patches/2024-December/214260.html ) 
titled "[gdb/cli] Warn about forced return from signal trampoline". 
Given the changed title, I've not marked it as a v2.

Thanks,
- Tom

>> FWIW, I've also tried using the return command while stopped in a 
>> syscall, and that didn't go well either, so I've filed a PR about that 
>> ( https://sourceware.org/bugzilla/show_bug.cgi?id=32455 ).  There I 
>> prefer a warning because detecting whether we're in a syscall is not 
>> guaranteed to be 100% accurate.
>>
>> Thanks,
>> - Tom
>
diff mbox series

Patch

diff --git a/gdb/stack.c b/gdb/stack.c
index 4a3e7e4ff00..57a6a414cbf 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2698,6 +2698,9 @@  return_command (const char *retval_exp, int from_tty)
   if (get_frame_type (get_current_frame ()) == INLINE_FRAME)
     error (_("Can not force return from an inlined function."));
 
+  if (get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME)
+    error (_("Can not force return from a signal trampoline."));
+
   /* Compute the return value.  If the computation triggers an error,
      let it bail.  If the return type can't be handled, set
      RETURN_VALUE to NULL, and QUERY_PREFIX to an informational
diff --git a/gdb/testsuite/gdb.base/sigstep.exp b/gdb/testsuite/gdb.base/sigstep.exp
index 315d89d97c5..69b26822b2a 100644
--- a/gdb/testsuite/gdb.base/sigstep.exp
+++ b/gdb/testsuite/gdb.base/sigstep.exp
@@ -256,13 +256,12 @@  proc advancei { cmd } {
 		send_gdb "$cmd\n"
 		exp_continue -continue_timer
 	    }
+	    -re -wrap "Can not force return from a signal trampoline." {
+		pass "$gdb_test_name (cannot return)"
+	    }
 	    -re "return .*${gdb_prompt} $" {
 		fail "$test (stepped)"
 	    }
-	    -re "Make .*frame return now.*y or n. $" {
-		send_gdb "y\n"
-		exp_continue -continue_timer
-	    }
 	    -re "$inferior_exited_re normally.*${gdb_prompt} $" {
 		kfail gdb/8744 "$test (program exited)"
 		set program_exited 1