Re: [PATCH v2] [gdb/testsuite] Handle ptrace operation not permitted in can_spawn_for_attach

[Pedro Alves <pedro@palves.net>]

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.
>