[PATCH] shallow: verify shallow file after taking lock

[PATCH] shallow: verify shallow file after taking lock

[Jeff King]

Before writing the shallow file, we stat() the existing file
to make sure it has not been updated since our operation
began. However, we do not do so under a lock, so there is a
possible race:

  1. Process A takes the lock.

  2. Process B calls check_shallow_file_for_update and finds
     no update.

  3. Process A commits the lockfile.

  4. Process B takes the lock, then overwrite's process A's
     changes.

We can fix this by doing our check while we hold the lock.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a repost of:

  http://article.gmane.org/gmane.comp.version-control.git/242795

You picked up the other two related patches as jk/shallow-update-fix,
but I suspect this one just got lost in the noise because I didn't
format it as a proper series.

 shallow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index bbc98b5..75da07a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -246,9 +246,9 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
@@ -293,9 +293,9 @@ void prune_shallow(int show_only)
 		strbuf_release(&sb);
 		return;
 	}
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-- 
1.9.0.532.g9ab8f58

[PATCH] upload-pack: allow shallow fetching from a read-only repository

[Nguyễn Thái Ngọc Duy]

Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
pack-objects - 2013-08-16) upload-pack does not write to the source
repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a
shallow fetch, so the source repo must be writable.

Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in
this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX
and eventually rename it to $GIT_DIR/shallow, there is no fallback to
$TMPDIR.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/receive-pack.c   |  2 +-
 commit.h                 |  2 +-
 fetch-pack.c             |  2 +-
 shallow.c                | 13 +++++++++++--
 t/t5537-fetch-shallow.sh | 13 +++++++++++++
 upload-pack.c            |  2 +-
 6 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..9d96f2c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -936,7 +936,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
 	if (si->nr_ours || si->nr_theirs) {
-		alt_shallow_file = setup_temporary_shallow(si->shallow);
+		alt_shallow_file = setup_temporary_shallow(si->shallow, 0);
 		argv_array_pushl(&av, "--shallow-file", alt_shallow_file, NULL);
 	}
 
diff --git a/commit.h b/commit.h
index 16d9c43..44a40ad 100644
--- a/commit.h
+++ b/commit.h
@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
 				    const char **alternate_shallow_file,
 				    const struct sha1_array *extra);
-extern char *setup_temporary_shallow(const struct sha1_array *extra);
+extern char *setup_temporary_shallow(const struct sha1_array *extra, int read_only);
 extern void advertise_shallow_grafts(int);
 
 struct shallow_info {
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..382b825 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 	else if (si->nr_ours || si->nr_theirs)
-		alternate_shallow_file = setup_temporary_shallow(si->shallow);
+		alternate_shallow_file = setup_temporary_shallow(si->shallow, 0);
 	else
 		alternate_shallow_file = NULL;
 	if (get_pack(args, fd, pack_lockfile))
diff --git a/shallow.c b/shallow.c
index bbc98b5..3adfbd5 100644
--- a/shallow.c
+++ b/shallow.c
@@ -216,7 +216,7 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
-char *setup_temporary_shallow(const struct sha1_array *extra)
+char *setup_temporary_shallow(const struct sha1_array *extra, int read_only)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
@@ -224,7 +224,16 @@ char *setup_temporary_shallow(const struct sha1_array *extra)
 	if (write_shallow_commits(&sb, 0, extra)) {
 		struct strbuf path = STRBUF_INIT;
 		strbuf_addstr(&path, git_path("shallow_XXXXXX"));
-		fd = xmkstemp(path.buf);
+		if (read_only) {
+			fd = mkstemp(path.buf);
+			if (fd < 0) {
+				char buf[PATH_MAX];
+				fd = git_mkstemp(buf, sizeof(buf), "shallow_XXXXXX");
+			}
+			if (fd < 0)
+				die_errno("Unable to create temporary shallow file");
+		} else
+			fd = xmkstemp(path.buf);
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
 				  path.buf);
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..171db88 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,6 +173,19 @@ EOF
 	)
 '
 
+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '
+	cp -R .git read-only.git &&
+	find read-only.git -print | xargs chmod -w &&
+	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	git clone --no-local --depth=2 read-only.git from-read-only &&
+	git --git-dir=from-read-only/.git log --format=%s >actual &&
+	cat >expect <<EOF &&
+add-1-back
+4
+EOF
+	test_cmp expect actual
+'
+
 if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	say 'skipping remaining tests, git built without http support'
 	test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..11404b4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -84,7 +84,7 @@ static void create_pack_file(void)
 	char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
+		shallow_file = setup_temporary_shallow(NULL, 1);
 		argv[arg++] = "--shallow-file";
 		argv[arg++] = shallow_file;
 	}
-- 
1.9.0.66.g14f785a

Re: [PATCH] upload-pack: allow shallow fetching from a read-only repository

[Jeff King]

On Thu, Feb 27, 2014 at 02:13:03PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
> pack-objects - 2013-08-16) upload-pack does not write to the source
> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a
> shallow fetch, so the source repo must be writable.
> 
> Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in
> this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX
> and eventually rename it to $GIT_DIR/shallow, there is no fallback to
> $TMPDIR.

That makes sense.

> @@ -224,7 +224,16 @@ char *setup_temporary_shallow(const struct sha1_array *extra)
>  	if (write_shallow_commits(&sb, 0, extra)) {
>  		struct strbuf path = STRBUF_INIT;
>  		strbuf_addstr(&path, git_path("shallow_XXXXXX"));
> -		fd = xmkstemp(path.buf);
> +		if (read_only) {
> +			fd = mkstemp(path.buf);
> +			if (fd < 0) {
> +				char buf[PATH_MAX];
> +				fd = git_mkstemp(buf, sizeof(buf), "shallow_XXXXXX");
> +			}
> +			if (fd < 0)
> +				die_errno("Unable to create temporary shallow file");
> +		} else
> +			fd = xmkstemp(path.buf);

This is not introduced by your patch, but I notice that we do not seem
to do anything with the tempfiles when the program dies prematurely.
We've started collecting stale shallow_XXXXXX files in our server repos.

For the writable cases, should we be using the lockfile API to take
shallow.lock? It looks like we do take a lock on it, but only after the
fetch, and then we have to do a manual check for it having moved (by the
way, shouldn't we do that check under the lock? I think
setup_alternate_shallow has a race condition).

I guess the reason to take the lock at the last minute is that the whole
fetch is heavyweight. But if the fetches are going to conflict, perhaps
it is better to have lock contention at the beginning, rather than
failing only at the end. I don't know very much about the shallow
system; does each operation rewrite the shallow file, or is it often
left untouched (in which case two simultaneous fetches could coexist
most of the time).

At any rate, if we used the lockfile API, it handles tempfile cleanup
automatically. Otherwise, I think we need something like the patch
below (in the interest of simplicity, I just drop all of the explicit
unlinks and let them use the atexit handler to clean up; you could also
add a function to explicitly unlink the tempfile and clear the handler).

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..ac1d9b5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -833,8 +833,6 @@ static void execute_commands(struct command *commands,
 			error("BUG: run 'git fsck' for safety.\n"
 			      "If there are errors, try to remove "
 			      "the reported refs above");
-		if (alt_shallow_file && *alt_shallow_file)
-			unlink(alt_shallow_file);
 	}
 }
 
@@ -1087,10 +1085,6 @@ static void update_shallow_info(struct command *commands,
 			cmd->skip_update = 1;
 		}
 	}
-	if (alt_shallow_file && *alt_shallow_file) {
-		unlink(alt_shallow_file);
-		alt_shallow_file = NULL;
-	}
 	free(ref_status);
 }
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..ae8550e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,
 	if (!si->shallow || !si->shallow->nr)
 		return;
 
-	if (alternate_shallow_file) {
-		/*
-		 * The temporary shallow file is only useful for
-		 * index-pack and unpack-objects because it may
-		 * contain more roots than we want. Delete it.
-		 */
-		if (*alternate_shallow_file)
-			unlink(alternate_shallow_file);
-		free((char *)alternate_shallow_file);
-	}
-
 	if (args->cloning) {
 		/*
 		 * remote is shallow, but this is a clone, there are
diff --git a/shallow.c b/shallow.c
index bbc98b5..0f2bb48 100644
--- a/shallow.c
+++ b/shallow.c
@@ -8,6 +8,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
+#include "sigchain.h"
 
 static int is_shallow = -1;
 static struct stat shallow_stat;
@@ -216,27 +217,49 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
+static struct strbuf shallow_tmpfile = STRBUF_INIT;
+
+static void remove_shallow_tmpfile(void)
+{
+	if (shallow_tmpfile.len) {
+		unlink_or_warn(shallow_tmpfile.buf);
+		strbuf_reset(&shallow_tmpfile);
+	}
+}
+
+static void remove_shallow_tmpfile_on_signal(int signo)
+{
+	remove_shallow_tmpfile();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
 char *setup_temporary_shallow(const struct sha1_array *extra)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
 	if (write_shallow_commits(&sb, 0, extra)) {
-		struct strbuf path = STRBUF_INIT;
-		strbuf_addstr(&path, git_path("shallow_XXXXXX"));
-		fd = xmkstemp(path.buf);
+		strbuf_addstr(&shallow_tmpfile, git_path("shallow_XXXXXX"));
+		fd = xmkstemp(shallow_tmpfile.buf);
+
+		/* XXX can there be multiple shallow tempfiles in one program?
+		 * In that case, we would need to maintain a list */
+		atexit(remove_shallow_tmpfile);
+		sigchain_push_common(remove_shallow_tmpfile_on_signal);
+
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  path.buf);
+				  shallow_tmpfile.buf);
 		close(fd);
 		strbuf_release(&sb);
-		return strbuf_detach(&path, NULL);
+		return shallow_tmpfile.buf;
 	}
 	/*
 	 * is_repository_shallow() sees empty string as "no shallow
 	 * file".
 	 */
-	return xstrdup("");
+	return shallow_tmpfile.buf;
 }
 
 void setup_alternate_shallow(struct lock_file *shallow_lock,
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..55c1f71 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -242,11 +242,6 @@ static void create_pack_file(void)
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}
-	if (shallow_file) {
-		if (*shallow_file)
-			unlink(shallow_file);
-		free(shallow_file);
-	}
 
 	/* flush the data */
 	if (0 <= buffered) {

[PATCH] shallow: verify shallow file after taking lock

[Jeff King]

Before writing the shallow file, we stat() the existing file
to make sure it has not been updated since our operation
began. However, we do not do so under a lock, so there is a
possible race:

  1. Process A takes the lock.

  2. Process B calls check_shallow_file_for_update and finds
     no update.

  3. Process A commits the lockfile.

  4. Process B takes the lock, then overwrite's process A's
     changes.

We can fix this by doing our check while we hold the lock.

Signed-off-by: Jeff King <peff@peff.net>
---
On Thu, Feb 27, 2014 at 04:04:26AM -0500, Jeff King wrote:

> by the way, shouldn't we do that check under the lock? I think
> setup_alternate_shallow has a race condition).

Here is the fix. I didn't actually reproduce the race experimentally,
but it seems pretty straightforward.

I also notice that check_shallow_file_for_update returns early if
!is_shallow. Is that safe? Is it possible for another process to have
made us shallow since the program began? In that case, we would have to
stat() the file always, then complain if it exists and !is_shallow.

 shallow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index bbc98b5..75da07a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -246,9 +246,9 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
@@ -293,9 +293,9 @@ void prune_shallow(int show_only)
 		strbuf_release(&sb);
 		return;
 	}
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-- 
1.8.5.2.500.g8060133

Re: [PATCH] shallow: verify shallow file after taking lock

[Jeff King]

On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:

> I also notice that check_shallow_file_for_update returns early if
> !is_shallow. Is that safe? Is it possible for another process to have
> made us shallow since the program began? In that case, we would have to
> stat() the file always, then complain if it exists and !is_shallow.

That patch would look like this:

diff --git a/shallow.c b/shallow.c
index 75da07a..e05a241 100644
--- a/shallow.c
+++ b/shallow.c
@@ -139,13 +139,13 @@ void check_shallow_file_for_update(void)
 {
 	struct stat st;
 
-	if (!is_shallow)
-		return;
-	else if (is_shallow == -1)
+	if (is_shallow == -1)
 		die("BUG: shallow must be initialized by now");
 
 	if (stat(git_path("shallow"), &st))
 		die("shallow file was removed during fetch");
+	else if (!is_shallow)
+		die("shallow file appeared during fetch");
 	else if (st.st_mtime != shallow_stat.st_mtime
 #ifdef USE_NSEC
 		 || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)

but again, I'm not really clear on whether this is possible.

-Peff

Re: [PATCH] shallow: verify shallow file after taking lock

[Duy Nguyen]

On Thu, Feb 27, 2014 at 4:22 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:
>
>> I also notice that check_shallow_file_for_update returns early if
>> !is_shallow. Is that safe? Is it possible for another process to have
>> made us shallow since the program began? In that case, we would have to
>> stat() the file always, then complain if it exists and !is_shallow.

I think it's safer to do it your way.

>
> That patch would look like this:
>
> diff --git a/shallow.c b/shallow.c
> index 75da07a..e05a241 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -139,13 +139,13 @@ void check_shallow_file_for_update(void)
>  {
>         struct stat st;
>
> -       if (!is_shallow)
> -               return;
> -       else if (is_shallow == -1)
> +       if (is_shallow == -1)
>                 die("BUG: shallow must be initialized by now");
>
>         if (stat(git_path("shallow"), &st))
>                 die("shallow file was removed during fetch");
> +       else if (!is_shallow)
> +               die("shallow file appeared during fetch");
>         else if (st.st_mtime != shallow_stat.st_mtime
>  #ifdef USE_NSEC
>                  || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)
>
> but again, I'm not really clear on whether this is possible.
>
> -Peff



-- 
Duy