[PATCH v2] [gdb/testsuite] Handle ptrace operation not permitted in can_spawn_for_attach
Pedro Alves
pedro@palves.net
Fri Apr 26 18:50:37 GMT 2024
On 2024-04-24 22:35, Tom de Vries wrote:
> On 4/24/24 21:40, Pedro Alves wrote:
>> On 2024-04-18 07:32, Tom de Vries wrote:
>>> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
>>> we run into attach failures.
>>>
>>> Fix this by recognizing "ptrace: Operation not permitted" in
>>> can_spawn_for_attach.
>>>
>>> Tested on aarch64-linux.
>>
>> I really don't like relying on "attach" 's behavior to decide whether to test attach.
>> It's an inversion of responsibilities.
>>
>
> Hi Pedro,
>
> I don't see it that way, I think this approach accurately handles that if there's no permission to attach to a spawned process, you cannot spawn-for-attach.
My inversion remark was more about of the risk of attach breaking/regressing, and attach
testcases stop being exercised. PASS -> UNSUPPORTED/UNTESTED regressions aren't that well
noticed. But I suppose that if the procedure is written in a way that is very lax in its attach
check, only really looking for the ptrace output case, and defaulting to return that yes,
we can attach", that is mitigated.
>
>> can_spawn_for_attach means "can I spawn a process, such that I will attach to it later", a
>> simple atomic thing.
>> > Also, with this change, it means, "can I spawn a process and does
> attaching to it work?"
>> So it ends up misnamed.
>>
>
> I see what you mean, though I don't see it as misnamed, but if that bothers you I'm glad to rename it something else. Can_spawn_and_attach? You have another suggestion?
can_spawn_for_attach's name was originally derived from spawn_wait_for_attach.
(git 60b3033e6e2936af6fcc37cf67cade99a89940ad). It was more about
"can I call spawn_wait_for_attach" than "can I attach". But given the
check for use_gdb_stub, I guess you could say that your check doesn't really
change that aspect. It's just my expectations that need to be adjusted.
Let's just leave it as you have it.
>
> I think the gdb_attach approach is problematic for the reasons that:
> - it's not versatile enough to handle all the cases where it's needed,
> and
> - it's required to be used in all the attach sites.
>
> On the contrary, the proposed patch very simply handles all the cases, and in only one proc which is already used in a trivial way in most test-cases that need it. So it seems obvious to me that the proposed solution is preferable.
One issue I was concerned about is that for extended-remote testing, this change
assumes the native-extended-gdbserver board's behavior of gdb_start spawning gdbserver
and making gdb connect to it. I don't know whether actually-remote extended-remote
boards actually do that. But I suppose it's a reasonable thing for them to do.
OK, I'm convinced.
Comments on the patch itself below.
On 2024-04-18 07:32, Tom de Vries wrote:
> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
> we run into attach failures.
>
> Fix this by recognizing "ptrace: Operation not permitted" in
> can_spawn_for_attach.
>
> Tested on aarch64-linux.
> ---
> gdb/testsuite/gdb.base/break-interp.exp | 4 +
> gdb/testsuite/gdb.base/dprintf-detach.exp | 2 +-
> .../run-control-while-bg-execution.exp | 10 ++-
> .../gdb.multi/attach-while-running.exp | 2 +-
> .../gdb.threads/attach-into-signal.exp | 3 +-
> .../gdb.threads/attach-slow-waitpid.exp | 3 +-
> gdb/testsuite/gdb.threads/attach-stopped.exp | 3 +-
> .../gdb.threads/check-libthread-db.exp | 42 ++++-----
> gdb/testsuite/lib/gdb.exp | 87 ++++++++++++++++---
> 9 files changed, 116 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
> index addacde552d..d7f84db4770 100644
> --- a/gdb/testsuite/gdb.base/break-interp.exp
> +++ b/gdb/testsuite/gdb.base/break-interp.exp
> @@ -318,6 +318,10 @@ proc test_attach_gdb {file pid displacement prefix} {
> }
>
> proc test_attach {file displacement {relink_args ""}} {
> + if { ![can_spawn_for_attach] } {
> + return
> + }
> +
> global board_info
> global exec
>
> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
> index 550d319a895..b4184d698df 100644
> --- a/gdb/testsuite/gdb.base/dprintf-detach.exp
> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
> @@ -21,7 +21,7 @@
> load_lib gdbserver-support.exp
>
> # The test relies on "detach/attach".
> -require !use_gdb_stub
> +require can_spawn_for_attach
>
> standard_testfile
> set escapedbinfile [string_to_regexp ${binfile}]
> diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> index f1cbd9351d3..a36c4ee3614 100644
> --- a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> +++ b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> @@ -108,8 +108,14 @@ proc do_test { action1 action2 } {
> }
> }
>
> -foreach_with_prefix action1 { kill detach add none } {
> - foreach_with_prefix action2 { start run attach } {
> +set actions1 { kill detach add none }
> +set actions2 { start run }
> +if { [can_spawn_for_attach] } {
> + lappend actions2 attach
> +}
> +
I suppose this isn't using the more typical form of:
foreach_with_prefix action1 { kill detach add none } {
foreach_with_prefix action2 { start run attach } {
if {$action2 == "attach" && ![can_spawn_for_attach]} {
continue
}
do_test $action1 $action2
}
}
... to make sure the can_spawn_for_attach call is done early?
Can you add a comment if this is important? Otherwise, I think
the more typical form is a lot more readable.
> +foreach_with_prefix action1 $actions1 {
> + foreach_with_prefix action2 $actions2 {
> do_test $action1 $action2
> }
> }
> diff --git a/gdb/testsuite/gdb.multi/attach-while-running.exp b/gdb/testsuite/gdb.multi/attach-while-running.exp
> index eade8b42a18..ca4fa635467 100644
> --- a/gdb/testsuite/gdb.multi/attach-while-running.exp
> +++ b/gdb/testsuite/gdb.multi/attach-while-running.exp
> @@ -36,7 +36,7 @@
>
> standard_testfile
>
> -require !use_gdb_stub
> +require can_spawn_for_attach
>
> if { [build_executable "failed to prepare" ${testfile} ${srcfile}] } {
> return
> diff --git a/gdb/testsuite/gdb.threads/attach-into-signal.exp b/gdb/testsuite/gdb.threads/attach-into-signal.exp
> index 87e34070548..91da960e09a 100644
> --- a/gdb/testsuite/gdb.threads/attach-into-signal.exp
> +++ b/gdb/testsuite/gdb.threads/attach-into-signal.exp
> @@ -17,7 +17,8 @@
> # This file was created by Jan Kratochvil <jan.kratochvil@redhat.com>.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> index dc3e62a7b7e..28d70daad8c 100644
> --- a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> +++ b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> @@ -37,7 +37,8 @@
> # during the attach phase.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/gdb.threads/attach-stopped.exp b/gdb/testsuite/gdb.threads/attach-stopped.exp
> index 78e194c992f..0421ffc3794 100644
> --- a/gdb/testsuite/gdb.threads/attach-stopped.exp
> +++ b/gdb/testsuite/gdb.threads/attach-stopped.exp
> @@ -18,7 +18,8 @@
> # This file was updated by Jan Kratochvil <jan.kratochvil@redhat.com>.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index d48ea37c0cc..937add7a272 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -6131,6 +6131,43 @@ proc gdb_exit { } {
> catch default_gdb_exit
> }
>
> +# Helper function for can_spawn_for_attach. Try to spawn and attach, and
> +# return 0 only if we cannot attach because it's unsupported.
> +
> +gdb_caching_proc can_spawn_for_attach_1 {} {
> + # Assume yes.
> + set res 1
> +
> + set me "can_spawn_for_attach"
> + set src { int main (void) { sleep (600); return 0; } }
Don't we need a header include for sleep?
> + if {![gdb_simple_compile $me $src executable]} {
> + return $res
> + }
> +
> + set test_spawn_id [spawn_wait_for_attach_1 $obj]
> + file delete $obj
I think this should be a remote_file call, not a build file delete.
can_spawn_wait_for_attach returns false if the target is remote,
not if the host is remote.
> +
> + gdb_start
> +
> + set test_pid [spawn_id_get_pid $test_spawn_id]
> + set attaching_re "Attaching to process $test_pid"
> + gdb_test_multiple "attach $test_pid" "can spawn for attach" {
> + -re -wrap "$attaching_re\r\n.*ptrace: Operation not permitted\\." {
> + # Not permitted.
> + set res 0
> + }
> + -re -wrap "" {
> + # Don't know.
> + }
> + }
> +
> + gdb_exit
The comment below suggests this gdb_exit shouldn't be here.
> +
> + kill_wait_spawned_process $test_spawn_id
> +
> + return $res
> +}
> +
> # Return true if we can spawn a program on the target and attach to
> # it.
>
> @@ -6151,8 +6188,25 @@ proc can_spawn_for_attach { } {
> return 0
> }
>
> - # Assume yes.
> - return 1
> + # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
> + # unpredictable which test-case will call it first, and consequently a
> + # test-case may pass in say a full test run, but fail when run
> + # individually, due to a can_spawn_for_attach call in location where a
> + # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
> + # To avoid this, we move the initial gdb_exit out of
> + # can_spawn_for_attach_1, guaranteeing that we end up in the same state
> + # regardless of whether can_spawn_for_attach_1 is called. However, that
> + # is only necessary for the first call in a test-case, so cache the result
> + # in a global (which should be reset after each test-case) to keep track
> + # of that.
> + global cache_can_spawn_for_attach_1
> + if { [info exists cache_can_spawn_for_attach_1] } {
> + return $cache_can_spawn_for_attach_1
> + }
> + gdb_exit
There is still a gdb_exit call in can_spawn_for_attach_1.
I guess you meant to remove it?
I think it'll also be good to document that can_spawn_for_attach calls gdb_exit.
> +
> + set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
> + return $cache_can_spawn_for_attach_1
> }
>
> # Centralize the failure checking of "attach" command.
> @@ -6265,20 +6319,12 @@ proc spawn_id_get_pid { spawn_id } {
> return $testpid
> }
>
> -# Start a set of programs running and then wait for a bit, to be sure
> -# that they can be attached to. Return a list of processes spawn IDs,
> -# one element for each process spawned. It's a test error to call
> -# this when [can_spawn_for_attach] is false.
> +# Helper function for spawn_wait_for_attach
... and can_spawn_for_attach_1. Just spawn_wait_for_attach by itself
wouldn't need the helper.
> As spawn_wait_for_attach, but
> +# doesn't check for can_spawn_for_attach.
>
More information about the Gdb-patches
mailing list