[Bug] Temp file use in t0018.6

Commit Message

Randall S. Becker Aug. 1, 2024, 6:15 p.m. UTC

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 <file>..." 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:

Regards,
Randall

Comments

Junio C Hamano Aug. 1, 2024, 6:34 p.m. UTC | #1

<[e-mail]> 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 <file>..." 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...

Randall S. Becker Aug. 1, 2024, 6:51 p.m. UTC | #2

On Thursday, August 1, 2024 2:35 PM, Junio C Hamano wrote:
><[e-mail]> 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 <file>..." 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

Jeff King Aug. 2, 2024, 3:39 a.m. UTC | #3

On Thu, Aug 01, 2024 at 02:51:17PM -0400, [e-mail] 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

Patch

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
 '