[PATCH] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[PATCH] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

Some gdb.base/fileio.exp tests expect the inferior to not have write
access to some files. If the test is being run as root, this is never
possible. This commit adds a way to identify if the user is root and
xfails the tests that expect no write access.
---
 gdb/testsuite/gdb.base/fileio.exp |  9 ++++++++-
 gdb/testsuite/lib/gdb.exp         | 11 +++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 33c88d064c4..e1c7a7f955e 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -17,7 +17,6 @@
 
 require {!target_info exists gdb,nofileio}
 
-
 standard_testfile
 
 if {[is_remote host]} {
@@ -75,6 +74,10 @@ gdb_test "continue" ".*" ""
 
 catch "system \"chmod -f -w [standard_output_file nowrt.fileio.test]\""
 
+# If the user is root, we will always have write permission
+if { [root_user] } {
+    setup_xfail *-*
+}
 gdb_test continue \
 "Continuing\\..*open 5:.*EACCES$stop_msg" \
 "Open for write but no write permission returns EACCES"
@@ -240,6 +243,10 @@ gdb_test continue \
 if [ishost *cygwin*] {
     setup_xfail "*-*-*"
 }
+# If the user is root, we will always have write permission
+if { [root_user] } {
+    setup_xfail *-*
+}
 gdb_test continue \
 "Continuing\\..*unlink 2:.*EACCES$stop_msg" \
 "Unlinking a file in a directory w/o write access returns EACCES"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 36bf738c667..de98f8bb2ac 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9952,5 +9952,16 @@ proc have_host_locale { locale } {
     return [expr $idx != -1]
 }
 
+# Return 1 if the test is being run as root, 0 otherwise
+
+gdb_caching_proc root_user {} {
+    # ID outputs to stdout, we have to use exec to capture it here
+    set user [exec id]
+
+    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
+
+    return [expr $uid == 0]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
-- 
2.41.0

Re: [PATCH] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Lancelot SIX]

Hi Guinevere,

I am not a big fan of the idea of running the testcases as root… but I
guess this is what you have when running within a container.

> +# Return 1 if the test is being run as root, 0 otherwise
> +
> +gdb_caching_proc root_user {} {
> +    # ID outputs to stdout, we have to use exec to capture it here
> +    set user [exec id]

Shouldn't it be `[remote_exec target]` instead?  You are interested in
who will end up running the test executable, which can be different from
who runs `runtest` locally.  I am not entirely sure if it should be
`host` or `target`.  I would think `target`, but the testcase does use
`host`, which seems odd to me.

Also, what happens if the underlying filesystem is for example a NFS
mount?  I am not sure being root will change what unlink can do, but I
have not tested.  Maybe it is not worth trying to figure out all setups,
if it comes up one can always adjust the test.

Best,
Lancelot.

> +
> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
> +
> +    return [expr $uid == 0]
> +}
> +
>  # Always load compatibility stuff.
>  load_lib future.exp
> -- 
> 2.41.0
> 

Re: [PATCH] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

On 11/08/2023 15:19, Lancelot SIX wrote:
> Hi Guinevere,
>
> I am not a big fan of the idea of running the testcases as root… but I
> guess this is what you have when running within a container.
yeah, this is focused on VM/containers for testing infrastructure.
>
>> +# Return 1 if the test is being run as root, 0 otherwise
>> +
>> +gdb_caching_proc root_user {} {
>> +    # ID outputs to stdout, we have to use exec to capture it here
>> +    set user [exec id]
> Shouldn't it be `[remote_exec target]` instead?  You are interested in
> who will end up running the test executable, which can be different from
> who runs `runtest` locally.  I am not entirely sure if it should be
> `host` or `target`.  I would think `target`, but the testcase does use
> `host`, which seems odd to me.
Thank you. I always forget about how the remote stuff works, but I think 
you are correct, it does make more sense to use remote_exec, because the 
test failures are based on if the underlying program can access it or 
not. I'll fix this in v2.
>
> Also, what happens if the underlying filesystem is for example a NFS
> mount?  I am not sure being root will change what unlink can do, but I
> have not tested.  Maybe it is not worth trying to figure out all setups,
> if it comes up one can always adjust the test.
I think you're right, being root wouldn't let you access those, but I am 
not sure many people are testing GDB with an NFS mounted build directory 
inside a VM/container. I think setups like that can be dealt with if 
they appear.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Best,
> Lancelot.
>
>> +
>> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
>> +
>> +    return [expr $uid == 0]
>> +}
>> +
>>   # Always load compatibility stuff.
>>   load_lib future.exp
>> -- 
>> 2.41.0
>>

[PATCH v2] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

Some gdb.base/fileio.exp tests expect the inferior to not have write
access to some files. If the test is being run as root, this is never
possible. This commit adds a way to identify if the user is root and
xfails the tests that expect no write access.
---

Changes for v2:
* root_user now uses remote_exec instead of exec to see the uid of the
  inferior

---
 gdb/testsuite/gdb.base/fileio.exp |  9 ++++++++-
 gdb/testsuite/lib/gdb.exp         | 11 +++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 33c88d064c4..e1c7a7f955e 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -17,7 +17,6 @@
 
 require {!target_info exists gdb,nofileio}
 
-
 standard_testfile
 
 if {[is_remote host]} {
@@ -75,6 +74,10 @@ gdb_test "continue" ".*" ""
 
 catch "system \"chmod -f -w [standard_output_file nowrt.fileio.test]\""
 
+# If the user is root, we will always have write permission
+if { [root_user] } {
+    setup_xfail *-*
+}
 gdb_test continue \
 "Continuing\\..*open 5:.*EACCES$stop_msg" \
 "Open for write but no write permission returns EACCES"
@@ -240,6 +243,10 @@ gdb_test continue \
 if [ishost *cygwin*] {
     setup_xfail "*-*-*"
 }
+# If the user is root, we will always have write permission
+if { [root_user] } {
+    setup_xfail *-*
+}
 gdb_test continue \
 "Continuing\\..*unlink 2:.*EACCES$stop_msg" \
 "Unlinking a file in a directory w/o write access returns EACCES"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 36bf738c667..d7947e0dcf4 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9952,5 +9952,16 @@ proc have_host_locale { locale } {
     return [expr $idx != -1]
 }
 
+# Return 1 if the test is being run as root, 0 otherwise
+
+gdb_caching_proc root_user {} {
+    # ID outputs to stdout, we have to use exec to capture it here
+    set user [remote_exec id]
+
+    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
+
+    return [expr $uid == 0]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
-- 
2.41.0

[PING][PATCH v2] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

Ping!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

On 14/08/2023 12:24, Guinevere Larsen wrote:
> Some gdb.base/fileio.exp tests expect the inferior to not have write
> access to some files. If the test is being run as root, this is never
> possible. This commit adds a way to identify if the user is root and
> xfails the tests that expect no write access.
> ---
>
> Changes for v2:
> * root_user now uses remote_exec instead of exec to see the uid of the
>    inferior
>
> ---
>   gdb/testsuite/gdb.base/fileio.exp |  9 ++++++++-
>   gdb/testsuite/lib/gdb.exp         | 11 +++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
> index 33c88d064c4..e1c7a7f955e 100644
> --- a/gdb/testsuite/gdb.base/fileio.exp
> +++ b/gdb/testsuite/gdb.base/fileio.exp
> @@ -17,7 +17,6 @@
>   
>   require {!target_info exists gdb,nofileio}
>   
> -
>   standard_testfile
>   
>   if {[is_remote host]} {
> @@ -75,6 +74,10 @@ gdb_test "continue" ".*" ""
>   
>   catch "system \"chmod -f -w [standard_output_file nowrt.fileio.test]\""
>   
> +# If the user is root, we will always have write permission
> +if { [root_user] } {
> +    setup_xfail *-*
> +}
>   gdb_test continue \
>   "Continuing\\..*open 5:.*EACCES$stop_msg" \
>   "Open for write but no write permission returns EACCES"
> @@ -240,6 +243,10 @@ gdb_test continue \
>   if [ishost *cygwin*] {
>       setup_xfail "*-*-*"
>   }
> +# If the user is root, we will always have write permission
> +if { [root_user] } {
> +    setup_xfail *-*
> +}
>   gdb_test continue \
>   "Continuing\\..*unlink 2:.*EACCES$stop_msg" \
>   "Unlinking a file in a directory w/o write access returns EACCES"
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 36bf738c667..d7947e0dcf4 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9952,5 +9952,16 @@ proc have_host_locale { locale } {
>       return [expr $idx != -1]
>   }
>   
> +# Return 1 if the test is being run as root, 0 otherwise
> +
> +gdb_caching_proc root_user {} {
> +    # ID outputs to stdout, we have to use exec to capture it here
> +    set user [remote_exec id]
> +
> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
> +
> +    return [expr $uid == 0]
> +}
> +
>   # Always load compatibility stuff.
>   load_lib future.exp

Re: [PATCH v2] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Tom de Vries]

On 8/14/23 12:24, Guinevere Larsen via Gdb-patches wrote:
> Some gdb.base/fileio.exp tests expect the inferior to not have write
> access to some files. If the test is being run as root, this is never
> possible. This commit adds a way to identify if the user is root and
> xfails the tests that expect no write access.

Hi,

thanks for working on this problem.  I also ran into this while running 
the gdb testsuite as root inside a container.

> ---
> 
> Changes for v2:
> * root_user now uses remote_exec instead of exec to see the uid of the
>    inferior
> 
> ---
>   gdb/testsuite/gdb.base/fileio.exp |  9 ++++++++-
>   gdb/testsuite/lib/gdb.exp         | 11 +++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
> index 33c88d064c4..e1c7a7f955e 100644
> --- a/gdb/testsuite/gdb.base/fileio.exp
> +++ b/gdb/testsuite/gdb.base/fileio.exp
> @@ -17,7 +17,6 @@
>   
>   require {!target_info exists gdb,nofileio}
>   
> -
>   standard_testfile
>   
>   if {[is_remote host]} {
> @@ -75,6 +74,10 @@ gdb_test "continue" ".*" ""
>   
>   catch "system \"chmod -f -w [standard_output_file nowrt.fileio.test]\""
>   
> +# If the user is root, we will always have write permission

Dot at end-of-line missing.

> +if { [root_user] } {
> +    setup_xfail *-*
> +}

This also works, but the usual idiom seems to be '*-*-*', so maybe we 
should also use that here.  Just a suggestion though.

>   gdb_test continue \
>   "Continuing\\..*open 5:.*EACCES$stop_msg" \
>   "Open for write but no write permission returns EACCES"
> @@ -240,6 +243,10 @@ gdb_test continue \
>   if [ishost *cygwin*] {
>       setup_xfail "*-*-*"
>   }
> +# If the user is root, we will always have write permission

Dot at end-of-line missing.

> +if { [root_user] } {
> +    setup_xfail *-*
> +}
>   gdb_test continue \
>   "Continuing\\..*unlink 2:.*EACCES$stop_msg" \
>   "Unlinking a file in a directory w/o write access returns EACCES"
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 36bf738c667..d7947e0dcf4 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9952,5 +9952,16 @@ proc have_host_locale { locale } {
>       return [expr $idx != -1]
>   }
>   
> +# Return 1 if the test is being run as root, 0 otherwise

Dot at end-of-line missing.

> +
> +gdb_caching_proc root_user {} {
> +    # ID outputs to stdout, we have to use exec to capture it here
> +    set user [remote_exec id]

This should be "[remote_exec target id]".

> +
> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid

remote_exec returns a list of exit val and output.  The usual thing to 
do is to break that apart and handle the exit val != 0 explicitly, and 
do the grepping in the output part.

For now I think it's ok to return 0 for the exit val != 0 case, but at 
some point we may want to return -1, to be able to check that the user 
is not root by checking that { [root_user] == 0 }.

Thanks,
- Tom

[PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

Some gdb.base/fileio.exp tests expect the inferior to not have write
access to some files. If the test is being run as root, this is never
possible. This commit adds a way to identify if the user is root and
xfails the tests that expect no write access.
---

Changes for v3:
* root_user can now identify if the "id" command failed. It returns that
  the user is not root, so that issues are more easily identified.
* Fix issues pointed out by Tom de Vries.

Changes for v2:
* root_user now uses remote_exec instead of exec to see the uid of the
   inferior

---
 gdb/testsuite/gdb.base/fileio.exp |  9 ++++++++-
 gdb/testsuite/lib/gdb.exp         | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/fileio.exp
index 33c88d064c4..6a708ed00ed 100644
--- a/gdb/testsuite/gdb.base/fileio.exp
+++ b/gdb/testsuite/gdb.base/fileio.exp
@@ -17,7 +17,6 @@
 
 require {!target_info exists gdb,nofileio}
 
-
 standard_testfile
 
 if {[is_remote host]} {
@@ -75,6 +74,10 @@ gdb_test "continue" ".*" ""
 
 catch "system \"chmod -f -w [standard_output_file nowrt.fileio.test]\""
 
+# If the user is root, we will always have write permission.
+if { [root_user] } {
+    setup_xfail *-*-*
+}
 gdb_test continue \
 "Continuing\\..*open 5:.*EACCES$stop_msg" \
 "Open for write but no write permission returns EACCES"
@@ -240,6 +243,10 @@ gdb_test continue \
 if [ishost *cygwin*] {
     setup_xfail "*-*-*"
 }
+# If the user is root, we will always have write permission.
+if { [root_user] } {
+    setup_xfail *-*-*
+}
 gdb_test continue \
 "Continuing\\..*unlink 2:.*EACCES$stop_msg" \
 "Unlinking a file in a directory w/o write access returns EACCES"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index de22da8d8a8..5b90b8af3b8 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9981,5 +9981,25 @@ gdb_caching_proc have_system_header { file } {
     return [gdb_can_simple_compile have_system_header_$name $src object]
 }
 
+# Return 1 if the test is being run as root, 0 otherwise.
+
+gdb_caching_proc root_user {} {
+    # ID outputs to stdout, we have to use exec to capture it here.
+    set user [remote_exec target id]
+
+    regexp -all "(-?\[0-9\]+).*" $user user ret_val
+
+    # If ret_val is not 0, we couldn't run `id` on the target for some
+    # reason.  Return that we are not root, so problems are easier to
+    # spot.
+    if {[expr $ret_val != 0]} {
+	return 0
+    }
+
+    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
+
+    return [expr $uid == 0]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
-- 
2.41.0

Re: [PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Tom de Vries]

On 10/4/23 16:02, Guinevere Larsen via Gdb-patches wrote:
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index de22da8d8a8..5b90b8af3b8 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9981,5 +9981,25 @@ gdb_caching_proc have_system_header { file } {
>       return [gdb_can_simple_compile have_system_header_$name $src object]
>   }
>   
> +# Return 1 if the test is being run as root, 0 otherwise.
> +
> +gdb_caching_proc root_user {} {
> +    # ID outputs to stdout, we have to use exec to capture it here.
> +    set user [remote_exec target id]
> +
> +    regexp -all "(-?\[0-9\]+).*" $user user ret_val

I can see that this works, but it's a bit unusual (and hard to parse for 
me) to use regexp to pick apart a list. The usual way is using lindex.

> +
> +    # If ret_val is not 0, we couldn't run `id` on the target for some
> +    # reason.  Return that we are not root, so problems are easier to
> +    # spot.
> +    if {[expr $ret_val != 0]} {

The expr is not necessary here.

> +	return 0
> +    }
> +
> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
> +

Also, the 'user' variable is used as dummy, better make that explicit.

So, please do:
...
     set res [remote_exec target id]
     set ret_val [lindex $res 0]
     set output [lindex $res 1]

     # If ret_val is not 0, we couldn't run `id` on the target for some 

     # reason.  Return that we are not root, so problems are easier to 

     # spot. 

     if { $ret_val != 0 } {
         return 0
     }

     regexp -all ".*uid=(\[0-9\]+).*" $output dummy uid

     return [expr $uid == 0]
...

Approved with that change.

Approved-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom


> +    return [expr $uid == 0]
> +}
> +
>   # Always load compatibility stuff.
>   load_lib future.exp

Re: [PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp

[Guinevere Larsen]

On 04/10/2023 17:33, Tom de Vries wrote:
> On 10/4/23 16:02, Guinevere Larsen via Gdb-patches wrote:
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index de22da8d8a8..5b90b8af3b8 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -9981,5 +9981,25 @@ gdb_caching_proc have_system_header { file } {
>>       return [gdb_can_simple_compile have_system_header_$name $src 
>> object]
>>   }
>>   +# Return 1 if the test is being run as root, 0 otherwise.
>> +
>> +gdb_caching_proc root_user {} {
>> +    # ID outputs to stdout, we have to use exec to capture it here.
>> +    set user [remote_exec target id]
>> +
>> +    regexp -all "(-?\[0-9\]+).*" $user user ret_val
>
> I can see that this works, but it's a bit unusual (and hard to parse 
> for me) to use regexp to pick apart a list. The usual way is using 
> lindex.

That's a good point.

TCL lists confuse me a lot, and the original plan was to have this along 
with the uid picking part but that failed for other reasons when the 
command failed.

>
>> +
>> +    # If ret_val is not 0, we couldn't run `id` on the target for some
>> +    # reason.  Return that we are not root, so problems are easier to
>> +    # spot.
>> +    if {[expr $ret_val != 0]} {
>
> The expr is not necessary here.
>
>> +    return 0
>> +    }
>> +
>> +    regexp -all ".*uid=(\[0-9\]+).*" $user user uid
>> +
>
> Also, the 'user' variable is used as dummy, better make that explicit.
>
> So, please do:
> ...
>     set res [remote_exec target id]
>     set ret_val [lindex $res 0]
>     set output [lindex $res 1]
>
>     # If ret_val is not 0, we couldn't run `id` on the target for some
>     # reason.  Return that we are not root, so problems are easier to
>     # spot.
>     if { $ret_val != 0 } {
>         return 0
>     }
>
>     regexp -all ".*uid=(\[0-9\]+).*" $output dummy uid
>
>     return [expr $uid == 0]
> ...
>
> Approved with that change.
Thanks! I applied your change and pushed it.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Approved-By: Tom de Vries <tdevries@suse.de>
>
> Thanks,
> - Tom
>
>
>> +    return [expr $uid == 0]
>> +}
>> +
>>   # Always load compatibility stuff.
>>   load_lib future.exp
>