[Bug] Temp file use in t0018.6

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:

<rsbecker@nexbridge.com> writes:

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...

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 <rsbecker@nexbridge.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0018-advice.sh | 1 -
 1 file changed, 1 deletion(-)

On Thursday, August 1, 2024 2:51 PM, Junio C Hamano wrote:

inspect it

cannot

be

b02448ea16..040a08be07

Verified to work correctly. Thanks.

On Thursday, August 1, 2024 2:35 PM, Junio C Hamano wrote:

add"

set to

and I

queuing it.  The

have to

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

On Thu, Aug 01, 2024 at 02:51:17PM -0400, rsbecker@nexbridge.com wrote:

This is OK; we're just doing string manipulation on the path.

Likewise, this test is just covering the config code. The actual contents of the variables are not used.

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.

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