[PATCH v5] lockfile.c: store absolute path

Michael Haggerty· Nov 2, 2014, 06:24 UTC · lore

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Locked paths can be saved in a linked list so that if something wrong happens, *.lock are removed. For relative paths, this works fine if we keep cwd the same, which is true 99% of time except:

- update-index and read-tree hold the lock on $GIT_DIR/index really
  early, then later on may call setup_work_tree() to move cwd.

- Suppose a lock is being held (e.g. by "git add") then somewhere
  down the line, somebody calls real_path (e.g. "link_alt_odb_entry"),
  which temporarily moves cwd away and back.

During that time when cwd is moved (either permanently or temporarily) and we decide to die(), attempts to remove relative *.lock will fail, and the next operation will complain that some files are still locked.

Avoid this case by turning relative paths to absolute before storing the path in "filename" field.

Reported-by: Yue Lin Ho <yuelinho777@gmail.com>
Helped-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Adapted-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This is a re-roll onto master to resolve conflicts with other changes
that have been merged since v4 [1]:

* The length of path is now available, so use strbuf_add() instead of
  strbuf_addstr().

* LOCK_NODEREF has been renamed to LOCK_NO_DEREF.

[1] http://thread.gmane.org/gmane.comp.version-control.git/255069/focus=256584

 lockfile.c                    | 14 +++++++++++---
 t/t2107-update-index-basic.sh | 15 +++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

Show changes to 2 files +26 −3

lockfile.c, t/t2107-update-index-basic.sh

diff --git a/lockfile.c b/lockfile.c
index 4f16ee7..9889277 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -128,9 +128,17 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		    path);
 	}
 
-	strbuf_add(&lk->filename, path, pathlen);
-	if (!(flags & LOCK_NO_DEREF))
-		resolve_symlink(&lk->filename);
+	if (flags & LOCK_NO_DEREF) {
+		strbuf_add_absolute_path(&lk->filename, path);
+	} else {
+		struct strbuf resolved_path = STRBUF_INIT;
+
+		strbuf_add(&resolved_path, path, pathlen);
+		resolve_symlink(&resolved_path);
+		strbuf_add_absolute_path(&lk->filename, resolved_path.buf);
+		strbuf_release(&resolved_path);
+	}
+
 	strbuf_addstr(&lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index 1bafb90..dfe02f4 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -65,4 +65,19 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '
 	test_cmp expect actual
 '
 
+test_expect_success '.lock files cleaned up' '
+	mkdir cleanup &&
+	(
+	cd cleanup &&
+	mkdir worktree &&
+	git init repo &&
+	cd repo &&
+	git config core.worktree ../../worktree &&
+	# --refresh triggers late setup_work_tree,
+	# active_cache_changed is zero, rollback_lock_file fails
+	git update-index --refresh &&
+	! test -f .git/index.lock
+	)
+'
+
 test_done

-- 
2.1.1

Scott Schmit· Nov 5, 2014, 02:23 UTC · re: Michael Haggerty · lore

Re: [PATCH v5] lockfile.c: store absolute path

On Sun, Nov 02, 2014 at 07:24:37AM +0100, Michael Haggerty wrote:

Show 17 quoted lines

> Locked paths can be saved in a linked list so that if something wrong
> happens, *.lock are removed. For relative paths, this works fine if we
> keep cwd the same, which is true 99% of time except:
> 
> - update-index and read-tree hold the lock on $GIT_DIR/index really
>   early, then later on may call setup_work_tree() to move cwd.
> 
> - Suppose a lock is being held (e.g. by "git add") then somewhere
>   down the line, somebody calls real_path (e.g. "link_alt_odb_entry"),
>   which temporarily moves cwd away and back.
> 
> During that time when cwd is moved (either permanently or temporarily)
> and we decide to die(), attempts to remove relative *.lock will fail,
> and the next operation will complain that some files are still locked.
> 
> Avoid this case by turning relative paths to absolute before storing
> the path in "filename" field.

This might be a little pathological, but it seems like this scheme would run into trouble if the entire repo is moved while the lock is held.

Michael Haggerty· Nov 5, 2014, 14:19 UTC · re: Scott Schmit · lore

Re: [PATCH v5] lockfile.c: store absolute path

On 11/05/2014 03:23 AM, Scott Schmit wrote:

Show 21 quoted lines

> On Sun, Nov 02, 2014 at 07:24:37AM +0100, Michael Haggerty wrote:
>> Locked paths can be saved in a linked list so that if something wrong
>> happens, *.lock are removed. For relative paths, this works fine if we
>> keep cwd the same, which is true 99% of time except:
>>
>> - update-index and read-tree hold the lock on $GIT_DIR/index really
>>   early, then later on may call setup_work_tree() to move cwd.
>>
>> - Suppose a lock is being held (e.g. by "git add") then somewhere
>>   down the line, somebody calls real_path (e.g. "link_alt_odb_entry"),
>>   which temporarily moves cwd away and back.
>>
>> During that time when cwd is moved (either permanently or temporarily)
>> and we decide to die(), attempts to remove relative *.lock will fail,
>> and the next operation will complain that some files are still locked.
>>
>> Avoid this case by turning relative paths to absolute before storing
>> the path in "filename" field.
> 
> This might be a little pathological, but it seems like this scheme would
> run into trouble if the entire repo is moved while the lock is held.

Correct. You shouldn't move a repository while Git operations are running in it. That would break for many reasons, not just because of this change to lockfile handling.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

v5lockfile.c: store absolute path