{"thread":{"id":"61883","subject":"[Bug] Temp file use in t0018.6","startedAt":"2024-08-01T18:15:14Z","lastAt":"2024-08-02T03:39:18Z","messageCount":6,"participants":["rsbecker@nexbridge.com","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"499885","messageId":"02d401dae43e$c076b000$41641000$@nexbridge.com","threadId":"61883","inReplyTo":null,"subject":"[Bug] Temp file use in t0018.6","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-08-01T18:15:07Z","receivedAt":"2024-08-01T18:15:14Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"In the 2.46.0 test suite on NonStop I'm getting the following surprise\nerror:\n\nexpecting success of 0018.6 'advice should be printed when GIT_ADVICE is set\nto true':\n q_to_tab >expect <<-\\EOF &&\n On branch trunk\n\n No commits yet\n\n Untracked files:\n (use \"git add ...\" to include in what will be committed)\n QREADME\n\n nothing added to commit but untracked files present (use \"git add\"\nto track)\n EOF\n\n test_when_finished \"rm -fr advice-test\" &&\n git init advice-test &&\n (\n cd advice-test &&\n >README &&\n GIT_ADVICE=true git status\n ) >actual &&\n cat actual > /tmp/actual &&\n test_cmp expect actual\n\nInitialized empty Git repository in /home/randall/git-clar/t/trash\ndirectory.t0018-advice/advice-test/.git/\n./test-lib.sh: line 1071: /tmp/actual: Permission denied\nnot ok 6 - advice should be printed when GIT_ADVICE is set to true\n\nThis is the first I'm seeing a failure I'm seeing of this kind. We should be\nusing randomized temp\nlocations, not fixed. What is going on is that our main CI system runs under\none user id while I was\nrunning another test under my own user. Our /tmp is configured so that users\nwho create files have\nexclusive access to those files, regardless of security so /tmp/actual in\nthis case can only be used in\none run by one user. What we should be doing is probably generating\n/tmp/actual.XXXXXXX\n(randomized) or using actual in the trash directory as normal. In any event,\nt0018 should be\ncleaning up and removing /tmp/actual when done. I would suggest doing\nsomething like this as\na start:\n\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 29306b367c..1676a1a31d 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -97,7 +97,8 @@ test_expect_success 'advice should be printed when\nGIT_ADVICE is set to true' '\n GIT_ADVICE=true git status\n ) >actual &&\n cat actual > /tmp/actual &&\n- test_cmp expect actual\n+ test_cmp expect actual &&\n+ rm /tmp/actual\n '\n\nAnd then, as a real fix:\n\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 29306b367c..deba8a1d91 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -96,8 +96,10 @@ test_expect_success 'advice should be printed when\nGIT_ADVICE is set to true' '\n >README &&\n GIT_ADVICE=true git status\n ) >actual &&\n- cat actual > /tmp/actual &&\n- test_cmp expect actual\n+ ACTUAL=/tmp/actual.$RANDOM &&\n+ cat actual > $ACTUAL &&\n+ test_cmp expect $ACTUAL &&\n+ rm $ACTUAL\n '\n\nRegards,\nRandall\n\n\n\n"},{"id":"499887","messageId":"xmqqbk2cum5o.fsf@gitster.g","threadId":"61883","inReplyTo":"02d401dae43e$c076b000$41641000$@nexbridge.com","subject":"Re: [Bug] Temp file use in t0018.6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T18:34:59Z","receivedAt":"2024-08-01T18:35:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> In the 2.46.0 test suite on NonStop I'm getting the following surprise\n> error:\n>\n> expecting success of 0018.6 'advice should be printed when GIT_ADVICE is set\n> to true':\n> q_to_tab >expect <<-\\EOF &&\n> On branch trunk\n>\n> No commits yet\n>\n> Untracked files:\n> (use \"git add ...\" to include in what will be committed)\n> QREADME\n>\n> nothing added to commit but untracked files present (use \"git add\"\n> to track)\n> EOF\n>\n> test_when_finished \"rm -fr advice-test\" &&\n> git init advice-test &&\n> (\n> cd advice-test &&\n> >README &&\n> GIT_ADVICE=true git status\n> ) >actual &&\n> cat actual > /tmp/actual &&\n> test_cmp expect actual\n\nSheesh.\n\nWe should *not* be assuming what is in /tmp. Our TMPDIR may not\neven be set to point at /tmp. Anybody can create directory 'actual'\nthere and break this test.\n\nI thought this was a left-over debugging copy while reviewing the\npatch, and I thought I had pointed it out to the author and/or I\nremoved it while queuing it. The copy to /tmp/actual with cat\nshould be removed.\n\nThanks for noticing. Are there other reference to /tmp in our test\nsuite I have to wonder...\n\n"},{"id":"499888","messageId":"xmqq7cd0ulen.fsf_-_@gitster.g","threadId":"61883","inReplyTo":"xmqqbk2cum5o.fsf@gitster.g","subject":"[PATCH] t0018: remove leftover debugging cruft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T18:51:12Z","receivedAt":"2024-08-01T18:51:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The actual file is copied out to /tmp, presumably so that the tester\ncan inspect it after the test is done, which may have been a useful\ndebugging aid.\n\nBut in the final shape of the test suite, such a code should not\nexist. We cannot even assume that we are allowed to write into /tmp\n(our TMPDIR may not even be pointing at it) or read from it for that\nmatter.\n\nNoticed-by: Randall S. Becker \nSigned-off-by: Junio C Hamano \n---\n t/t0018-advice.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex b02448ea16..040a08be07 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -93,7 +93,6 @@ EOF\n \t\t>README &&\n \t\tGIT_ADVICE=true git status\n \t) >actual &&\n-\tcat actual > /tmp/actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.46.0-179-g92bab5172e\n\n"},{"id":"499889","messageId":"02e101dae443$d01575a0$704060e0$@nexbridge.com","threadId":"61883","inReplyTo":"xmqqbk2cum5o.fsf@gitster.g","subject":"RE: [Bug] Temp file use in t0018.6","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-08-01T18:51:17Z","receivedAt":"2024-08-01T18:51:28Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Thursday, August 1, 2024 2:35 PM, Junio C Hamano wrote:\n> writes:\n>\n>> In the 2.46.0 test suite on NonStop I'm getting the following surprise\n>> error:\n>>\n>> expecting success of 0018.6 'advice should be printed when GIT_ADVICE\n>> is set to true':\n>> q_to_tab >expect <<-\\EOF &&\n>> On branch trunk\n>>\n>> No commits yet\n>>\n>> Untracked files:\n>> (use \"git add ...\" to include in what will be committed)\n>> QREADME\n>>\n>> nothing added to commit but untracked files present (use \"git\nadd\"\n>> to track)\n>> EOF\n>>\n>> test_when_finished \"rm -fr advice-test\" &&\n>> git init advice-test &&\n>> (\n>> cd advice-test &&\n>> >README &&\n>> GIT_ADVICE=true git status\n>> ) >actual &&\n>> cat actual > /tmp/actual &&\n>> test_cmp expect actual\n>\n>Sheesh.\n>\n>We should *not* be assuming what is in /tmp. Our TMPDIR may not even be\nset to\n>point at /tmp. Anybody can create directory 'actual'\n>there and break this test.\n>\n>I thought this was a left-over debugging copy while reviewing the patch,\nand I\n>thought I had pointed it out to the author and/or I removed it while\nqueuing it. The\n>copy to /tmp/actual with cat should be removed.\n>\n>Thanks for noticing. Are there other reference to /tmp in our test suite I\nhave to\n>wonder...\n\nOther than t0018...\n* t0060 references /tmp but only for a synthetic repo path\n* t1300 extensively uses /tmp with hard-coded file names for cookies.\n* t7400 appears to work with submodules in /tmp but that may only be a\nreference\n* t9902 hard-codes a reference to the user home directory ~/tmp, which might\nbe fine\n but prevents parallel tests\nThe clar infrastructure assumes tests are done in /tmp (in find_tmp_path)\nexcept\n for Windows, so that should be resolved also.\n\nRegards,\nRandall\n\n"},{"id":"499891","messageId":"02e401dae444$23172780$69457680$@nexbridge.com","threadId":"61883","inReplyTo":"xmqq7cd0ulen.fsf_-_@gitster.g","subject":"RE: [PATCH] t0018: remove leftover debugging cruft","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-08-01T18:53:40Z","receivedAt":"2024-08-01T18:53:47Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Thursday, August 1, 2024 2:51 PM, Junio C Hamano wrote:\n>The actual file is copied out to /tmp, presumably so that the tester can\ninspect it\n>after the test is done, which may have been a useful debugging aid.\n>\n>But in the final shape of the test suite, such a code should not exist. We\ncannot\n>even assume that we are allowed to write into /tmp (our TMPDIR may not even\nbe\n>pointing at it) or read from it for that matter.\n>\n>Noticed-by: Randall S. Becker \n>Signed-off-by: Junio C Hamano \n>---\n> t/t0018-advice.sh | 1 -\n> 1 file changed, 1 deletion(-)\n>\n>diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh index\nb02448ea16..040a08be07\n>100755\n>--- a/t/t0018-advice.sh\n>+++ b/t/t0018-advice.sh\n>@@ -93,7 +93,6 @@ EOF\n> \t\t>README &&\n> \t\tGIT_ADVICE=true git status\n> \t) >actual &&\n>-\tcat actual > /tmp/actual &&\n> \ttest_cmp expect actual\n> '\n>\n>--\n>2.46.0-179-g92bab5172e\n\nVerified to work correctly.\nThanks.\n\n"},{"id":"499923","messageId":"20240802033829.GA1246312@coredump.intra.peff.net","threadId":"61883","inReplyTo":"02e101dae443$d01575a0$704060e0$@nexbridge.com","subject":"Re: [Bug] Temp file use in t0018.6","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-02T03:39:16Z","receivedAt":"2024-08-02T03:39:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2024 at 02:51:17PM -0400, rsbecker@nexbridge.com wrote:\n\n> >Thanks for noticing. Are there other reference to /tmp in our test\n> >suite I have to wonder...\n> \n> Other than t0018...\n> * t0060 references /tmp but only for a synthetic repo path\n\nThis is OK; we're just doing string manipulation on the path.\n\n> * t1300 extensively uses /tmp with hard-coded file names for cookies.\n\nLikewise, this test is just covering the config code. The actual\ncontents of the variables are not used.\n\n> * t7400 appears to work with submodules in /tmp but that may only be a\n> reference\n\nThis does do a \"submodule init\" with that path. But since we only care\nabout the string handling of the relative URL, we never actually do a\n\"submodule update\", so we never look at the path.\n\n> * t9902 hard-codes a reference to the user home directory ~/tmp, which might\n> be fine\n> but prevents parallel tests\n\nRemember that in the test environment, $HOME is the trash directory. So\nthis \"~/tmp\" is inside there, and parallel tests each get their own.\n\nI tried doing a \"sudo chmod 755 /tmp\" and running the test suite, to see\nif there were other cases. Lots of stuff fails, because processes want\nto make temporary files behind the scenes (git itself, but also\nsub-programs like gpg). Those cases are all OK; they do touch /tmp, but\nthey should be getting their own randomized files.\n\nSetting TMPDIR=/some/actual/path on top of that yields only two\nfailures:\n\n - t5541 fails on push certificate inspection over http. I suspect this\n is because we are running gpg on the server side, and apache does\n not pass our custom TMPDIR through the CGI environmentT.\n\n - t7528 seems to fail to invoke ssh-agent. Probably ssh-agent is\n unhappy with the unwritable /tmp.\n\nSo I don't think there's anything else we need to fix on our end, though\npossibly some of the cosmetic uses of /tmp could make it more clear we\ndo not expect the path to exist by using file:///some/repo.git, etc.\n\n-Peff\n"}]}