[PATCH v2 1/3] read-cache: use shared perms when writing shared index

[PATCH v2 1/3] read-cache: use shared perms when writing shared index

[Christian Couder]

Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
write_shared_index() has been using mks_tempfile() to create the
temporary file that will become the shared index.

But even before that, it looks like the functions used to create this
file didn't call adjust_shared_perm(), which means that the shared
index file has always been created with 600 permissions regardless
of the shared permission settings.

Because of that, on repositories created with `git init --shared=all`
and using the split index feature, one gets an error like:

fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied

when another user performs any operation that reads the shared index.

We could use create_tempfile() that calls adjust_shared_perm(), but
unfortunately create_tempfile() doesn't replace the XXXXXX at the end
of the path it is passed. So let's just call adjust_shared_perm() by
ourselves.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index bc156a133e..66f85f8d58 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state *istate,
 		delete_tempfile(&temporary_sharedindex);
 		return ret;
 	}
+	ret = adjust_shared_perm(temporary_sharedindex.filename.buf);
+	if (ret) {
+		int save_errno = errno;
+		error("cannot fix permission bits on %s", temporary_sharedindex.filename.buf);
+		delete_tempfile(&temporary_sharedindex);
+		errno = save_errno;
+		return ret;
+	}
 	ret = rename_tempfile(&temporary_sharedindex,
 			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
 	if (!ret) {
-- 
2.13.1.519.g0a0746bea4

[PATCH v2 2/3] t1301: move modebits() to test-lib-functions.sh

[Christian Couder]

As the modebits() function can be useful outside t1301,
let's move it into test-lib-functions.sh, and while at
it let's rename it test_modebits().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1301-shared-repo.sh  | 18 +++++++-----------
 t/test-lib-functions.sh |  5 +++++
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1312004f8c..dfece751b5 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -19,10 +19,6 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	)
 '
 
-modebits () {
-	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
-}
-
 for u in 002 022
 do
 	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
@@ -88,7 +84,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(modebits .git/info/refs)" &&
+		actual="$(test_modebits .git/info/refs)" &&
 		verbose test "x$actual" = "x-$y"
 
 	'
@@ -98,7 +94,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(modebits .git/info/refs)" &&
+		actual="$(test_modebits .git/info/refs)" &&
 		verbose test "x$actual" = "x-$x"
 
 	'
@@ -111,7 +107,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	umask 002 &&
 	git update-server-info &&
 	echo "-rw-rw-r--" >expect &&
-	modebits .git/info/refs >actual &&
+	test_modebits .git/info/refs >actual &&
 	test_cmp expect actual
 '
 
@@ -177,7 +173,7 @@ test_expect_success POSIXPERM 'remote init does not use config from cwd' '
 	umask 0022 &&
 	git init --bare child.git &&
 	echo "-rw-r--r--" >expect &&
-	modebits child.git/config >actual &&
+	test_modebits child.git/config >actual &&
 	test_cmp expect actual
 '
 
@@ -187,7 +183,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
 	echo whatever >templates/foo &&
 	git init --template=templates &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits .git/foo >actual &&
+	test_modebits .git/foo >actual &&
 	test_cmp expect actual
 '
 
@@ -198,7 +194,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
 	test_path_is_missing child.git/foo &&
 	git init --bare --template=../templates child.git &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits child.git/foo >actual &&
+	test_modebits child.git/foo >actual &&
 	test_cmp expect actual
 '
 
@@ -209,7 +205,7 @@ test_expect_success POSIXPERM 'template can set core.sharedrepository' '
 	cp .git/config templates/config &&
 	git init --bare --template=../templates child.git &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits child.git/HEAD >actual &&
+	test_modebits child.git/HEAD >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5ee124332a..db622c3555 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -216,6 +216,11 @@ test_chmod () {
 	git update-index --add "--chmod=$@"
 }
 
+# Get the modebits from a file.
+test_modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
 # Unset a configuration variable, but don't fail if it doesn't exist.
 test_unconfig () {
 	config_dir=
-- 
2.13.1.519.g0a0746bea4

[PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository

[Christian Couder]

Add a few tests to check that both the split-index file and the
shared-index file are created using the right permissions when
core.sharedrepository is set.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1700-split-index.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af3ec0da5a..2c5be732e4 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
 	test $(ls .git/sharedindex.* | wc -l) -le 2
 '
 
+while read -r mode modebits filename; do
+	test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '
+		git config core.sharedrepository "$mode" &&
+		: >"$filename" &&
+		git update-index --add "$filename" &&
+		echo "$modebits" >expect &&
+		test_modebits .git/index >actual &&
+		test_cmp expect actual &&
+		newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&
+		test_modebits "$newest_shared_index" >actual &&
+		test_cmp expect actual
+	'
+done <<\EOF
+0666 -rw-rw-rw- seventeen
+0642 -rw-r---w- eightteen
+EOF
+
 test_done
-- 
2.13.1.519.g0a0746bea4

Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> Because of that, on repositories created with `git init --shared=all`
> and using the split index feature, one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied
>
> when another user performs any operation that reads the shared index.
>
> We could use create_tempfile() that calls adjust_shared_perm(), but
> unfortunately create_tempfile() doesn't replace the XXXXXX at the end
> of the path it is passed. So let's just call adjust_shared_perm() by
> ourselves.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  read-cache.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index bc156a133e..66f85f8d58 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state *istate,
>  		delete_tempfile(&temporary_sharedindex);
>  		return ret;
>  	}
> +	ret = adjust_shared_perm(temporary_sharedindex.filename.buf);

Shouldn't we be using the API function get_tempfile_path() for this
instead of reaching into its implementation detail?

> +	if (ret) {
> +		int save_errno = errno;
> +		error("cannot fix permission bits on %s", temporary_sharedindex.filename.buf);
> +		delete_tempfile(&temporary_sharedindex);
> +		errno = save_errno;
> +		return ret;
> +	}
>  	ret = rename_tempfile(&temporary_sharedindex,
>  			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>  	if (!ret) {

Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> Because of that, on repositories created with `git init --shared=all`
> and using the split index feature, one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied
>
> when another user performs any operation that reads the shared index.
>
> We could use create_tempfile() that calls adjust_shared_perm(), but
> unfortunately create_tempfile() doesn't replace the XXXXXX at the end
> of the path it is passed. So let's just call adjust_shared_perm() by
> ourselves.

Because create_tempfile() is not even a viable alternative, the
above sounds just as silly as saying "We could use X, but
unfortunately that X doesn't create a temporary file and return its
file descriptor" with X replaced with any one of about a dozen
functions that happen to call adjust_shared_perm().

	Call adjust_shared_perm() on the temporary file created by
	mks_tempfile() ourselves to adjust the permission bits.

should be sufficient.

Thanks.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  read-cache.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index bc156a133e..66f85f8d58 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state *istate,
>  		delete_tempfile(&temporary_sharedindex);
>  		return ret;
>  	}
> +	ret = adjust_shared_perm(temporary_sharedindex.filename.buf);
> +	if (ret) {
> +		int save_errno = errno;
> +		error("cannot fix permission bits on %s", temporary_sharedindex.filename.buf);
> +		delete_tempfile(&temporary_sharedindex);
> +		errno = save_errno;
> +		return ret;
> +	}
>  	ret = rename_tempfile(&temporary_sharedindex,
>  			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>  	if (!ret) {

Re: [PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Add a few tests to check that both the split-index file and the
> shared-index file are created using the right permissions when
> core.sharedrepository is set.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t1700-split-index.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index af3ec0da5a..2c5be732e4 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
>  	test $(ls .git/sharedindex.* | wc -l) -le 2
>  '
>  
> +while read -r mode modebits filename; do

Style.

	while read -r mode modebits filename
	do

> +	test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '
> +		git config core.sharedrepository "$mode" &&
> +		: >"$filename" &&
> +		git update-index --add "$filename" &&
> +		echo "$modebits" >expect &&
> +		test_modebits .git/index >actual &&
> +		test_cmp expect actual &&
> +		newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&
> +		test_modebits "$newest_shared_index" >actual &&
> +		test_cmp expect actual
> +	'

Running this twice in a loop would create two .git/sharedindex.*
files in quick succession.  I do not think we want to assume that
the filesystem timestamp can keep up with us to allow "ls -t" to
work reliably in the second round (if there is a leftover shared
index from previous test, even the first round may not catch the
latest one).

How about doing each iteration this way instead?  Which might be a
better solution to work around that.

    - with core.sharedrepository set to false, force the index to be
      unsplit; "index" will have the default unshared permission
      bits (but we do not care what it is and no need to check it).

    - remove any leftover sharedindex.*, if any.

    - with core.sharedrepository set to whatever mode being tested,
      do the adding to force split.

    - test the permission of index file.

    - test the permission of sharedindex.* file; there should be
      only one instance, so erroring out when we see two or more is
      also a good test.

The last two steps may look like:

	test_modebits .git/index >actual && test_cmp expect actual &&
	shared=$(ls .git/sharedindex.*) &&
	case "$shared" in
	*" "*)
		# we have more than one???
		false ;;
	*)	
		test_modebits "shared" >actual &&
		test_cmp expect actual ;;
	esac

> +done <<\EOF
> +0666 -rw-rw-rw- seventeen
> +0642 -rw-r---w- eightteen
> +EOF
> +
>  test_done

Re: [PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository

[Christian Couder]

On Sat, Jun 24, 2017 at 12:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Add a few tests to check that both the split-index file and the
>> shared-index file are created using the right permissions when
>> core.sharedrepository is set.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  t/t1700-split-index.sh | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
>> index af3ec0da5a..2c5be732e4 100755
>> --- a/t/t1700-split-index.sh
>> +++ b/t/t1700-split-index.sh
>> @@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
>>       test $(ls .git/sharedindex.* | wc -l) -le 2
>>  '
>>
>> +while read -r mode modebits filename; do
>
> Style.

Fixed in the version (v3) I just sent.

> Running this twice in a loop would create two .git/sharedindex.*
> files in quick succession.  I do not think we want to assume that
> the filesystem timestamp can keep up with us to allow "ls -t" to
> work reliably in the second round (if there is a leftover shared
> index from previous test, even the first round may not catch the
> latest one).

Yeah, it might be a problem on some systems.

> How about doing each iteration this way instead?  Which might be a
> better solution to work around that.
>
>     - with core.sharedrepository set to false, force the index to be
>       unsplit; "index" will have the default unshared permission
>       bits (but we do not care what it is and no need to check it).
>
>     - remove any leftover sharedindex.*, if any.
>
>     - with core.sharedrepository set to whatever mode being tested,
>       do the adding to force split.
>
>     - test the permission of index file.
>
>     - test the permission of sharedindex.* file; there should be
>       only one instance, so erroring out when we see two or more is
>       also a good test.
>
> The last two steps may look like:
>
>         test_modebits .git/index >actual && test_cmp expect actual &&
>         shared=$(ls .git/sharedindex.*) &&
>         case "$shared" in
>         *" "*)
>                 # we have more than one???
>                 false ;;
>         *)
>                 test_modebits "shared" >actual &&
>                 test_cmp expect actual ;;
>         esac

Ok, it does what you suggest in v3.

Thanks.

Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index

[Christian Couder]

On Sat, Jun 24, 2017 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:

>> Because of that, on repositories created with `git init --shared=all`
>> and using the split index feature, one gets an error like:
>>
>> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied
>>
>> when another user performs any operation that reads the shared index.
>>
>> We could use create_tempfile() that calls adjust_shared_perm(), but
>> unfortunately create_tempfile() doesn't replace the XXXXXX at the end
>> of the path it is passed. So let's just call adjust_shared_perm() by
>> ourselves.
>
> Because create_tempfile() is not even a viable alternative, the
> above sounds just as silly as saying "We could use X, but
> unfortunately that X doesn't create a temporary file and return its
> file descriptor" with X replaced with any one of about a dozen
> functions that happen to call adjust_shared_perm().
>
>         Call adjust_shared_perm() on the temporary file created by
>         mks_tempfile() ourselves to adjust the permission bits.
>
> should be sufficient.

Ok, the v3 has the above in the commit message and also uses
get_tempfile_path().