# [Bug] Temp file use in t0018.6 6 messages from 2024-08-01 to 2024-08-02. Participants: rsbecker@nexbridge.com, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/61883 ## rsbecker@nexbridge.com, 2024-08-01 18:15 Subject: [Bug] Temp file use in t0018.6 Message-ID: <02d401dae43e$c076b000$41641000$@nexbridge.com> URL: https://gitlist.dev/e/02d401dae43e%24c076b000%2441641000%24%40nexbridge.com ``` In the 2.46.0 test suite on NonStop I'm getting the following surprise error: expecting success of 0018.6 'advice should be printed when GIT_ADVICE is set to true': q_to_tab >expect <<-\EOF && On branch trunk No commits yet Untracked files: (use "git add ..." to include in what will be committed) QREADME nothing added to commit but untracked files present (use "git add" to track) EOF test_when_finished "rm -fr advice-test" && git init advice-test && ( cd advice-test && >README && GIT_ADVICE=true git status ) >actual && cat actual > /tmp/actual && test_cmp expect actual Initialized empty Git repository in /home/randall/git-clar/t/trash directory.t0018-advice/advice-test/.git/ ./test-lib.sh: line 1071: /tmp/actual: Permission denied not ok 6 - advice should be printed when GIT_ADVICE is set to true This is the first I'm seeing a failure I'm seeing of this kind. We should be using randomized temp locations, not fixed. What is going on is that our main CI system runs under one user id while I was running another test under my own user. Our /tmp is configured so that users who create files have exclusive access to those files, regardless of security so /tmp/actual in this case can only be used in one run by one user. What we should be doing is probably generating /tmp/actual.XXXXXXX (randomized) or using actual in the trash directory as normal. In any event, t0018 should be cleaning up and removing /tmp/actual when done. I would suggest doing something like this as a start: diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index 29306b367c..1676a1a31d 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -97,7 +97,8 @@ test_expect_success 'advice should be printed when GIT_ADVICE is set to true' ' GIT_ADVICE=true git status ) >actual && cat actual > /tmp/actual && - test_cmp expect actual + test_cmp expect actual && + rm /tmp/actual ' And then, as a real fix: diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index 29306b367c..deba8a1d91 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -96,8 +96,10 @@ test_expect_success 'advice should be printed when GIT_ADVICE is set to true' ' >README && GIT_ADVICE=true git status ) >actual && - cat actual > /tmp/actual && - test_cmp expect actual + ACTUAL=/tmp/actual.$RANDOM && + cat actual > $ACTUAL && + test_cmp expect $ACTUAL && + rm $ACTUAL ' Regards, Randall ``` ## Junio C Hamano, 2024-08-01 18:34 Subject: Re: [Bug] Temp file use in t0018.6 Message-ID: URL: https://gitlist.dev/e/xmqqbk2cum5o.fsf%40gitster.g In-Reply-To: <02d401dae43e$c076b000$41641000$@nexbridge.com> ``` writes: > In the 2.46.0 test suite on NonStop I'm getting the following surprise > error: > > expecting success of 0018.6 'advice should be printed when GIT_ADVICE is set > to true': > q_to_tab >expect <<-\EOF && > On branch trunk > > No commits yet > > Untracked files: > (use "git add ..." to include in what will be committed) > QREADME > > nothing added to commit but untracked files present (use "git add" > to track) > EOF > > test_when_finished "rm -fr advice-test" && > git init advice-test && > ( > cd advice-test && > >README && > GIT_ADVICE=true git status > ) >actual && > cat actual > /tmp/actual && > test_cmp expect actual Sheesh. We should *not* be assuming what is in /tmp. Our TMPDIR may not even be set to point at /tmp. Anybody can create directory 'actual' there and break this test. I thought this was a left-over debugging copy while reviewing the patch, and I thought I had pointed it out to the author and/or I removed it while queuing it. The copy to /tmp/actual with cat should be removed. Thanks for noticing. Are there other reference to /tmp in our test suite I have to wonder... ``` ## Junio C Hamano, 2024-08-01 18:51 Subject: [PATCH] t0018: remove leftover debugging cruft Message-ID: URL: https://gitlist.dev/e/xmqq7cd0ulen.fsf_-_%40gitster.g In-Reply-To: ``` The actual file is copied out to /tmp, presumably so that the tester can inspect it after the test is done, which may have been a useful debugging aid. But in the final shape of the test suite, such a code should not exist. We cannot even assume that we are allowed to write into /tmp (our TMPDIR may not even be pointing at it) or read from it for that matter. Noticed-by: Randall S. Becker Signed-off-by: Junio C Hamano --- t/t0018-advice.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index b02448ea16..040a08be07 100755 --- a/t/t0018-advice.sh +++ b/t/t0018-advice.sh @@ -93,7 +93,6 @@ EOF >README && GIT_ADVICE=true git status ) >actual && - cat actual > /tmp/actual && test_cmp expect actual ' -- 2.46.0-179-g92bab5172e ``` ## rsbecker@nexbridge.com, 2024-08-01 18:51 Subject: RE: [Bug] Temp file use in t0018.6 Message-ID: <02e101dae443$d01575a0$704060e0$@nexbridge.com> URL: https://gitlist.dev/e/02e101dae443%24d01575a0%24704060e0%24%40nexbridge.com In-Reply-To: ``` On Thursday, August 1, 2024 2:35 PM, Junio C Hamano wrote: > writes: > >> In the 2.46.0 test suite on NonStop I'm getting the following surprise >> error: >> >> expecting success of 0018.6 'advice should be printed when GIT_ADVICE >> is set to true': >> q_to_tab >expect <<-\EOF && >> On branch trunk >> >> No commits yet >> >> Untracked files: >> (use "git add ..." to include in what will be committed) >> QREADME >> >> nothing added to commit but untracked files present (use "git add" >> to track) >> EOF >> >> test_when_finished "rm -fr advice-test" && >> git init advice-test && >> ( >> cd advice-test && >> >README && >> GIT_ADVICE=true git status >> ) >actual && >> cat actual > /tmp/actual && >> test_cmp expect actual > >Sheesh. > >We should *not* be assuming what is in /tmp. Our TMPDIR may not even be set to >point at /tmp. Anybody can create directory 'actual' >there and break this test. > >I thought this was a left-over debugging copy while reviewing the patch, and I >thought I had pointed it out to the author and/or I removed it while queuing it. The >copy to /tmp/actual with cat should be removed. > >Thanks for noticing. Are there other reference to /tmp in our test suite I have to >wonder... Other than t0018... * t0060 references /tmp but only for a synthetic repo path * t1300 extensively uses /tmp with hard-coded file names for cookies. * t7400 appears to work with submodules in /tmp but that may only be a reference * t9902 hard-codes a reference to the user home directory ~/tmp, which might be fine but prevents parallel tests The clar infrastructure assumes tests are done in /tmp (in find_tmp_path) except for Windows, so that should be resolved also. Regards, Randall ``` ## rsbecker@nexbridge.com, 2024-08-01 18:53 Subject: RE: [PATCH] t0018: remove leftover debugging cruft Message-ID: <02e401dae444$23172780$69457680$@nexbridge.com> URL: https://gitlist.dev/e/02e401dae444%2423172780%2469457680%24%40nexbridge.com In-Reply-To: ``` On Thursday, August 1, 2024 2:51 PM, Junio C Hamano wrote: >The actual file is copied out to /tmp, presumably so that the tester can inspect it >after the test is done, which may have been a useful debugging aid. > >But in the final shape of the test suite, such a code should not exist. We cannot >even assume that we are allowed to write into /tmp (our TMPDIR may not even be >pointing at it) or read from it for that matter. > >Noticed-by: Randall S. Becker >Signed-off-by: Junio C Hamano >--- > t/t0018-advice.sh | 1 - > 1 file changed, 1 deletion(-) > >diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index b02448ea16..040a08be07 >100755 >--- a/t/t0018-advice.sh >+++ b/t/t0018-advice.sh >@@ -93,7 +93,6 @@ EOF > >README && > GIT_ADVICE=true git status > ) >actual && >- cat actual > /tmp/actual && > test_cmp expect actual > ' > >-- >2.46.0-179-g92bab5172e Verified to work correctly. Thanks. ``` ## Jeff King, 2024-08-02 03:39 Subject: Re: [Bug] Temp file use in t0018.6 Message-ID: <20240802033829.GA1246312@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240802033829.GA1246312%40coredump.intra.peff.net In-Reply-To: <02e101dae443$d01575a0$704060e0$@nexbridge.com> ``` On Thu, Aug 01, 2024 at 02:51:17PM -0400, rsbecker@nexbridge.com wrote: > >Thanks for noticing. Are there other reference to /tmp in our test > >suite I have to wonder... > > Other than t0018... > * t0060 references /tmp but only for a synthetic repo path This is OK; we're just doing string manipulation on the path. > * t1300 extensively uses /tmp with hard-coded file names for cookies. Likewise, this test is just covering the config code. The actual contents of the variables are not used. > * t7400 appears to work with submodules in /tmp but that may only be a > reference This does do a "submodule init" with that path. But since we only care about the string handling of the relative URL, we never actually do a "submodule update", so we never look at the path. > * t9902 hard-codes a reference to the user home directory ~/tmp, which might > be fine > but prevents parallel tests Remember that in the test environment, $HOME is the trash directory. So this "~/tmp" is inside there, and parallel tests each get their own. I tried doing a "sudo chmod 755 /tmp" and running the test suite, to see if there were other cases. Lots of stuff fails, because processes want to make temporary files behind the scenes (git itself, but also sub-programs like gpg). Those cases are all OK; they do touch /tmp, but they should be getting their own randomized files. Setting TMPDIR=/some/actual/path on top of that yields only two failures: - t5541 fails on push certificate inspection over http. I suspect this is because we are running gpg on the server side, and apache does not pass our custom TMPDIR through the CGI environmentT. - t7528 seems to fail to invoke ssh-agent. Probably ssh-agent is unhappy with the unwritable /tmp. So I don't think there's anything else we need to fix on our end, though possibly some of the cosmetic uses of /tmp could make it more clear we do not expect the path to exist by using file:///some/repo.git, etc. -Peff ```