[PATCH] Make locked paths absolute when current directory is changed

[PATCH] Make locked paths absolute when current directory is changed

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

Locked paths are saved in a linked list so that if something wrong
happens, *.lock are removed. 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 when chdir() is
called (or soon to be called, in setup_git_directory_gently case).

Reported-by: Yue Lin Ho <yuelinho777@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 It occurred to me while writing this commit message that it would be
 better if we can unlink via a file descriptor so we don't have to
 convert paths. But I'm not sure about the availability of unlinkat(),
 especially on Windows.

 abspath.c     |  2 +-
 cache.h       |  6 ++++++
 git.c         |  6 +++---
 lockfile.c    | 16 ++++++++++++++++
 path.c        |  4 ++--
 run-command.c |  2 +-
 setup.c       |  3 ++-
 unix-socket.c |  2 +-
 8 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/abspath.c b/abspath.c
index ca33558..78c963f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -87,7 +87,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 					goto error_out;
 			}
 
-			if (chdir(buf)) {
+			if (chdir_safe(buf)) {
 				if (die_on_error)
 					die_errno("Could not switch to '%s'", buf);
 				else
diff --git a/cache.h b/cache.h
index 44aa439..d3f2596 100644
--- a/cache.h
+++ b/cache.h
@@ -564,6 +564,12 @@ extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
 extern void update_index_if_able(struct index_state *, struct lock_file *);
+extern void make_locked_paths_absolute(void);
+static inline int chdir_safe(const char *path)
+{
+	make_locked_paths_absolute();
+	return chdir(path);
+}
 
 extern int hold_locked_index(struct lock_file *, int);
 extern int commit_locked_index(struct lock_file *);
diff --git a/git.c b/git.c
index 5b6c761..27766c3 100644
--- a/git.c
+++ b/git.c
@@ -48,7 +48,7 @@ static void save_env(void)
 static void restore_env(void)
 {
 	int i;
-	if (*orig_cwd && chdir(orig_cwd))
+	if (*orig_cwd && chdir_safe(orig_cwd))
 		die_errno("could not move to %s", orig_cwd);
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		if (orig_env[i])
@@ -206,7 +206,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				fprintf(stderr, "No directory given for -C.\n" );
 				usage(git_usage_string);
 			}
-			if (chdir((*argv)[1]))
+			if (chdir_safe((*argv)[1]))
 				die_errno("Cannot change to '%s'", (*argv)[1]);
 			if (envchanged)
 				*envchanged = 1;
@@ -292,7 +292,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	if (subdir && chdir(subdir))
+	if (subdir && chdir_safe(subdir))
 		die_errno("Cannot change to '%s'", subdir);
 
 	errno = saved_errno;
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..a70d107 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)
 	}
 	lk->filename[0] = 0;
 }
+
+void make_locked_paths_absolute(void)
+{
+	struct lock_file *lk;
+	const char *abspath;
+	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
+		if (!lk->filename[0] || lk->filename[0] == '/')
+			continue;
+		abspath = absolute_path(lk->filename);
+		if (strlen(abspath) >= sizeof(lk->filename))
+			warning("locked path %s is relative when current directory "
+				"is changed", lk->filename);
+		else
+			strcpy(lk->filename, abspath);
+	}
+}
diff --git a/path.c b/path.c
index bc804a3..9e8e101 100644
--- a/path.c
+++ b/path.c
@@ -372,11 +372,11 @@ const char *enter_repo(const char *path, int strict)
 		gitfile = read_gitfile(used_path) ;
 		if (gitfile)
 			strcpy(used_path, gitfile);
-		if (chdir(used_path))
+		if (chdir_safe(used_path))
 			return NULL;
 		path = validated_path;
 	}
-	else if (chdir(path))
+	else if (chdir_safe(path))
 		return NULL;
 
 	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
diff --git a/run-command.c b/run-command.c
index be07d4a..55f360e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -399,7 +399,7 @@ fail_pipe:
 			close(cmd->out);
 		}
 
-		if (cmd->dir && chdir(cmd->dir))
+		if (cmd->dir && chdir_safe(cmd->dir))
 			die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
 			    cmd->dir);
 		if (cmd->env) {
diff --git a/setup.c b/setup.c
index 0a22f8b..921045e 100644
--- a/setup.c
+++ b/setup.c
@@ -290,7 +290,7 @@ void setup_work_tree(void)
 	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
 		git_dir = real_path(get_git_dir());
-	if (!work_tree || chdir(work_tree))
+	if (!work_tree || chdir_safe(work_tree))
 		die("This operation must be run in a work tree");
 
 	/*
@@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		die_errno("Unable to read current working directory");
 	offset = len = strlen(cwd);
 
+	make_locked_paths_absolute();
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
 	 * to do any discovery, but we still do repository
diff --git a/unix-socket.c b/unix-socket.c
index 01f119f..eeb8007 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -12,7 +12,7 @@ static int unix_stream_socket(void)
 static int chdir_len(const char *orig, int len)
 {
 	char *path = xmemdupz(orig, len);
-	int r = chdir(path);
+	int r = chdir_safe(path);
 	free(path);
 	return r;
 }
-- 
1.9.1.346.ga2b5940

Re: [PATCH] Make locked paths absolute when current directory is changed

[Junio C Hamano]

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

> Locked paths are saved in a linked list so that if something wrong
> happens, *.lock are removed. 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 when chdir() is
> called (or soon to be called, in setup_git_directory_gently case).

The rationale makes sense.

> +extern void make_locked_paths_absolute(void);
> +static inline int chdir_safe(const char *path)
> +{
> +	make_locked_paths_absolute();
> +	return chdir(path);
> +}

Clever ;-).  Instead of making paths absolute when you receive
requests to lock them, you lazily turn the ones relative to cwd()
absolute just before they are about to become invalid/problematic
because the program wants to chdir.

> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..a70d107 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)
>  	}
>  	lk->filename[0] = 0;
>  }
> +
> +void make_locked_paths_absolute(void)
> +{
> +	struct lock_file *lk;
> +	const char *abspath;
> +	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
> +		if (!lk->filename[0] || lk->filename[0] == '/')
> +			continue;

Do we have to worry about Windows?

> +		abspath = absolute_path(lk->filename);
> +		if (strlen(abspath) >= sizeof(lk->filename))
> +			warning("locked path %s is relative when current directory "
> +				"is changed", lk->filename);

Shouldn't this be a die() or an error return (which will kill the
caller anyway)?

> @@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>  		die_errno("Unable to read current working directory");
>  	offset = len = strlen(cwd);
>  
> +	make_locked_paths_absolute();

Just being curious, but this early in the start-up sequence, what
files do we have locks on?

Re: [PATCH] Make locked paths absolute when current directory is changed

[Duy Nguyen]

On Sat, Jul 19, 2014 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +             abspath = absolute_path(lk->filename);
>> +             if (strlen(abspath) >= sizeof(lk->filename))
>> +                     warning("locked path %s is relative when current directory "
>> +                             "is changed", lk->filename);
>
> Shouldn't this be a die() or an error return (which will kill the
> caller anyway)?

We don't know for sure there will be a die() or something to trigger
the roll back (or commit). If the chdir() is temporary, absolute path
not fitting in PATH_MAX chars is not fatal because cwd will be
reverted before commit/rollback. A better solution is probably avoid
PATH_MAX in lk->filename. But yeah, changing it to die() is safer
(especially when cwd is moved permanently for some options in
update-index and read-tree)

>> @@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>>               die_errno("Unable to read current working directory");
>>       offset = len = strlen(cwd);
>>
>> +     make_locked_paths_absolute();
>
> Just being curious, but this early in the start-up sequence, what
> files do we have locks on?

We don't know. For most builtin commands, the setup is done early and
we can be sure of no locks. Some commands (especially non-builtin) can
still delay calling setup_git_directory() until later and they might
do something in between, so better be safe than sorry.
-- 
Duy

Re: [PATCH] Make locked paths absolute when current directory is changed

[Johannes Sixt]

Am 18.07.2014 15:08, schrieb Nguyễn Thái Ngọc Duy:
> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..a70d107 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)
>  	}
>  	lk->filename[0] = 0;
>  }
> +
> +void make_locked_paths_absolute(void)
> +{
> +	struct lock_file *lk;
> +	const char *abspath;
> +	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
> +		if (!lk->filename[0] || lk->filename[0] == '/')

Please use is_absolute_path().

> +			continue;
> +		abspath = absolute_path(lk->filename);
> +		if (strlen(abspath) >= sizeof(lk->filename))
> +			warning("locked path %s is relative when current directory "
> +				"is changed", lk->filename);
> +		else
> +			strcpy(lk->filename, abspath);
> +	}
> +}

> --- a/run-command.c
> +++ b/run-command.c
> @@ -399,7 +399,7 @@ fail_pipe:
>  			close(cmd->out);
>  		}
>  
> -		if (cmd->dir && chdir(cmd->dir))
> +		if (cmd->dir && chdir_safe(cmd->dir))

This one shouldn't be necessary: It's in the child, and the child
process does not release the locks; see the check for the owner in
remove_lock_file.

>  			die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
>  			    cmd->dir);
>  		if (cmd->env) {

-- Hannes

[PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

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

Something extra is, because struct lock_file is usually used as static
variables in many places. This patch reduces bss section by about 80k
bytes (or 23%) on Linux.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This helps remove the length check in v1 of the next patch.

 cache.h    |  2 +-
 lockfile.c | 56 ++++++++++++++++++++++++++++++++------------------------
 2 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/cache.h b/cache.h
index 44aa439..9ecb636 100644
--- a/cache.h
+++ b/cache.h
@@ -554,7 +554,7 @@ struct lock_file {
 	int fd;
 	pid_t owner;
 	char on_list;
-	char filename[PATH_MAX];
+	char *filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..968b28f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -7,13 +7,19 @@
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
 
+static void clear_filename(struct lock_file *lk)
+{
+	free(lk->filename);
+	lk->filename = NULL;
+}
+
 static void remove_lock_file(void)
 {
 	pid_t me = getpid();
 
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0]) {
+		    lock_file_list->filename) {
 			if (lock_file_list->fd >= 0)
 				close(lock_file_list->fd);
 			unlink_or_warn(lock_file_list->filename);
@@ -77,10 +83,16 @@ static char *last_path_elm(char *p)
  * Always returns p.
  */
 
-static char *resolve_symlink(char *p, size_t s)
+static char *resolve_symlink(const char *in)
 {
+	static char p[PATH_MAX];
+	size_t s = sizeof(p);
 	int depth = MAXDEPTH;
 
+	if (strlen(in) >= sizeof(p))
+		return NULL;
+	strcpy(p, in);
+
 	while (depth--) {
 		char link[PATH_MAX];
 		int link_len = readlink(p, link, sizeof(link));
@@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
 
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	/*
-	 * subtract 5 from size to make sure there's room for adding
-	 * ".lock" for the lock file name
-	 */
-	static const size_t max_path_len = sizeof(lk->filename) - 5;
-
-	if (strlen(path) >= max_path_len)
+	int len;
+	if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
 		return -1;
+	len = strlen(path) + 5; /* .lock */
+	lk->filename = xmallocz(len);
 	strcpy(lk->filename, path);
-	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(lk->filename, max_path_len);
 	strcat(lk->filename, ".lock");
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
@@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 				     lk->filename);
 	}
 	else
-		lk->filename[0] = 0;
+		clear_filename(lk);
 	return lk->fd;
 }
 
@@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	char result_file[PATH_MAX];
-	size_t i;
-	if (lk->fd >= 0 && close_lock_file(lk))
+	char *result_file;
+	if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
 		return -1;
-	strcpy(result_file, lk->filename);
-	i = strlen(result_file) - 5; /* .lock */
-	result_file[i] = 0;
-	if (rename(lk->filename, result_file))
+	result_file = xmemdupz(lk->filename,
+			       strlen(lk->filename) - 5 /* .lock */);
+	if (rename(lk->filename, result_file)) {
+		free(result_file);
 		return -1;
-	lk->filename[0] = 0;
+	}
+	free(result_file);
+	clear_filename(lk);
 	return 0;
 }
 
@@ -260,11 +268,11 @@ void set_alternate_index_output(const char *name)
 int commit_locked_index(struct lock_file *lk)
 {
 	if (alternate_index_output) {
-		if (lk->fd >= 0 && close_lock_file(lk))
+		if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
 			return -1;
 		if (rename(lk->filename, alternate_index_output))
 			return -1;
-		lk->filename[0] = 0;
+		clear_filename(lk);
 		return 0;
 	}
 	else
@@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename[0]) {
+	if (lk->filename) {
 		if (lk->fd >= 0)
 			close(lk->fd);
 		unlink_or_warn(lk->filename);
 	}
-	lk->filename[0] = 0;
+	clear_filename(lk);
 }
-- 
1.9.1.346.ga2b5940

[PATCH v2 2/2] Make locked paths absolute when current directory is changed

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

Locked paths are saved in a linked list so that if something wrong
happens, *.lock are removed. 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 when chdir() is
called (or soon to be called, in setup_git_directory_gently case).

Reported-by: Yue Lin Ho <yuelinho777@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Compared to v1, make_locked_paths_absolute() now always succeeds (ok
 absolute_path could die inside, but that's a separate problem) and
 supports Windows.

 abspath.c     |  2 +-
 cache.h       |  6 ++++++
 git.c         |  6 +++---
 lockfile.c    | 12 ++++++++++++
 path.c        |  4 ++--
 run-command.c |  2 +-
 setup.c       |  3 ++-
 unix-socket.c |  2 +-
 8 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/abspath.c b/abspath.c
index ca33558..78c963f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -87,7 +87,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 					goto error_out;
 			}
 
-			if (chdir(buf)) {
+			if (chdir_safe(buf)) {
 				if (die_on_error)
 					die_errno("Could not switch to '%s'", buf);
 				else
diff --git a/cache.h b/cache.h
index 9ecb636..48ffa21 100644
--- a/cache.h
+++ b/cache.h
@@ -564,6 +564,12 @@ extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
 extern void update_index_if_able(struct index_state *, struct lock_file *);
+extern void make_locked_paths_absolute(void);
+static inline int chdir_safe(const char *path)
+{
+	make_locked_paths_absolute();
+	return chdir(path);
+}
 
 extern int hold_locked_index(struct lock_file *, int);
 extern int commit_locked_index(struct lock_file *);
diff --git a/git.c b/git.c
index 5b6c761..27766c3 100644
--- a/git.c
+++ b/git.c
@@ -48,7 +48,7 @@ static void save_env(void)
 static void restore_env(void)
 {
 	int i;
-	if (*orig_cwd && chdir(orig_cwd))
+	if (*orig_cwd && chdir_safe(orig_cwd))
 		die_errno("could not move to %s", orig_cwd);
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		if (orig_env[i])
@@ -206,7 +206,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				fprintf(stderr, "No directory given for -C.\n" );
 				usage(git_usage_string);
 			}
-			if (chdir((*argv)[1]))
+			if (chdir_safe((*argv)[1]))
 				die_errno("Cannot change to '%s'", (*argv)[1]);
 			if (envchanged)
 				*envchanged = 1;
@@ -292,7 +292,7 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	if (subdir && chdir(subdir))
+	if (subdir && chdir_safe(subdir))
 		die_errno("Cannot change to '%s'", subdir);
 
 	errno = saved_errno;
diff --git a/lockfile.c b/lockfile.c
index 968b28f..cf1e795 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -288,3 +288,15 @@ void rollback_lock_file(struct lock_file *lk)
 	}
 	clear_filename(lk);
 }
+
+void make_locked_paths_absolute(void)
+{
+	struct lock_file *lk;
+	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
+		if (lk->filename && !is_absolute_path(lk->filename)) {
+			char *to_free = lk->filename;
+			lk->filename = xstrdup(absolute_path(lk->filename));
+			free(to_free);
+		}
+	}
+}
diff --git a/path.c b/path.c
index bc804a3..9e8e101 100644
--- a/path.c
+++ b/path.c
@@ -372,11 +372,11 @@ const char *enter_repo(const char *path, int strict)
 		gitfile = read_gitfile(used_path) ;
 		if (gitfile)
 			strcpy(used_path, gitfile);
-		if (chdir(used_path))
+		if (chdir_safe(used_path))
 			return NULL;
 		path = validated_path;
 	}
-	else if (chdir(path))
+	else if (chdir_safe(path))
 		return NULL;
 
 	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
diff --git a/run-command.c b/run-command.c
index be07d4a..55f360e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -399,7 +399,7 @@ fail_pipe:
 			close(cmd->out);
 		}
 
-		if (cmd->dir && chdir(cmd->dir))
+		if (cmd->dir && chdir_safe(cmd->dir))
 			die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
 			    cmd->dir);
 		if (cmd->env) {
diff --git a/setup.c b/setup.c
index 0a22f8b..921045e 100644
--- a/setup.c
+++ b/setup.c
@@ -290,7 +290,7 @@ void setup_work_tree(void)
 	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
 		git_dir = real_path(get_git_dir());
-	if (!work_tree || chdir(work_tree))
+	if (!work_tree || chdir_safe(work_tree))
 		die("This operation must be run in a work tree");
 
 	/*
@@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		die_errno("Unable to read current working directory");
 	offset = len = strlen(cwd);
 
+	make_locked_paths_absolute();
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
 	 * to do any discovery, but we still do repository
diff --git a/unix-socket.c b/unix-socket.c
index 01f119f..eeb8007 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -12,7 +12,7 @@ static int unix_stream_socket(void)
 static int chdir_len(const char *orig, int len)
 {
 	char *path = xmemdupz(orig, len);
-	int r = chdir(path);
+	int r = chdir_safe(path);
 	free(path);
 	return r;
 }
-- 
1.9.1.346.ga2b5940

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Ramsay Jones]

On 20/07/14 13:13, Nguyễn Thái Ngọc Duy wrote:
> Locked paths are saved in a linked list so that if something wrong
> happens, *.lock are removed. 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 when chdir() is
> called (or soon to be called, in setup_git_directory_gently case).
> 
> Reported-by: Yue Lin Ho <yuelinho777@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

[snip]

> diff --git a/lockfile.c b/lockfile.c
> index 968b28f..cf1e795 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -288,3 +288,15 @@ void rollback_lock_file(struct lock_file *lk)
>  	}
>  	clear_filename(lk);
>  }
> +
> +void make_locked_paths_absolute(void)
> +{
> +	struct lock_file *lk;
> +	for (lk = lock_file_list; lk != NULL; lk = lk->next) {
> +		if (lk->filename && !is_absolute_path(lk->filename)) {
> +			char *to_free = lk->filename;
> +			lk->filename = xstrdup(absolute_path(lk->filename));
> +			free(to_free);
> +		}
> +	}
> +}

I just have to ask, why are we putting relative pathnames in this
list to begin with? Why not use an absolute path when taking the
lock in all cases? (calling absolute_path() and using the result
to take the lock, storing it in the lock_file list, should not be
in the critical path, right? Not that I have measured it, of course! :)

ATB,
Ramsay Jones

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Duy Nguyen]

On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
>> +void make_locked_paths_absolute(void)
>> +{
>> +     struct lock_file *lk;
>> +     for (lk = lock_file_list; lk != NULL; lk = lk->next) {
>> +             if (lk->filename && !is_absolute_path(lk->filename)) {
>> +                     char *to_free = lk->filename;
>> +                     lk->filename = xstrdup(absolute_path(lk->filename));
>> +                     free(to_free);
>> +             }
>> +     }
>> +}
>
> I just have to ask, why are we putting relative pathnames in this
> list to begin with? Why not use an absolute path when taking the
> lock in all cases? (calling absolute_path() and using the result
> to take the lock, storing it in the lock_file list, should not be
> in the critical path, right? Not that I have measured it, of course! :)

Conservative :) I'm still scared from 044bbbc (Make git_dir a path
relative to work_tree in setup_work_tree() - 2008-06-19). But yeah
looking through "grep hold_" I think none of the locks is in critical
path. absolute_path() can die() if cwd is longer than PATH_MAX (and
doing this reduces the chances of that happening). But René is adding
strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be
fine with putting absolute_path() in hold_lock_file_...*
-- 
Duy

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Ramsay Jones]

On 21/07/14 14:47, Duy Nguyen wrote:
> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> wrote:
>>> +void make_locked_paths_absolute(void)
>>> +{
>>> +     struct lock_file *lk;
>>> +     for (lk = lock_file_list; lk != NULL; lk = lk->next) {
>>> +             if (lk->filename && !is_absolute_path(lk->filename)) {
>>> +                     char *to_free = lk->filename;
>>> +                     lk->filename = xstrdup(absolute_path(lk->filename));
>>> +                     free(to_free);
>>> +             }
>>> +     }
>>> +}
>>
>> I just have to ask, why are we putting relative pathnames in this
>> list to begin with? Why not use an absolute path when taking the
>> lock in all cases? (calling absolute_path() and using the result
>> to take the lock, storing it in the lock_file list, should not be
>> in the critical path, right? Not that I have measured it, of course! :)
> 
> Conservative :) I'm still scared from 044bbbc (Make git_dir a path
> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah
> looking through "grep hold_" I think none of the locks is in critical
> path. absolute_path() can die() if cwd is longer than PATH_MAX (and
> doing this reduces the chances of that happening). But René is adding
> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be
> fine with putting absolute_path() in hold_lock_file_...*

Hmm, yes, thank you for reminding me about 044bbbc. So, yes it could
cause a (small) performance hit and a change in behaviour (die) in
deeply nested working directories. Hmm, OK.

ATB,
Ramsay Jones

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> wrote:
>>> +void make_locked_paths_absolute(void)
>>> +{
>>> +     struct lock_file *lk;
>>> +     for (lk = lock_file_list; lk != NULL; lk = lk->next) {
>>> +             if (lk->filename && !is_absolute_path(lk->filename)) {
>>> +                     char *to_free = lk->filename;
>>> +                     lk->filename = xstrdup(absolute_path(lk->filename));
>>> +                     free(to_free);
>>> +             }
>>> +     }
>>> +}
>>
>> I just have to ask, why are we putting relative pathnames in this
>> list to begin with? Why not use an absolute path when taking the
>> lock in all cases? (calling absolute_path() and using the result
>> to take the lock, storing it in the lock_file list, should not be
>> in the critical path, right? Not that I have measured it, of course! :)
>
> Conservative :) I'm still scared from 044bbbc (Make git_dir a path
> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah
> looking through "grep hold_" I think none of the locks is in critical
> path. absolute_path() can die() if cwd is longer than PATH_MAX (and
> doing this reduces the chances of that happening). But René is adding
> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be
> fine with putting absolute_path() in hold_lock_file_...*

OK, we should center these efforts around the strbuf_getcwd() topic,
basing the other topic on realpath() and this one on it then?

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Duy Nguyen]

On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones
>> <ramsay@ramsay1.demon.co.uk> wrote:
>>>> +void make_locked_paths_absolute(void)
>>>> +{
>>>> +     struct lock_file *lk;
>>>> +     for (lk = lock_file_list; lk != NULL; lk = lk->next) {
>>>> +             if (lk->filename && !is_absolute_path(lk->filename)) {
>>>> +                     char *to_free = lk->filename;
>>>> +                     lk->filename = xstrdup(absolute_path(lk->filename));
>>>> +                     free(to_free);
>>>> +             }
>>>> +     }
>>>> +}
>>>
>>> I just have to ask, why are we putting relative pathnames in this
>>> list to begin with? Why not use an absolute path when taking the
>>> lock in all cases? (calling absolute_path() and using the result
>>> to take the lock, storing it in the lock_file list, should not be
>>> in the critical path, right? Not that I have measured it, of course! :)
>>
>> Conservative :) I'm still scared from 044bbbc (Make git_dir a path
>> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah
>> looking through "grep hold_" I think none of the locks is in critical
>> path. absolute_path() can die() if cwd is longer than PATH_MAX (and
>> doing this reduces the chances of that happening). But René is adding
>> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be
>> fine with putting absolute_path() in hold_lock_file_...*
>
> OK, we should center these efforts around the strbuf_getcwd() topic,
> basing the other topic on realpath() and this one on it then?

OK.
-- 
Duy

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Yue Lin Ho]

Hi:

> 2014-07-23 19:55 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano <gitster@pobox.com>
> wrote:
> > Duy Nguyen <pclouds@gmail.com> writes:
​[snip]​
> > OK, we should center these efforts around the strbuf_getcwd() topic,
> > basing the other topic on realpath() and this one on it then?
> 
> OK.
> --
> Duy

​Excuse me.
How do I trace these patches applied?

I just fetch from https://github.com/gitster/git.git
Then tried to find these patches if it is applied.
(Seems not.)

Then, I took a look at
http://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Jul-2014-04-Tue-22-td7615627.html
seems no related information there.

So, could you please tell me how to trace it?

Thank you. ^_^

Yue Lin Ho
​



--
View this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7616119.html
Sent from the git mailing list archive at Nabble.com.

Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed

[Duy Nguyen]

On Thu, Jul 31, 2014 at 10:01 AM, Yue Lin Ho <yuelinho777@gmail.com> wrote:
> Hi:
> How do I trace these patches applied?

They are not applied yet. I'll needto redo them on top of rs/strbuf-getcwd.
-- 
Duy

Re: [PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Philip Oakley]

From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
> Something extra is, because struct lock_file is usually used as static
> variables in many places. This patch reduces bss section by about 80k
> bytes (or 23%) on Linux.

This didn't scan for me. Perhaps it's the punctuation. Maybe:

Additionally, because the struct lock_file variables are [were] in many
places static, this patch reduces the bss section size by about 80k
bytes (or 23%) on Linux.

Does my tweak reflect your intent?
Philip.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> This helps remove the length check in v1 of the next patch.
>
> cache.h    |  2 +-
> lockfile.c | 56 
> ++++++++++++++++++++++++++++++++------------------------
> 2 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 44aa439..9ecb636 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -554,7 +554,7 @@ struct lock_file {
>  int fd;
>  pid_t owner;
>  char on_list;
> - char filename[PATH_MAX];
> + char *filename;
> };
> #define LOCK_DIE_ON_ERROR 1
> #define LOCK_NODEREF 2
> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..968b28f 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -7,13 +7,19 @@
> static struct lock_file *lock_file_list;
> static const char *alternate_index_output;
>
> +static void clear_filename(struct lock_file *lk)
> +{
> + free(lk->filename);
> + lk->filename = NULL;
> +}
> +
> static void remove_lock_file(void)
> {
>  pid_t me = getpid();
>
>  while (lock_file_list) {
>  if (lock_file_list->owner == me &&
> -     lock_file_list->filename[0]) {
> +     lock_file_list->filename) {
>  if (lock_file_list->fd >= 0)
>  close(lock_file_list->fd);
>  unlink_or_warn(lock_file_list->filename);
> @@ -77,10 +83,16 @@ static char *last_path_elm(char *p)
>  * Always returns p.
>  */
>
> -static char *resolve_symlink(char *p, size_t s)
> +static char *resolve_symlink(const char *in)
> {
> + static char p[PATH_MAX];
> + size_t s = sizeof(p);
>  int depth = MAXDEPTH;
>
> + if (strlen(in) >= sizeof(p))
> + return NULL;
> + strcpy(p, in);
> +
>  while (depth--) {
>  char link[PATH_MAX];
>  int link_len = readlink(p, link, sizeof(link));
> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
>
> static int lock_file(struct lock_file *lk, const char *path, int 
> flags)
> {
> - /*
> - * subtract 5 from size to make sure there's room for adding
> - * ".lock" for the lock file name
> - */
> - static const size_t max_path_len = sizeof(lk->filename) - 5;
> -
> - if (strlen(path) >= max_path_len)
> + int len;
> + if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
>  return -1;
> + len = strlen(path) + 5; /* .lock */
> + lk->filename = xmallocz(len);
>  strcpy(lk->filename, path);
> - if (!(flags & LOCK_NODEREF))
> - resolve_symlink(lk->filename, max_path_len);
>  strcat(lk->filename, ".lock");
>  lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
>  if (0 <= lk->fd) {
> @@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const 
> char *path, int flags)
>       lk->filename);
>  }
>  else
> - lk->filename[0] = 0;
> + clear_filename(lk);
>  return lk->fd;
> }
>
> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
>
> int commit_lock_file(struct lock_file *lk)
> {
> - char result_file[PATH_MAX];
> - size_t i;
> - if (lk->fd >= 0 && close_lock_file(lk))
> + char *result_file;
> + if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
>  return -1;
> - strcpy(result_file, lk->filename);
> - i = strlen(result_file) - 5; /* .lock */
> - result_file[i] = 0;
> - if (rename(lk->filename, result_file))
> + result_file = xmemdupz(lk->filename,
> +        strlen(lk->filename) - 5 /* .lock */);
> + if (rename(lk->filename, result_file)) {
> + free(result_file);
>  return -1;
> - lk->filename[0] = 0;
> + }
> + free(result_file);
> + clear_filename(lk);
>  return 0;
> }
>
> @@ -260,11 +268,11 @@ void set_alternate_index_output(const char 
> *name)
> int commit_locked_index(struct lock_file *lk)
> {
>  if (alternate_index_output) {
> - if (lk->fd >= 0 && close_lock_file(lk))
> + if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
>  return -1;
>  if (rename(lk->filename, alternate_index_output))
>  return -1;
> - lk->filename[0] = 0;
> + clear_filename(lk);
>  return 0;
>  }
>  else
> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
>
> void rollback_lock_file(struct lock_file *lk)
> {
> - if (lk->filename[0]) {
> + if (lk->filename) {
>  if (lk->fd >= 0)
>  close(lk->fd);
>  unlink_or_warn(lk->filename);
>  }
> - lk->filename[0] = 0;
> + clear_filename(lk);
> }
> -- 
> 1.9.1.346.ga2b5940
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Duy Nguyen]

On Sun, Jul 20, 2014 at 7:47 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
>
>> Something extra is, because struct lock_file is usually used as static
>> variables in many places. This patch reduces bss section by about 80k
>> bytes (or 23%) on Linux.
>
>
> This didn't scan for me. Perhaps it's the punctuation. Maybe:
>
> Additionally, because the struct lock_file variables are [were] in many
> places static, this patch reduces the bss section size by about 80k
>
> bytes (or 23%) on Linux.
>
> Does my tweak reflect your intent?

Yes. But I should probably put that below the "---" line. We're not
busybox. 80k is pratically nothing.
-- 
Duy

[PATCH v3 0/3] Keep .lock file paths absolute

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

v3 requires rs/strbuf-getcwd, turns paths to absolute from the
beginning, and kills the last use of PATH_MAX in lockfile.c thanks to
strbuf_readlink().

Nguyễn Thái Ngọc Duy (3):
  lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)
  lockfile.c: remove PATH_MAX limit in resolve_symlink()
  lockfile.c: store absolute path

 cache.h                       |  2 +-
 lockfile.c                    | 95 +++++++++++++++++++------------------------
 t/t2107-update-index-basic.sh | 15 +++++++
 3 files changed, 58 insertions(+), 54 deletions(-)

-- 
2.1.0.rc0.78.gc0d8480

[PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h    |  2 +-
 lockfile.c | 56 ++++++++++++++++++++++++++++++++------------------------
 2 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/cache.h b/cache.h
index cc46be4..0d8dce7 100644
--- a/cache.h
+++ b/cache.h
@@ -539,7 +539,7 @@ struct lock_file {
 	int fd;
 	pid_t owner;
 	char on_list;
-	char filename[PATH_MAX];
+	char *filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..968b28f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -7,13 +7,19 @@
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
 
+static void clear_filename(struct lock_file *lk)
+{
+	free(lk->filename);
+	lk->filename = NULL;
+}
+
 static void remove_lock_file(void)
 {
 	pid_t me = getpid();
 
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0]) {
+		    lock_file_list->filename) {
 			if (lock_file_list->fd >= 0)
 				close(lock_file_list->fd);
 			unlink_or_warn(lock_file_list->filename);
@@ -77,10 +83,16 @@ static char *last_path_elm(char *p)
  * Always returns p.
  */
 
-static char *resolve_symlink(char *p, size_t s)
+static char *resolve_symlink(const char *in)
 {
+	static char p[PATH_MAX];
+	size_t s = sizeof(p);
 	int depth = MAXDEPTH;
 
+	if (strlen(in) >= sizeof(p))
+		return NULL;
+	strcpy(p, in);
+
 	while (depth--) {
 		char link[PATH_MAX];
 		int link_len = readlink(p, link, sizeof(link));
@@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
 
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	/*
-	 * subtract 5 from size to make sure there's room for adding
-	 * ".lock" for the lock file name
-	 */
-	static const size_t max_path_len = sizeof(lk->filename) - 5;
-
-	if (strlen(path) >= max_path_len)
+	int len;
+	if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
 		return -1;
+	len = strlen(path) + 5; /* .lock */
+	lk->filename = xmallocz(len);
 	strcpy(lk->filename, path);
-	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(lk->filename, max_path_len);
 	strcat(lk->filename, ".lock");
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
@@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 				     lk->filename);
 	}
 	else
-		lk->filename[0] = 0;
+		clear_filename(lk);
 	return lk->fd;
 }
 
@@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	char result_file[PATH_MAX];
-	size_t i;
-	if (lk->fd >= 0 && close_lock_file(lk))
+	char *result_file;
+	if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
 		return -1;
-	strcpy(result_file, lk->filename);
-	i = strlen(result_file) - 5; /* .lock */
-	result_file[i] = 0;
-	if (rename(lk->filename, result_file))
+	result_file = xmemdupz(lk->filename,
+			       strlen(lk->filename) - 5 /* .lock */);
+	if (rename(lk->filename, result_file)) {
+		free(result_file);
 		return -1;
-	lk->filename[0] = 0;
+	}
+	free(result_file);
+	clear_filename(lk);
 	return 0;
 }
 
@@ -260,11 +268,11 @@ void set_alternate_index_output(const char *name)
 int commit_locked_index(struct lock_file *lk)
 {
 	if (alternate_index_output) {
-		if (lk->fd >= 0 && close_lock_file(lk))
+		if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
 			return -1;
 		if (rename(lk->filename, alternate_index_output))
 			return -1;
-		lk->filename[0] = 0;
+		clear_filename(lk);
 		return 0;
 	}
 	else
@@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename[0]) {
+	if (lk->filename) {
 		if (lk->fd >= 0)
 			close(lk->fd);
 		unlink_or_warn(lk->filename);
 	}
-	lk->filename[0] = 0;
+	clear_filename(lk);
 }
-- 
2.1.0.rc0.78.gc0d8480

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Junio C Hamano]

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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Somewhat underexplained, given that it seems to add some new
semantics.

> +static void clear_filename(struct lock_file *lk)
> +{
> +	free(lk->filename);
> +	lk->filename = NULL;
> +}

It is good to abstract out lk->filename[0] = '\0', which used to be
the way we say that we are done with the lock.  But I am somewhat
surprised to see that there aren't so many locations that used to
check !!lk->filename[0] to see if we are done with the lock to require
a corresponding wrapper.

>  static void remove_lock_file(void)
>  {
>  	pid_t me = getpid();
>  
>  	while (lock_file_list) {
>  		if (lock_file_list->owner == me &&
> -		    lock_file_list->filename[0]) {
> +		    lock_file_list->filename) {

... and this seems to be the only location?

> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
>  
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
> -	/*
> -	 * subtract 5 from size to make sure there's room for adding
> -	 * ".lock" for the lock file name
> -	 */
> -	static const size_t max_path_len = sizeof(lk->filename) - 5;
> -
> -	if (strlen(path) >= max_path_len)
> +	int len;
> +	if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
>  		return -1;

Somehow I found it unnecessarily denser; had to read it twice before
caffeine kicked in ;-)

> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
>  
>  int commit_lock_file(struct lock_file *lk)
>  {
> -	char result_file[PATH_MAX];
> -	size_t i;
> -	if (lk->fd >= 0 && close_lock_file(lk))
> +	char *result_file;
> +	if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
>  		return -1;

We did not protect against somebody calling this with an already
closed lock, but we now return early without attempting renameing
etc., which is a good change but is not explained.  Was there a
specific code path that you needed this change for?

Also the order of the check is not consistent with how the same
check is done in rollback_lock_file().  The order you use in this
new code (and also in commit_locked_index()) may be better than the
existing order in the rollback code path; we want to see the fd
closed, if it is open, even if lk->filename has already been
cleared.  On the other hand, one could argue that anything such a
broken caller tells this function is suspicious, and we shouldn't
close random file descriptor that is likely not owned by the caller
in the first place.  I dunno.

> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
>  
>  void rollback_lock_file(struct lock_file *lk)
>  {
> -	if (lk->filename[0]) {
> +	if (lk->filename) {
>  		if (lk->fd >= 0)
>  			close(lk->fd);
>  		unlink_or_warn(lk->filename);
>  	}
> -	lk->filename[0] = 0;
> +	clear_filename(lk);
>  }

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> Somewhat underexplained, given that it seems to add some new
> semantics.
>
>> +static void clear_filename(struct lock_file *lk)
>> +{
>> +	free(lk->filename);
>> +	lk->filename = NULL;
>> +}
>
> It is good to abstract out lk->filename[0] = '\0', which used to be
> the way we say that we are done with the lock.  But I am somewhat
> surprised to see that there aren't so many locations that used to
> check !!lk->filename[0] to see if we are done with the lock to require
> a corresponding wrapper.
>
>>  static void remove_lock_file(void)
>>  {
>>  	pid_t me = getpid();
>>  
>>  	while (lock_file_list) {
>>  		if (lock_file_list->owner == me &&
>> -		    lock_file_list->filename[0]) {
>> +		    lock_file_list->filename) {
>
> ... and this seems to be the only location?

While looking at possible fallout of merging this topic to any
branch, I am starting to suspect that it is probably a bad idea for
clear-filename to free lk->filename.  I am wondering if it would be
safer to do:

 - in lock_file(), free lk->filename if it already exists before
   what you do in that function with your series;

 - update "is this lock already held?" check !!lk->filename[0] to
   check for (lk->filename && !!lk->filename[0]);

 - in clear_filename(), clear lk->filename[0] = '\0', but do not
   free lk->filename itself.

Then existing callers that never suspected that lk->filename can be
NULL and thought that it does not need freeing can keep doing the
same thing as before without leaking nor breaking.

If we want to adopt the new world order at once, alternatively, you
can keep the code in this series but then lk->filename needs to be
renamed to something that the current code base has not heard of to
force breakage at the link time for us to notice.

I grepped for 'lk->filename' and checked if the ones in read-cache.c
and refs.c are OK (they seem to be), but that is not a very robust
check.

I dunno.

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Torsten Bögershausen]

On 08/01/2014 07:55 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> Somewhat underexplained, given that it seems to add some new
>> semantics.
>>
>>> +static void clear_filename(struct lock_file *lk)
>>> +{
>>> +	free(lk->filename);
>>> +	lk->filename = NULL;
>>> +}
>> It is good to abstract out lk->filename[0] = '\0', which used to be
>> the way we say that we are done with the lock.  But I am somewhat
>> surprised to see that there aren't so many locations that used to
>> check !!lk->filename[0] to see if we are done with the lock to require
>> a corresponding wrapper.
>>
>>>   static void remove_lock_file(void)
>>>   {
>>>   	pid_t me = getpid();
>>>   
>>>   	while (lock_file_list) {
>>>   		if (lock_file_list->owner == me &&
>>> -		    lock_file_list->filename[0]) {
>>> +		    lock_file_list->filename) {
>> ... and this seems to be the only location?
> While looking at possible fallout of merging this topic to any
> branch, I am starting to suspect that it is probably a bad idea for
> clear-filename to free lk->filename.  I am wondering if it would be
> safer to do:
>
>   - in lock_file(), free lk->filename if it already exists before
>     what you do in that function with your series;
>
>   - update "is this lock already held?" check !!lk->filename[0] to
>     check for (lk->filename && !!lk->filename[0]);
>
>   - in clear_filename(), clear lk->filename[0] = '\0', but do not
>     free lk->filename itself.
>
> Then existing callers that never suspected that lk->filename can be
> NULL and thought that it does not need freeing can keep doing the
> same thing as before without leaking nor breaking.
>
> If we want to adopt the new world order at once, alternatively, you
> can keep the code in this series but then lk->filename needs to be
> renamed to something that the current code base has not heard of to
> force breakage at the link time for us to notice.
>
> I grepped for 'lk->filename' and checked if the ones in read-cache.c
> and refs.c are OK (they seem to be), but that is not a very robust
> check.
>
> I dunno.

My first impression reading this patch was to rename
clear_filename() into free_and_clear_filename() or better free_filename(),
but I never pressed the send button ;-)

Reading the discussion above makes me wonder if lk->filename may be replaced by a strbuf
some day, and in this case clear_filename() will become reset_filenmae() ?

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Duy Nguyen]

On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 08/01/2014 07:55 PM, Junio C Hamano wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>>
>>> Somewhat underexplained, given that it seems to add some new
>>> semantics.
>>>
>>>> +static void clear_filename(struct lock_file *lk)
>>>> +{
>>>> +       free(lk->filename);
>>>> +       lk->filename = NULL;
>>>> +}
>>>
>>> It is good to abstract out lk->filename[0] = '\0', which used to be
>>> the way we say that we are done with the lock.  But I am somewhat
>>> surprised to see that there aren't so many locations that used to
>>> check !!lk->filename[0] to see if we are done with the lock to require
>>> a corresponding wrapper.
>>>
>>>>   static void remove_lock_file(void)
>>>>   {
>>>>         pid_t me = getpid();
>>>>         while (lock_file_list) {
>>>>                 if (lock_file_list->owner == me &&
>>>> -                   lock_file_list->filename[0]) {
>>>> +                   lock_file_list->filename) {
>>>
>>> ... and this seems to be the only location?
>>
>> While looking at possible fallout of merging this topic to any
>> branch, I am starting to suspect that it is probably a bad idea for
>> clear-filename to free lk->filename.  I am wondering if it would be
>> safer to do:
>>
>>   - in lock_file(), free lk->filename if it already exists before
>>     what you do in that function with your series;
>>
>>   - update "is this lock already held?" check !!lk->filename[0] to
>>     check for (lk->filename && !!lk->filename[0]);
>>
>>   - in clear_filename(), clear lk->filename[0] = '\0', but do not
>>     free lk->filename itself.
>>
>> Then existing callers that never suspected that lk->filename can be
>> NULL and thought that it does not need freeing can keep doing the
>> same thing as before without leaking nor breaking.
>>
>> If we want to adopt the new world order at once, alternatively, you
>> can keep the code in this series but then lk->filename needs to be
>> renamed to something that the current code base has not heard of to
>> force breakage at the link time for us to notice.
>>
>> I grepped for 'lk->filename' and checked if the ones in read-cache.c
>> and refs.c are OK (they seem to be), but that is not a very robust
>> check.
>>
>> I dunno.
>
>
> My first impression reading this patch was to rename
> clear_filename() into free_and_clear_filename() or better free_filename(),
> but I never pressed the send button ;-)
>
> Reading the discussion above makes me wonder if lk->filename may be replaced
> by a strbuf
> some day, and in this case clear_filename() will become reset_filenmae() ?

I didn't realize Mike is making a lot more changes in lockfile.c, part
of that is converting lk->filename to use strbuf [1]. Perhaps I should
just withdraw this series, wait until Mike's series is merged, then
redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.

[1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232
-- 
Duy

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I didn't realize Mike is making a lot more changes in lockfile.c, part
> of that is converting lk->filename to use strbuf [1]. Perhaps I should
> just withdraw this series, wait until Mike's series is merged, then
> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.

During the pre-release freeze I would like to see new topics be
calmer ;-)  Serializing or not, inter-developer coordination is
always very much appreciated.

Thanks.

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Michael Haggerty]

On 08/04/2014 03:13 AM, Duy Nguyen wrote:
> On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> [...]
>> My first impression reading this patch was to rename
>> clear_filename() into free_and_clear_filename() or better free_filename(),
>> but I never pressed the send button ;-)
>>
>> Reading the discussion above makes me wonder if lk->filename may be replaced
>> by a strbuf
>> some day, and in this case clear_filename() will become reset_filenmae() ?
> 
> I didn't realize Mike is making a lot more changes in lockfile.c, part
> of that is converting lk->filename to use strbuf [1]. Perhaps I should
> just withdraw this series, wait until Mike's series is merged, then
> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232

I've neglected my patch series for ages (sorry!)  The last round of
review pointed out a couple of places where lock_file objects were still
being left in undefined states, and since then it also bit-rotted.

Over the past few days I re-rolled the patch series and fixed some more
code paths.  I still want to check it over before submitting it to the
list, but if you are interested the current version is here [1].

Duy, I'll try to look at your patches, but probably won't get to it
until next week when I return from vacation.

Michael

[1] https://github.com/mhagger/git branch "lock-correctness"

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Yue Lin Ho]

Hi Michael:

> On 08/04/2014 03:13 AM, Duy Nguyen wrote:
>
>> On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <[hidden email]>
>> wrote: 
>> [...] 
>>> My first impression reading this patch was to rename 
>>> clear_filename() into free_and_clear_filename() or better
>>> free_filename(), 
>>> but I never pressed the send button ;-) 
>>> 
>>> Reading the discussion above makes me wonder if lk->filename may be
>>> replaced 
>>> by a strbuf 
>>> some day, and in this case clear_filename() will become reset_filenmae()
>>> ? 
>> 
>> I didn't realize Mike is making a lot more changes in lockfile.c, part 
>> of that is converting lk->filename to use strbuf [1]. Perhaps I should 
>> just withdraw this series, wait until Mike's series is merged, then 
>> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series. 
>> 
>> [1]
>> http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232
> 
> I've neglected my patch series for ages (sorry!)  The last round of 
> review pointed out a couple of places where lock_file objects were still 
> being left in undefined states, and since then it also bit-rotted. 
> 
> Over the past few days I re-rolled the patch series and fixed some more 
> code paths.  I still want to check it over before submitting it to the 
> list, but if you are interested the current version is here [1]. 
> 
> Duy, I'll try to look at your patches, but probably won't get to it 
> until next week when I return from vacation. 
> 
> Michael 
> 
> [1] https://github.com/mhagger/git branch "lock-correctness" 

​I am tracing the lock path issue.
(http://git.661346.n2.nabble.com/git-update-index-not-delete-lock-file-when-using-different-worktree-td7615300.html)

and I see mh/lockfile part in 
http://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Sep-2014-01-Tue-2-td7617955.html
as following:

* mh/lockfile (2014-04-15) 25 commits
 . trim_last_path_elm(): replace last_path_elm()
 . resolve_symlink(): take a strbuf parameter
 . resolve_symlink(): use a strbuf for internal scratch space
 . change lock_file::filename into a strbuf
 . commit_lock_file(): use a strbuf to manage temporary space
 . try_merge_strategy(): use a statically-allocated lock_file object
 . try_merge_strategy(): remove redundant lock_file allocation
 . struct lock_file: declare some fields volatile
 . lockfile: avoid transitory invalid states
 . commit_lock_file(): die() if called for unlocked lockfile object
 . commit_lock_file(): inline temporary variable
 . remove_lock_file(): call rollback_lock_file()
 . lock_file(): exit early if lockfile cannot be opened
 . write_packed_entry_fn(): convert cb_data into a (const int *)
 . prepare_index(): declare return value to be (const char *)
 . delete_ref_loose(): don't muck around in the lock_file's filename
 . cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
 . lockfile.c: document the various states of lock_file objects
 . lock_file(): always add lock_file object to lock_file_list
 . hold_lock_file_for_append(): release lock on errors
 . lockfile: unlock file if lockfile permissions cannot be adjusted
 . rollback_lock_file(): set fd to -1
 . rollback_lock_file(): do not clear filename redundantly
 . api-lockfile: expand the documentation
 . unable_to_lock_die(): rename function from unable_to_lock_index_die()

 Expecting a reroll.

So, do you have any plan about mh/lockfile and the lock path issue?

Thank you.​ ^_^

Yue Lin Ho



--
View this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7617967.html
Sent from the git mailing list archive at Nabble.com.

Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Junio C Hamano]

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

> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..968b28f 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -7,13 +7,19 @@
>  static struct lock_file *lock_file_list;
>  static const char *alternate_index_output;
>  
> +static void clear_filename(struct lock_file *lk)
> +{
> +	free(lk->filename);
> +	lk->filename = NULL;
> +}
> +

Given that you move commit_locked_index(), which you need to use
this function in, to read-cache.c in your own nd/split-index series,
this will need to be exposed to the wider world, and at that point,
its name will turn out to be too generic.

[PATCH v3 2/3] lockfile.c: remove PATH_MAX limit in resolve_symlink()

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 lockfile.c | 47 ++++++++++++++---------------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 968b28f..154915f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -85,52 +85,33 @@ static char *last_path_elm(char *p)
 
 static char *resolve_symlink(const char *in)
 {
-	static char p[PATH_MAX];
-	size_t s = sizeof(p);
+	static struct strbuf p = STRBUF_INIT;
+	struct strbuf link = STRBUF_INIT;
 	int depth = MAXDEPTH;
 
-	if (strlen(in) >= sizeof(p))
-		return NULL;
-	strcpy(p, in);
+	strbuf_reset(&p);
+	strbuf_addstr(&p, in);
 
 	while (depth--) {
-		char link[PATH_MAX];
-		int link_len = readlink(p, link, sizeof(link));
-		if (link_len < 0) {
-			/* not a symlink anymore */
-			return p;
-		}
-		else if (link_len < sizeof(link))
-			/* readlink() never null-terminates */
-			link[link_len] = '\0';
-		else {
-			warning("%s: symlink too long", p);
-			return p;
-		}
+		if (strbuf_readlink(&link, p.buf, 0) < 0)
+			break;	/* not a symlink anymore */
 
-		if (is_absolute_path(link)) {
+		if (is_absolute_path(link.buf)) {
 			/* absolute path simply replaces p */
-			if (link_len < s)
-				strcpy(p, link);
-			else {
-				warning("%s: symlink too long", p);
-				return p;
-			}
+			strbuf_reset(&p);
+			strbuf_addbuf(&p, &link);
 		} else {
 			/*
 			 * link is a relative path, so I must replace the
 			 * last element of p with it.
 			 */
-			char *r = (char *)last_path_elm(p);
-			if (r - p + link_len < s)
-				strcpy(r, link);
-			else {
-				warning("%s: symlink too long", p);
-				return p;
-			}
+			char *r = (char *)last_path_elm(p.buf);
+			strbuf_setlen(&p, r - p.buf);
+			strbuf_addbuf(&p, &link);
 		}
 	}
-	return p;
+	strbuf_release(&link);
+	return p.buf;
 }
 
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v3 3/3] lockfile.c: store absolute path

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

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>
---
 lockfile.c                    | 10 +++++-----
 t/t2107-update-index-basic.sh | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 154915f..0aa70a5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -117,13 +117,13 @@ static char *resolve_symlink(const char *in)
 
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	int len;
+	struct strbuf sb = STRBUF_INIT;
+
 	if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
 		return -1;
-	len = strlen(path) + 5; /* .lock */
-	lk->filename = xmallocz(len);
-	strcpy(lk->filename, path);
-	strcat(lk->filename, ".lock");
+	strbuf_add_absolute_path(&sb, path);
+	strbuf_addstr(&sb, ".lock");
+	lk->filename = strbuf_detach(&sb, NULL);
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
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.0.rc0.78.gc0d8480