* [PING][PATCH v2] gdb/testsuite: XFAIL some gdb.base/fileio.exp
2023-08-14 10:24 ` [PATCH v2] " Guinevere Larsen
@ 2023-09-14 13:12 ` Guinevere Larsen
2023-09-26 13:21 ` [PATCH " Tom de Vries
2023-10-04 14:02 ` [PATCH v3] " Guinevere Larsen
2 siblings, 0 replies; 9+ messages in thread
From: Guinevere Larsen @ 2023-09-14 13:12 UTC (permalink / raw)
To: gdb-patches, Guinevere Larsen; +Cc: lsix
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] gdb/testsuite: XFAIL some gdb.base/fileio.exp
2023-08-14 10:24 ` [PATCH v2] " Guinevere Larsen
2023-09-14 13:12 ` [PING][PATCH " Guinevere Larsen
@ 2023-09-26 13:21 ` Tom de Vries
2023-10-04 14:02 ` [PATCH v3] " Guinevere Larsen
2 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-09-26 13:21 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: lsix
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp
2023-08-14 10:24 ` [PATCH v2] " Guinevere Larsen
2023-09-14 13:12 ` [PING][PATCH " Guinevere Larsen
2023-09-26 13:21 ` [PATCH " Tom de Vries
@ 2023-10-04 14:02 ` Guinevere Larsen
2023-10-04 15:33 ` Tom de Vries
2 siblings, 1 reply; 9+ messages in thread
From: Guinevere Larsen @ 2023-10-04 14:02 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp
2023-10-04 14:02 ` [PATCH v3] " Guinevere Larsen
@ 2023-10-04 15:33 ` Tom de Vries
2023-10-04 15:47 ` Guinevere Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-10-04 15:33 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] gdb/testsuite: XFAIL some gdb.base/fileio.exp
2023-10-04 15:33 ` Tom de Vries
@ 2023-10-04 15:47 ` Guinevere Larsen
0 siblings, 0 replies; 9+ messages in thread
From: Guinevere Larsen @ 2023-10-04 15:47 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread