[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>
---
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

[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