t4129 failure when sticky bit set
The new test (do not use core.sharedRepository for working tree files) in t4129-apply-sammode.sh does not account for sticky bits set:
--- d_mode.expected 2020-12-30 11:56:11.869555700 +0000
+++ d_mode.actual 2020-12-30 11:56:11.872889055 +0000
@@ -1 +1 @@
-drwx------
+drwx--S---
This sticky bit (g+s) is set on my home dir and thus inherrited.
PATCH 1/1 t4129: don't fail if setgid is set in the parent directory
The last test of t4129 creates a directory and expects its setgid bit (g+s) to be off. But this makes the test fail when the parent directory has the bit set, as setgid's state is inherited by newly created subdirectories. Make the test more robust by accepting the presence of the setgid bit on the created directory. We only allow 'S' (setgid on but no executable permission) and not 's' (setgid on with executable permission) because the previous 'umask 0077' shouldn't allow the second scenario to happen.
Note that only subdirectories inherit this bit, so we don't have to make the same change for the regular file that is also created by this test. But checking the permissions using grep instead of test_cmp makes the test a little simpler, so let's use it for the regular file as well.
Also note that the sticky bit (+t) and the setuid bit (u+s) are not inherited, so we don't have to worry about those.
--- t/t4129-apply-samemode.sh | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
t/t4129-apply-samemode.sh +4 -6
@@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree rm -rf d f1 && git apply patch-f1-and-f2.txt && - echo "-rw-------" >f1_mode.expected && - echo "drwx------" >d_mode.expected && - test_modebits f1 >f1_mode.actual && - test_modebits d >d_mode.actual && - test_cmp f1_mode.expected f1_mode.actual && - test_cmp d_mode.expected d_mode.actual + test_modebits f1 >f1_mode && + test_modebits d >d_mode && + grep "^-rw-------$" f1_mode && + grep "^drwx--[-S]---$" d_mode ) '
Signature · 1 line
2.29.2
Re: [PATCH] t4129: don't fail if setgid is set in the parent directory
Quoted 47 lines · Matheus Tavares
The last test of t4129 creates a directory and expects its setgid bit (g+s) to be off. But this makes the test fail when the parent directory has the bit set, as setgid's state is inherited by newly created subdirectories. Make the test more robust by accepting the presence of the setgid bit on the created directory. We only allow 'S' (setgid on but no executable permission) and not 's' (setgid on with executable permission) because the previous 'umask 0077' shouldn't allow the second scenario to happen.
Note that only subdirectories inherit this bit, so we don't have to make the same change for the regular file that is also created by this test. But checking the permissions using grep instead of
test_cmpmakes the test a little simpler, so let's use it for the regular file as well.Also note that the sticky bit (+t) and the setuid bit (u+s) are not inherited, so we don't have to worry about those.
Reported-by Kevin Daudt <[email protected]>Signed-off-by Matheus Tavares <[email protected]>--- t/t4129-apply-samemode.sh | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)t/t4129-apply-samemode.sh +4
-6@@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree rm -rf d f1 && git apply patch-f1-and-f2.txt && - echo "-rw-------" >f1_mode.expected && - echo "drwx------" >d_mode.expected && - test_modebits f1 >f1_mode.actual && - test_modebits d >d_mode.actual && - test_cmp f1_mode.expected f1_mode.actual && - test_cmp d_mode.expected d_mode.actual + test_modebits f1 >f1_mode && + test_modebits d >d_mode && + grep "^-rw-------$" f1_mode && + grep "^drwx--[-S]---$" d_mode ) 'Signature · 1 line
2.29.2
Re: [PATCH] t4129: don't fail if setgid is set in the parent directory
In reply to Matheus TavaresQuoted 10 lines · Matheus Tavares
t/t4129-apply-samemode.sh +0
-2@@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree rm -rf d f1 && git apply patch-f1-and-f2.txt && - echo "-rw-------" >f1_mode.expected && - echo "drwx------" >d_mode.expected &&
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 98
Quoted 10 lines
t/t4129-apply-samemode.sh +4
-4- test_modebits f1 >f1_mode.actual && - test_modebits d >d_mode.actual && - test_cmp f1_mode.expected f1_mode.actual && - test_cmp d_mode.expected d_mode.actual + test_modebits f1 >f1_mode && + test_modebits d >d_mode && + grep "^-rw-------$" f1_mode && + grep "^drwx--[-S]---$" d_mode ) '
It somehow feels to me that this approach would not scale well. Shouldn't this knowledge of inherited sticky gid bit hidden behind the test_modebits helper function?
Thanks.
PATCH v2 1/1 t4129: don't fail if setgid is set in the test directory
In reply to Matheus TavaresThe last test of t4129 creates a directory and expects its setgid bit (g+s) to be off. But this makes the test fail when the parent directory has the bit set, as setgid's state is inherited by newly created subdirectories.
One way to solve this problem is to allow the presence of this bit when comparing the return of test_modebits with the expected value. But then we may have the same problem in the future when other tests start using test_modebits on directories (currently t4129 is the only one) and forget about setgid. Instead, let's make the helper function more robust with respect to the state of the setgid bit in the test directory by removing this bit from the returning value. There should be no problem with existing callers as no one currently expects this bit to be on.
Note that the sticky bit (+t) and the setuid bit (u+s) are not inherited, so we don't have to worry about those.
--- t/test-lib-functions.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
t/test-lib-functions.sh +7 -2
@@ -367,9 +367,14 @@ test_chmod () { git update-index --add "--chmod=$@" } -# Get the modebits from a file or directory. +# Get the modebits from a file or directory, ignoring the setgid bit (g+s). +# This bit is inherited by subdirectories at their creation. So we remove it +# from the returning string to prevent callers from having to worry about the +# state of the bit in the test directory. +# test_modebits () { - ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|' } # Unset a configuration variable, but don't fail if it doesn't exist.
Signature · 1 line
2.29.2
Re: [PATCH v2] t4129: don't fail if setgid is set in the test directory
t/test-lib-functions.sh test_chmod () { · line 374
Quoted 5 lines · Matheus Tavares
t/test-lib-functions.sh +5
-0+# Get the modebits from a file or directory, ignoring the setgid bit (g+s). +# This bit is inherited by subdirectories at their creation. So we remove it +# from the returning string to prevent callers from having to worry about the +# state of the bit in the test directory. +#
We probably do not use "chmod g+s" manually on regular files, so I may be being overly "correct", but shouldn't these be done only for directories?
t/test-lib-functions.sh test_chmod () { · line 377
Quoted 4 lines
test_modebits() {t/test-lib-functions.sh +2
-1- ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|'
That is,
-e 's|^\(d.....\)S|\1-|' -e 's|^\(d.....\)s|\1x|'
instead of applying the rule to any filetype.
Will queue as-is, as the distinction probably would not matter in practice.
Thanks.
Re: [PATCH v2] t4129: don't fail if setgid is set in the test directory
Quoted 18 lines · Junio C Hamano
t/test-lib-functions.sh test_chmod () { · line 374
Quoted 5 lines · Matheus Tavares
t/test-lib-functions.sh +5
-0+# Get the modebits from a file or directory, ignoring the setgid bit (g+s). +# This bit is inherited by subdirectories at their creation. So we remove it +# from the returning string to prevent callers from having to worry about the +# state of the bit in the test directory. +#We probably do not use "chmod g+s" manually on regular files, so I may be being overly "correct", but shouldn't these be done only for directories?
t/test-lib-functions.sh test_chmod () { · line 377
Quoted 4 lines
test_modebits() {t/test-lib-functions.sh +2
-1- ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|'
Quoted 5 lines
That is,
-e 's|^\(d.....\)S|\1-|' -e 's|^\(d.....\)s|\1x|'instead of applying the rule to any filetype.
Yeah, you're right. I ended up applying the rule on regular files as well just for standardization. That is, if some day, for some reason, a test script decides to use "chmod g+s" on a regular file and a directory, test_modebits would treat them equally to avoid any confusion. But I guess it's very unlikely that we will ever need to set the setgid bit on a test, anyway...
Quoted 4 lines
Will queue as-is, as the distinction probably would not matter in practice.
Thanks.