Re: [PATCH] t0001: fix broken not-quite getcwd(3) test in bed67874e2

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 7, 2021, 15:38 UTC
Message-ID: <87pmwx4r94.fsf@evledraar.gmail.com>
In-Reply-To: <57c2f4dc-06b9-b330-a01c-d2ae0e86df7c@web.de>

On Mon, Jun 07 2021, René Scharfe wrote:

Show 53 quoted lines

> Am 07.06.21 um 13:24 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Tue, Jun 01 2021, René Scharfe wrote:
>>
>>> Am 01.06.21 um 02:38 schrieb Ævar Arnfjörð Bjarmason:
>>>> With a54e938e5b (strbuf: support long paths w/o read rights in
>>>> strbuf_getcwd() on FreeBSD, 2017-03-26) we had t0001 break on systems
>>>> like OpenBSD and AIX whose getcwd(3) has standard (but not like glibc
>>>> et al) behavior.
>>>>
>>>> This was partially fixed in bed67874e2 (t0001: skip test with
>>>> restrictive permissions if getpwd(3) respects them, 2017-08-07).
>>>>
>>>> The problem with that fix is that while its analysis of the problem is
>>>> correct, it doesn't actually call getcwd(3), instead it invokes "pwd
>>>> -P". There is no guarantee that "pwd -P" is actually going to call
>>>> getcwd(3), as opposed to e.g. being a shell built-in.
>>>>
>>>> On AIX under both bash and ksh this test breaks because "pwd -P" will
>>>> happily display the current working directory, but getcwd(3) called by
>>>> the "git init" we're testing here will fail to get it.
>>>>
>>>> I checked whether clobbering the $PWD environment variable would
>>>> affect it, and it didn't. Presumably these shells keep track of their
>>>> working directory internally.
>>>>
>>>> Let's change the test to a new "test-tool getcwd".
>>>
>>> Makes sense.
>>>
>>> If /bin/pwd can figure out the path to the current working directory
>>> without read permissions to parent directories then it might be possible
>>> to teach strbuf_getcwd() the same trick, though.  How does it do it?
>>>
>>> Perhaps it falls back to $PWD; POSIX says the behavior of pwd is
>>> unspecified if that variable would be changed, so a compliant
>>> implementation would be allowed to do that.  I think that way is not
>>> interesting for strbuf_getcwd(), though, because if we trust that
>>> variable then we can read it directly instead.  It gets stale if any
>>> parent directory is renamed.  E.g. the following commands would print a
>>> string ending in "stale":
>>>
>>> 	mkdir stale
>>> 	cd stale
>>> 	mv ../stale ../fresh
>>> 	chmod 111 ../fresh
>>> 	/bin/pwd -P
>>
>> Yes, AIX prints "stale" here, but e.g. my Linux box prints "fresh".
>
> OK, thanks for checking.  I find it weird: Why would they add a command
> that basically prints $PWD when callers can easily access this variable
> directly?  Anyway, it is what it is.

Not so much them but POSIX sayeth:

    If an application sets or unsets the value of PWD , the behavior of
    pwd is unspecified.
    -- https://pubs.opengroup.org/onlinepubs/007904875/utilities/pwd.html

And you can't be POSIX-compatible without a pwd(1) command. Ergo a system like AIX needs a pwd utility, whether it'll return the same thing as "$PWD" in some scenarios or not.

By they way: I don't know how AIX implements pwd(1), and whether it's purely redundant or whatever in this case.

Show 36 quoted lines

>>> Perhaps it asks the kernel, like getcwd() does on FreeBSD.  It would
>>> be a bit weird to expose this functionality in a command line tool, but
>>> not in the library function, so this is unlikely.  You seem to say that
>>> /bin/pwd is a shell builtin on your system, which is also weird, though.
>>> The commands above would print a string ending in "fresh" with the
>>> syscall method.
>>>
>>> An evil way would be to temporarily add read permission to all parent
>>> directories.  It would also print a string ending in "fresh".  You'd
>>> probably see chmod calls when running /bin/pwd using truss in that
>>> case, and it would fail if chmod is not allowed.
>>>
>>> That's all I can think of.
>>>
>>> If strbuf_getcwd() were to learn any of these tricks, then so would
>>> "test-tool getcwd", via its xgetcwd() call.  At that point we'd better
>>> rename GETCWD_IGNORES_PERMS to XGETCWD_IGNORES_PERMS.
>>>
>>> But I guess we need none of that because we never got a request from
>>> an AIX user to support a /home directory without read permissions,
>>> right?
>>
>> I don't really see the point of trying that hard. Yes, we could make
>> some forward progress if we bent over backwards and got the current
>> working directory, but what would we be left with? A git repository the
>> user can't "ls" inside of.
>
> The reason would be support for execute-only (e.g. 0711) /home, which
> some systems have for privacy reasons.
>
>> So any number of other thing after that now-working xgetcwd() call would
>> fail, we couldn't list any files in the working tree or .git directory.
>
> Users own their /home/directory in that scenario and have full
> permissions in their repositories.  They cannot verify the name of their
> /home/directory using readdir(), though.

Ah, I think I'd earlier misunderstood the test-case and thought it was merely about a "git init xyz" where xyz itself is in that state..

Show 6 quoted lines

>> Why not just fix the bug in there being disconnect between pwd and
>> getcwd() here and move on?
>
> If you mean the false assumption that /bin/pwd uses getcwd(): I still
> think that your patch makes sense, as I wrote in my first reply,
> implying that I agree it should be applied.

Ah, thanks. I wasn't quite clear or that, i.e. whether the strbuf_getcwd() was a suggestion of some follow-up work or that this fix should take that alternate route. Thanks!