# [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY 7 messages from 2015-01-27 to 2015-01-28. Participants: Torsten Bögershausen, Chris Packham, Junio C Hamano. Thread: https://gitlist.dev/t/38461 ## Torsten Bögershausen, 2015-01-27 15:39 Subject: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: <54C7B115.7020405@web.de> URL: https://gitlist.dev/e/54C7B115.7020405%40web.de ``` The SANITY precondition was not set when running as root, but this is not 100% reliable for CYGWIN: A file may be allowed to be deleted when the containing directory does not have write permissions. See https://technet.microsoft.com/en-us/library/bb463216.aspx "...In UNIX, the write permission bit on a directory permits both the creation and removal of new files or sub-directories in the directory. On Windows, the Write_Data access right controls the creation of new sub-files and the Delete_Child access right controls the deletion. ...." We may argue that the translation of the POSIX write permission bit into "Windows access rights" can be improved in CYGWIN. A dynamic test handles all cases: when the sequence $ mkdir SANETESTD && $ chmod +w SANETESTD && $ >SANETESTD/x && $ ! rm SANETESTD/x succeeds the prerequisite SANITY is true. Helped-by: Junio C Hamano Signed-off-by: Torsten Bögershausen --- t/test-lib.sh | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 93f7cad..887e986 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT ' # When the tests are run as root, permission tests will report that # things are writable when they shouldn't be. +# Special check for CYGWIN (or Windows in general): +# A file can be deleted, even if the containing directory does'nt +# have write permissions test_lazy_prereq SANITY ' - test_have_prereq POSIXPERM,NOT_ROOT + mkdir SANETESTD && + chmod +w SANETESTD && + >SANETESTD/x && + chmod -w SANETESTD || + error "bug in test sript: cannot prepare SANETESTD" + + ! rm SANETESTD/x + status=$? + + chmod +w SANETESTD && + rm -rf SANETESTD || + error "bug in test sript: cannot clean SANETESTD" + return $status ' GIT_UNZIP=${GIT_UNZIP:-unzip} -- 2.2.0.rc1.26.g3e3a70d ``` ## Chris Packham, 2015-01-27 19:53 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: URL: https://gitlist.dev/e/CAFOYHZDcvBvF8rgyax_4xMc88sZJFTVX-fXwV1iZa8veTaFixQ%40mail.gmail.com In-Reply-To: <54C7B115.7020405@web.de> ``` Minor typo in a comment. On Wed, Jan 28, 2015 at 4:39 AM, Torsten Bögershausen wrote: > The SANITY precondition was not set when running as root, > but this is not 100% reliable for CYGWIN: > > A file may be allowed to be deleted when the containing > directory does not have write permissions. > > See > https://technet.microsoft.com/en-us/library/bb463216.aspx > "...In UNIX, the write permission bit on a directory permits both > the creation and removal of new files or sub-directories in the directory. > On Windows, the Write_Data access right controls the creation of new > sub-files and the Delete_Child access right controls the deletion. ...." > > We may argue that the translation of the POSIX write permission bit > into "Windows access rights" can be improved in CYGWIN. > > A dynamic test handles all cases: when the sequence > $ mkdir SANETESTD && > $ chmod +w SANETESTD && > $ >SANETESTD/x && > $ ! rm SANETESTD/x > succeeds the prerequisite SANITY is true. > > Helped-by: Junio C Hamano > Signed-off-by: Torsten Bögershausen > --- > t/test-lib.sh | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 93f7cad..887e986 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1038,8 +1038,23 @@ test_lazy_prereq NOT_ROOT ' > > # When the tests are run as root, permission tests will report that > # things are writable when they shouldn't be. > +# Special check for CYGWIN (or Windows in general): > +# A file can be deleted, even if the containing directory does'nt s/does'nt/doesn't/ > +# have write permissions > test_lazy_prereq SANITY ' > - test_have_prereq POSIXPERM,NOT_ROOT > + mkdir SANETESTD && > + chmod +w SANETESTD && > + >SANETESTD/x && > + chmod -w SANETESTD || > + error "bug in test sript: cannot prepare SANETESTD" > + > + ! rm SANETESTD/x > + status=$? > + > + chmod +w SANETESTD && > + rm -rf SANETESTD || > + error "bug in test sript: cannot clean SANETESTD" > + return $status > ' > > GIT_UNZIP=${GIT_UNZIP:-unzip} > -- > 2.2.0.rc1.26.g3e3a70d > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Junio C Hamano, 2015-01-27 22:20 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: URL: https://gitlist.dev/e/xmqqh9vbkgrg.fsf%40gitster.dls.corp.google.com In-Reply-To: <54C7B115.7020405@web.de> ``` Torsten Bögershausen writes: > # When the tests are run as root, permission tests will report that > # things are writable when they shouldn't be. This no longer is relevant, I think. > +# Special check for CYGWIN (or Windows in general): Misleading comment in the end result, as your new test drops SANITY correctly on POSIX for the root user, too. In a commit log message it is correct to say "This adds special check for Cygwin", but the resulting code is sensible with or without Cygwin, I would think, with the justification to "test by checking what we really want, not by inferring from the result of indirectly testing something else". > +# A file can be deleted, even if the containing directory does'nt > +# have write permissions We also rely on SANITY to make sure that "chmod -rx directory" makes "directory/file" undiscoverable. How about extending it like this (not tested)? -- >8 -- From: Torsten Bögershausen Date: Tue, 27 Jan 2015 16:39:01 +0100 Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need What we wanted out of the SANITY precondition is that the filesystem behaves sensibly with permission bits settings. - You should not be able to remove a file in a read-only directory, - You should not be able to tell if a file in a directory exists if the directory lacks read or execute permission bits. We used to cheat by approximating that condition with "is the / writable?" test and/or "are we running as root?" test. Neither test is sufficient or appropriate in more exotic environments like Cygwin. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- t/test-lib.sh | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index b2b2ec7..446d8d5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -997,9 +997,28 @@ test_lazy_prereq NOT_ROOT ' test "$uid" != 0 ' -# When the tests are run as root, permission tests will report that -# things are writable when they shouldn't be. -test -w / || test_set_prereq SANITY +# On a filesystem that lacks SANITY, a file can be deleted even if +# the containing directory doesn't have write permissions, or a file +# can be accessed even if the containing directory doesn't have read +# or execute permissions, causing our tests that validate that Git +# works sensibly in such situations. +test_lazy_prereq SANITY ' + mkdir SANETESTD.1 SANETESTD.2 && + + chmod +w SANETESTD.1 SANETESTD.2 && + >SANETESTD.1/x 2>SANETESTD.2/x && + chmod -w SANETESTD.1 && + chmod -rx SANETESTD.2 || + error "bug in test sript: cannot prepare SANETESTD" + + ! rm SANETESTD.1/x && ! test -f SANETESTD.2/x + status=$? + + chmod +rwx SANETESTD.1 SANETESTD.2 && + rm -rf SANETESTD.1 SANETESTD.2 || + error "bug in test sript: cannot clean SANETESTD" + return $status +' GIT_UNZIP=${GIT_UNZIP:-unzip} test_lazy_prereq UNZIP ' -- 2.3.0-rc1-180-g1a69fe5 ``` ## Torsten Bögershausen, 2015-01-28 08:28 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: <54C89DC8.1050805@web.de> URL: https://gitlist.dev/e/54C89DC8.1050805%40web.de In-Reply-To: ``` On 27.01.15 23:20, Junio C Hamano wrote: > How about extending it like this (not tested)? Thanks, this looks good: the test is more extensive, I can test this next week. > > -- >8 -- > From: Torsten Bögershausen > Date: Tue, 27 Jan 2015 16:39:01 +0100 > Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need > > What we wanted out of the SANITY precondition is that the filesystem > behaves sensibly with permission bits settings. > > - You should not be able to remove a file in a read-only directory, > > - You should not be able to tell if a file in a directory exists if > the directory lacks read or execute permission bits. > > We used to cheat by approximating that condition with "is the / > writable?" test and/or "are we running as root?" test. Neither test > is sufficient or appropriate in more exotic environments like > Cygwin. How about going this direction: We used to cheat by approximating that condition with "is the / writable?" test and/or "are we running as root?" test. Neither test is sufficient or appropriate, especially in environments like Cygwin, Mingw or Mac OS X. ``` ## Junio C Hamano, 2015-01-28 17:38 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: URL: https://gitlist.dev/e/CAPc5daVAGOnu3pe_r%3DGuDXp1kNkJgbCqP-qD38kXmdEJa3ZSFQ%40mail.gmail.com In-Reply-To: <54C89DC8.1050805@web.de> ``` On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen wrote: > On 27.01.15 23:20, Junio C Hamano wrote: > >> How about extending it like this (not tested)? > Thanks, this looks good: the test is more extensive, > I can test this next week. > >> >> -- >8 -- >> From: Torsten Bögershausen >> Date: Tue, 27 Jan 2015 16:39:01 +0100 >> Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need >> >> What we wanted out of the SANITY precondition is that the filesystem >> behaves sensibly with permission bits settings. >> >> - You should not be able to remove a file in a read-only directory, >> >> - You should not be able to tell if a file in a directory exists if >> the directory lacks read or execute permission bits. Forgot one thing. I do not offhand know if tests that needs SANITY depends on this, but we may also want to check for this: - You should not be able to write to a file that is marked as read-only. by adding something like >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && ! test -s sanitytest" in the mix. >> >> We used to cheat by approximating that condition with "is the / >> writable?" test and/or "are we running as root?" test. Neither test >> is sufficient or appropriate in more exotic environments like >> Cygwin. > How about going this direction: > > We used to cheat by approximating that condition with "is the / > writable?" test and/or "are we running as root?" test. Neither test > is sufficient or appropriate, especially in environments like > Cygwin, Mingw or Mac OS X. OK, but MacOS X does not have SANITY problem; "is the / writable?" test was misdetecting and declaring a system with SANITY does not have one. Perhaps roll Cygwin and Mingw into a single Windows category? I dunno. ``` ## Torsten Bögershausen, 2015-01-28 19:19 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: <54C9362E.2000705@web.de> URL: https://gitlist.dev/e/54C9362E.2000705%40web.de In-Reply-To: ``` On 28.01.15 18:38, Junio C Hamano wrote: > On Wed, Jan 28, 2015 at 12:28 AM, Torsten Bögershausen wrote: >> On 27.01.15 23:20, Junio C Hamano wrote: >> >>> How about extending it like this (not tested)? >> Thanks, this looks good: the test is more extensive, >> I can test this next week. >> >>> -- >8 -- >>> From: Torsten Bögershausen >>> Date: Tue, 27 Jan 2015 16:39:01 +0100 >>> Subject: [PATCH] test-lib.sh: set prerequisite SANITY by testing what we really need >>> >>> What we wanted out of the SANITY precondition is that the filesystem >>> behaves sensibly with permission bits settings. >>> >>> - You should not be able to remove a file in a read-only directory, >>> >>> - You should not be able to tell if a file in a directory exists if >>> the directory lacks read or execute permission bits. > Forgot one thing. I do not offhand know if tests that needs SANITY > depends on this, but we may also want to check for this: > > - You should not be able to write to a file that is marked as read-only. > > by adding something like > > >sanitytest && chmod -w sanitytest && ! echo boo >sanitytest && ! > test -s sanitytest" > > in the mix. > >>> We used to cheat by approximating that condition with "is the / >>> writable?" test and/or "are we running as root?" test. Neither test >>> is sufficient or appropriate in more exotic environments like >>> Cygwin. >> How about going this direction: >> >> We used to cheat by approximating that condition with "is the / >> writable?" test and/or "are we running as root?" test. Neither test >> is sufficient or appropriate, especially in environments like >> Cygwin, Mingw or Mac OS X. > OK, but MacOS X does not have SANITY problem; "is the / writable?" test > was misdetecting and declaring a system with SANITY does not have one. > > Perhaps roll Cygwin and Mingw into a single Windows category? I dunno. The whole discussion actually started with Mac OS X, and the conclusion was that Mac OS X should have SANITY set, but hadn't, because / is writable (if you install from scratch): $gmane/262389 and especially: $gmane/262456 The whole discussion ended up a fix for t5539, and, as a different improvement, the lazy SANITY probing - which works for me on all systems I had the chance to test it. The code is OK (we can add more tests, as you suggested). The only problem I can see is to put everything into a good commit-msg. ``` ## Junio C Hamano, 2015-01-28 20:38 Subject: Re: [PATCH 2/2] test-lib.sh: Dynamic test for the prerequisite SANITY Message-ID: URL: https://gitlist.dev/e/xmqq7fw6hc83.fsf%40gitster.dls.corp.google.com In-Reply-To: <54C9362E.2000705@web.de> ``` Torsten Bögershausen writes: >> OK, but MacOS X does not have SANITY problem; "is the / writable?" test >> was misdetecting and declaring a system with SANITY does not have one. >> >> Perhaps roll Cygwin and Mingw into a single Windows category? I dunno. > The whole discussion actually started with Mac OS X, > and the conclusion was that Mac OS X should have SANITY set, but hadn't, > because / is writable (if you install from scratch): Exactly. On MacOSX running as non-root-but-can-write-to-slash-user, we should say SANITY is satisfied, but "is the / writable?" check was misdetecting and declaring the system does not have SANITY. So we are in agreement, no? ```