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".
> 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.
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.
Why not just fix the bug in there being disconnect between pwd and
getcwd() here and move on?
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> ---
>>
>> Not an issue new in v2.32.0, but an easy enough fix, so I thought I'd
>> send it now anyway in case we'd like these various AIX fixes in one
>> batch...
>>
>> Makefile | 1 +
>> t/helper/test-getcwd.c | 26 ++++++++++++++++++++++++++
>> t/helper/test-tool.c | 1 +
>> t/helper/test-tool.h | 1 +
>> t/t0001-init.sh | 5 ++++-
>> 5 files changed, 33 insertions(+), 1 deletion(-)
>> create mode 100644 t/helper/test-getcwd.c
>>
>> diff --git a/Makefile b/Makefile
>> index c3565fc0f8..8c000ba48b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -711,6 +711,7 @@ TEST_BUILTINS_OBJS += test-example-decorate.o
>> TEST_BUILTINS_OBJS += test-fast-rebase.o
>> TEST_BUILTINS_OBJS += test-genrandom.o
>> TEST_BUILTINS_OBJS += test-genzeros.o
>> +TEST_BUILTINS_OBJS += test-getcwd.o
>> TEST_BUILTINS_OBJS += test-hash-speed.o
>> TEST_BUILTINS_OBJS += test-hash.o
>> TEST_BUILTINS_OBJS += test-hashmap.o
>> diff --git a/t/helper/test-getcwd.c b/t/helper/test-getcwd.c
>> new file mode 100644
>> index 0000000000..d680038a78
>> --- /dev/null
>> +++ b/t/helper/test-getcwd.c
>> @@ -0,0 +1,26 @@
>> +#include "test-tool.h"
>> +#include "git-compat-util.h"
>> +#include "parse-options.h"
>> +
>> +static const char *getcwd_usage[] = {
>> + "test-tool getcwd",
>> + NULL
>> +};
>> +
>> +int cmd__getcwd(int argc, const char **argv)
>> +{
>> + struct option options[] = {
>> + OPT_END()
>> + };
>> + char *cwd;
>> +
>> + argc = parse_options(argc, argv, "test-tools", options, getcwd_usage, 0);
>> + if (argc > 0)
>> + usage_with_options(getcwd_usage, options);
>> +
>> + cwd = xgetcwd();
>> + puts(cwd);
>> + free(cwd);
>> +
>> + return 0;
>> +}
>> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
>> index c5bd0c6d4c..3c13cb19b5 100644
>> --- a/t/helper/test-tool.c
>> +++ b/t/helper/test-tool.c
>> @@ -33,6 +33,7 @@ static struct test_cmd cmds[] = {
>> { "fast-rebase", cmd__fast_rebase },
>> { "genrandom", cmd__genrandom },
>> { "genzeros", cmd__genzeros },
>> + { "getcwd", cmd__getcwd },
>> { "hashmap", cmd__hashmap },
>> { "hash-speed", cmd__hash_speed },
>> { "index-version", cmd__index_version },
>> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
>> index e8069a3b22..e691a4d9e9 100644
>> --- a/t/helper/test-tool.h
>> +++ b/t/helper/test-tool.h
>> @@ -23,6 +23,7 @@ int cmd__example_decorate(int argc, const char **argv);
>> int cmd__fast_rebase(int argc, const char **argv);
>> int cmd__genrandom(int argc, const char **argv);
>> int cmd__genzeros(int argc, const char **argv);
>> +int cmd__getcwd(int argc, const char **argv);
>> int cmd__hashmap(int argc, const char **argv);
>> int cmd__hash_speed(int argc, const char **argv);
>> int cmd__index_version(int argc, const char **argv);
>> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
>> index acd662e403..df544bb321 100755
>> --- a/t/t0001-init.sh
>> +++ b/t/t0001-init.sh
>> @@ -356,7 +356,10 @@ test_lazy_prereq GETCWD_IGNORES_PERMS '
>> chmod 100 $base ||
>> BUG "cannot prepare $base"
>>
>> - (cd $base/dir && /bin/pwd -P)
>> + (
>> + cd $base/dir &&
>> + test-tool getcwd
>> + )
>> status=$?
>>
>> chmod 700 $base &&
>>
