{"thread":{"id":"54905","subject":"t4129 failure when sticky bit set","startedAt":"2020-12-30T12:14:17Z","lastAt":"2021-01-09T14:20:15Z","messageCount":7,"participants":["Kevin Daudt","Matheus Tavares","Junio C Hamano","Matheus Tavares Bernardino"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"413168","messageId":"X+xtAR87vWuNiLoE@alpha","threadId":"54905","inReplyTo":null,"subject":"t4129 failure when sticky bit set","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2020-12-30T12:05:21Z","receivedAt":"2020-12-30T12:14:17Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"The new test (do not use core.sharedRepository for working tree files)\nin t4129-apply-sammode.sh does not account for sticky bits set:\n\n  --- d_mode.expected     2020-12-30 11:56:11.869555700 +0000\n  +++ d_mode.actual       2020-12-30 11:56:11.872889055 +0000\n  @@ -1 +1 @@                                                \n  -drwx------                                                \n  +drwx--S---                                                \n\nThis sticky bit (g+s) is set on my home dir and thus inherrited.\n"},{"id":"413172","messageId":"88398ff952a68e8d134dcd50ef0772bb6fc3b456.1609339792.git.matheus.bernardino@usp.br","threadId":"54905","inReplyTo":"X+xtAR87vWuNiLoE@alpha","subject":"[PATCH] t4129: don't fail if setgid is set in the parent directory","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-12-30T14:52:25Z","receivedAt":"2020-12-30T14:53:35Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The last test of t4129 creates a directory and expects its setgid bit\n(g+s) to be off. But this makes the test fail when the parent directory\nhas the bit set, as setgid's state is inherited by newly created\nsubdirectories. Make the test more robust by accepting the presence of\nthe setgid bit on the created directory. We only allow 'S' (setgid on\nbut no executable permission) and not 's' (setgid on with executable\npermission) because the previous 'umask 0077' shouldn't allow the second\nscenario to happen.\n\nNote that only subdirectories inherit this bit, so we don't have to make\nthe same change for the regular file that is also created by this test.\nBut checking the permissions using grep instead of test_cmp makes the\ntest a little simpler, so let's use it for the regular file as well.\n\nAlso note that the sticky bit (+t) and the setuid bit (u+s) are not\ninherited, so we don't have to worry about those.\n\nReported-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/t4129-apply-samemode.sh | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 41818d8315..3818398ca9 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t\trm -rf d f1 &&\n \t\tgit apply patch-f1-and-f2.txt &&\n \n-\t\techo \"-rw-------\" >f1_mode.expected &&\n-\t\techo \"drwx------\" >d_mode.expected &&\n-\t\ttest_modebits f1 >f1_mode.actual &&\n-\t\ttest_modebits d >d_mode.actual &&\n-\t\ttest_cmp f1_mode.expected f1_mode.actual &&\n-\t\ttest_cmp d_mode.expected d_mode.actual\n+\t\ttest_modebits f1 >f1_mode &&\n+\t\ttest_modebits d >d_mode &&\n+\t\tgrep \"^-rw-------$\" f1_mode &&\n+\t\tgrep \"^drwx--[-S]---$\" d_mode\n \t)\n '\n \n-- \n2.29.2\n\n"},{"id":"413199","messageId":"X+zrryp6ndOa5rOM@alpha","threadId":"54905","inReplyTo":"88398ff952a68e8d134dcd50ef0772bb6fc3b456.1609339792.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] t4129: don't fail if setgid is set in the parent directory","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2020-12-30T21:05:51Z","receivedAt":"2020-12-30T21:06:34Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Wed, Dec 30, 2020 at 11:52:25AM -0300, Matheus Tavares wrote:\n> The last test of t4129 creates a directory and expects its setgid bit\n> (g+s) to be off. But this makes the test fail when the parent directory\n> has the bit set, as setgid's state is inherited by newly created\n> subdirectories. Make the test more robust by accepting the presence of\n> the setgid bit on the created directory. We only allow 'S' (setgid on\n> but no executable permission) and not 's' (setgid on with executable\n> permission) because the previous 'umask 0077' shouldn't allow the second\n> scenario to happen.\n> \n> Note that only subdirectories inherit this bit, so we don't have to make\n> the same change for the regular file that is also created by this test.\n> But checking the permissions using grep instead of test_cmp makes the\n> test a little simpler, so let's use it for the regular file as well.\n> \n> Also note that the sticky bit (+t) and the setuid bit (u+s) are not\n> inherited, so we don't have to worry about those.\n> \n> Reported-by: Kevin Daudt <me@ikke.info>\n> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> ---\n>  t/t4129-apply-samemode.sh | 10 ++++------\n>  1 file changed, 4 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index 41818d8315..3818398ca9 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n>  \t\trm -rf d f1 &&\n>  \t\tgit apply patch-f1-and-f2.txt &&\n>  \n> -\t\techo \"-rw-------\" >f1_mode.expected &&\n> -\t\techo \"drwx------\" >d_mode.expected &&\n> -\t\ttest_modebits f1 >f1_mode.actual &&\n> -\t\ttest_modebits d >d_mode.actual &&\n> -\t\ttest_cmp f1_mode.expected f1_mode.actual &&\n> -\t\ttest_cmp d_mode.expected d_mode.actual\n> +\t\ttest_modebits f1 >f1_mode &&\n> +\t\ttest_modebits d >d_mode &&\n> +\t\tgrep \"^-rw-------$\" f1_mode &&\n> +\t\tgrep \"^drwx--[-S]---$\" d_mode\n>  \t)\n>  '\n>  \n> -- \n> 2.29.2\n> \n\nTested-by: Kevin Daudt <me@ikke.info>\n"},{"id":"413412","messageId":"xmqqpn2k1ci0.fsf@gitster.c.googlers.com","threadId":"54905","inReplyTo":"88398ff952a68e8d134dcd50ef0772bb6fc3b456.1609339792.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] t4129: don't fail if setgid is set in the parent directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-04T23:57:43Z","receivedAt":"2021-01-04T23:58:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares <matheus.bernardino@usp.br> writes:\n\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index 41818d8315..3818398ca9 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -90,12 +90,10 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n>  \t\trm -rf d f1 &&\n>  \t\tgit apply patch-f1-and-f2.txt &&\n>  \n> -\t\techo \"-rw-------\" >f1_mode.expected &&\n> -\t\techo \"drwx------\" >d_mode.expected &&\n> -\t\ttest_modebits f1 >f1_mode.actual &&\n> -\t\ttest_modebits d >d_mode.actual &&\n> -\t\ttest_cmp f1_mode.expected f1_mode.actual &&\n> -\t\ttest_cmp d_mode.expected d_mode.actual\n> +\t\ttest_modebits f1 >f1_mode &&\n> +\t\ttest_modebits d >d_mode &&\n> +\t\tgrep \"^-rw-------$\" f1_mode &&\n> +\t\tgrep \"^drwx--[-S]---$\" d_mode\n>  \t)\n>  '\n\nIt somehow feels to me that this approach would not scale well.\nShouldn't this knowledge of inherited sticky gid bit hidden behind\nthe test_modebits helper function?\n\nThanks.\n"},{"id":"413462","messageId":"b734425e3235651e738e6eac47eae0db7db92e7e.1609861567.git.matheus.bernardino@usp.br","threadId":"54905","inReplyTo":"88398ff952a68e8d134dcd50ef0772bb6fc3b456.1609339792.git.matheus.bernardino@usp.br","subject":"[PATCH v2] t4129: don't fail if setgid is set in the test directory","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-01-05T15:47:39Z","receivedAt":"2021-01-05T15:48:45Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The last test of t4129 creates a directory and expects its setgid bit\n(g+s) to be off. But this makes the test fail when the parent directory\nhas the bit set, as setgid's state is inherited by newly created\nsubdirectories.\n\nOne way to solve this problem is to allow the presence of this bit when\ncomparing the return of `test_modebits` with the expected value. But\nthen we may have the same problem in the future when other tests start\nusing `test_modebits` on directories (currently t4129 is the only one)\nand forget about setgid. Instead, let's make the helper function more\nrobust with respect to the state of the setgid bit in the test directory\nby removing this bit from the returning value. There should be no\nproblem with existing callers as no one currently expects this bit to be\non.\n\nNote that the sticky bit (+t) and the setuid bit (u+s) are not\ninherited, so we don't have to worry about those.\n\nReported-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/test-lib-functions.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a..2f08ce7cba 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -367,9 +367,14 @@ test_chmod () {\n \tgit update-index --add \"--chmod=$@\"\n }\n \n-# Get the modebits from a file or directory.\n+# Get the modebits from a file or directory, ignoring the setgid bit (g+s).\n+# This bit is inherited by subdirectories at their creation. So we remove it\n+# from the returning string to prevent callers from having to worry about the\n+# state of the bit in the test directory.\n+#\n test_modebits () {\n-\tls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n+\tls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|' \\\n+\t\t\t  -e 's|^\\(......\\)S|\\1-|' -e 's|^\\(......\\)s|\\1x|'\n }\n \n # Unset a configuration variable, but don't fail if it doesn't exist.\n-- \n2.29.2\n\n"},{"id":"413628","messageId":"xmqqlfd5obvx.fsf@gitster.c.googlers.com","threadId":"54905","inReplyTo":"b734425e3235651e738e6eac47eae0db7db92e7e.1609861567.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2] t4129: don't fail if setgid is set in the test directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T23:59:14Z","receivedAt":"2021-01-07T00:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares <matheus.bernardino@usp.br> writes:\n\n> +# Get the modebits from a file or directory, ignoring the setgid bit (g+s).\n> +# This bit is inherited by subdirectories at their creation. So we remove it\n> +# from the returning string to prevent callers from having to worry about the\n> +# state of the bit in the test directory.\n> +#\n\nWe probably do not use \"chmod g+s\" manually on regular files, so I\nmay be being overly \"correct\", but shouldn't these be done only for\ndirectories?\n\n>  test_modebits () {\n> -\tls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n> +\tls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|' \\\n> +\t\t\t  -e 's|^\\(......\\)S|\\1-|' -e 's|^\\(......\\)s|\\1x|'\n\nThat is, \n\n\t\t\t  -e 's|^\\(d.....\\)S|\\1-|' -e 's|^\\(d.....\\)s|\\1x|'\n\ninstead of applying the rule to any filetype.\n\nWill queue as-is, as the distinction probably would not matter in\npractice.\n\nThanks.\n"},{"id":"413913","messageId":"CAHd-oW4Yus5E5U0dykUBfwYM7dryz94YZW_9OFUSZNA-6pp3UA@mail.gmail.com","threadId":"54905","inReplyTo":"xmqqlfd5obvx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] t4129: don't fail if setgid is set in the test directory","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-01-09T14:19:20Z","receivedAt":"2021-01-09T14:20:15Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jan 6, 2021 at 8:59 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matheus Tavares <matheus.bernardino@usp.br> writes:\n>\n> > +# Get the modebits from a file or directory, ignoring the setgid bit (g+s).\n> > +# This bit is inherited by subdirectories at their creation. So we remove it\n> > +# from the returning string to prevent callers from having to worry about the\n> > +# state of the bit in the test directory.\n> > +#\n>\n> We probably do not use \"chmod g+s\" manually on regular files, so I\n> may be being overly \"correct\", but shouldn't these be done only for\n> directories?\n>\n> >  test_modebits () {\n> > -     ls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n> > +     ls -ld \"$1\" | sed -e 's|^\\(..........\\).*|\\1|' \\\n> > +                       -e 's|^\\(......\\)S|\\1-|' -e 's|^\\(......\\)s|\\1x|'\n>\n> That is,\n>\n>                           -e 's|^\\(d.....\\)S|\\1-|' -e 's|^\\(d.....\\)s|\\1x|'\n>\n> instead of applying the rule to any filetype.\n\nYeah, you're right. I ended up applying the rule on regular files as\nwell just for standardization. That is, if some day, for some reason,\na test script decides to use \"chmod g+s\" on a regular file and a\ndirectory, test_modebits would treat them equally to avoid any\nconfusion. But I guess it's very unlikely that we will ever need to\nset the setgid bit on a test, anyway...\n\n> Will queue as-is, as the distinction probably would not matter in\n> practice.\n>\n> Thanks.\n"}]}