git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	David Coppa <dcoppa@openbsd.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH] t0001: fix broken not-quite getcwd(3) test in bed67874e2
Date: Mon, 07 Jun 2021 13:24:23 +0200	[thread overview]
Message-ID: <871r9d6hhy.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <b650bef5-d739-d98d-e9f1-fa292b6ce982@web.de>


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@gmail.com>
>> ---
>>
>> 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 &&
>>


  parent reply	other threads:[~2021-06-07 11:28 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-06 23:38 [PATCH] tests: don't give unportable ">" to "test" built-in, use -gt Ævar Arnfjörð Bjarmason
2017-08-07  1:18 ` brian m. carlson
2017-08-07 10:17   ` René Scharfe
2017-08-07 10:59     ` Ævar Arnfjörð Bjarmason
2017-08-07 11:04     ` [PATCH] t0001: skip test with restrictive permissions if getpwd(3) respects them René Scharfe
2021-06-01  0:38       ` [PATCH] t0001: fix broken not-quite getcwd(3) test in bed67874e2 Ævar Arnfjörð Bjarmason
2021-06-01 16:15         ` René Scharfe
2021-06-01 21:20           ` Junio C Hamano
2021-06-07 11:29             ` Ævar Arnfjörð Bjarmason
2021-06-07 11:24           ` Ævar Arnfjörð Bjarmason [this message]
2021-06-07 15:26             ` René Scharfe
2021-06-07 15:38               ` Ævar Arnfjörð Bjarmason
2021-07-30 16:18         ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-07-30 17:16           ` Junio C Hamano
2017-08-07  1:33 ` [PATCH] tests: don't give unportable ">" to "test" built-in, use -gt Junio C Hamano
2017-08-07 17:32 ` Junio C Hamano
2017-08-07 18:06 ` Andreas Schwab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871r9d6hhy.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=dcoppa@openbsd.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).