Re: git-config: replaces ~/.gitconfig symlink with real file

4 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: git-config: replaces ~/.gitconfig symlink with real file

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:21

"Bradford Smith" [off-list ref] writes:
...  So, I guess I need to add a GIT_CONFIG_HOME
environment variable.
I suspect that is going down a wrong path.

We use the sequence:

	fd = creat("temporary location");
        write(fd, ...);
        close(fd);
        rename("temporary location", "final location");

in quite a lot of codepaths.  I think they can be factored out,
to take the "final location" (and perhaps a suggested temporary
directory) as an parameter, and that code can check that "final
location" is a symlink to somewhere else and create the
temporary next to the target file.

[PATCH 1/2] resolve symlinks when creating lockfiles

From: Bradford C. Smith <hidden>
Date: 2016-06-15 22:43:23

From: Bradford C. Smith <redacted>

Without this fix, the lockfile code will replace a symlink with a real file.

Signed-off-by: "Bradford C. Smith" <redacted>
---
 lockfile.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 86 insertions(+), 1 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index fb8f13b..4c35224 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -25,10 +25,95 @@ static void remove_lock_file_on_signal(int signo)
 	raise(signo);
 }
 
+/**
+ * p = absolute or relative path name
+ *
+ * Return a pointer into p showing the beginning of the last path name
+ * element.  If p is empty or the root directory ("/"), just return p.
+ */
+static char * last_path_elm(char * p)
+{
+	int	p_len = strlen(p);
+	char *	r;
+
+	if (p_len < 1) return p;
+	/* r points to last non-null character in p */
+	r = p + p_len - 1;
+	/* first skip any trailing slashes */
+	while (*r == '/' && r > p) r--;
+	/* then go back to the first non-slash */
+	while (r > p && *(r-1) != '/') r--;
+	return r;
+}
+
+/**
+ * p = char array containing path to existing file or symlink
+ * s = size of p
+ *
+ * If p indicates a valid symlink to an existing file, overwrite p with
+ * the path to the real file.  Otherwise, leave p unmodified.
+ *
+ * Always returns p in any case.
+ *
+ * NOTE: This is a best-effort routine.  It will give no indication of
+ * failure if it is unable to fully resolve p.  However, it is
+ * guaranteed to leave p in one of the following states if there isn't
+ * enough room in p or some other failure occurs:
+ *
+ * 1. unmodified
+ *      OR
+ * 2. path to a different symlink in a chain that eventually leads to a
+ *    real file or directory.
+ */
+static char * resolve_symlink(char * p, size_t s)
+{
+	struct stat st;
+	char link[PATH_MAX];
+	int link_len;
+
+	/* To avoid an infinite loop of symlinks, try a normal stat()
+	 * first.  This will fail if p is a symlink that cannot be
+	 * resolved, so we won't waste our time following a bad link. */
+	if (stat(p, &st)) return p;
+	/* if I can stat() the file, I sure ought to be able to lstat()
+	 * it, but if something bizarre happens, just return p.  */
+	if (lstat(p, &st)) return p;
+	/* if not a link, return p unmodified */
+	if (!S_ISLNK(st.st_mode)) return p;
+	link_len = st.st_size;
+	/* link is too big, so just return p */
+	if (link_len >= sizeof(link)) return p;
+	/* fail if readlink fails, and just return p */
+	if (link_len != readlink(p, link, sizeof(link))) return p;
+	/* readlink never null-terminates */
+	link[link_len] = '\0';
+	if (link[0] == '/') {
+		/* absolute path simply replaces p */
+		/* fail if link won't fit in p */
+		if (link_len >= s) return p;
+		strcpy(p, link);
+	} else {
+		/* link is relative path, so we must replace the last
+		 * element of p with it. */
+		char * r = last_path_elm(p);
+		/* make sure there's room in p for us to replace the
+		 * last element with the link contents */
+		if (r - p + link_len >= s) return p;
+		strcpy(r, link);
+	}
+	/* try again in case we've resolved to another symlink */
+	return resolve_symlink(p, s);
+}
+
 static int lock_file(struct lock_file *lk, const char *path)
 {
 	int fd;
-	sprintf(lk->filename, "%s.lock", path);
+	if (strlen(path) >= sizeof(lk->filename)) return -1;
+	strcpy(lk->filename, path);
+	/* subtract 5 from size to make sure there's room for adding
+	 * ".lock" for the lock file name */
+	resolve_symlink(lk->filename, sizeof(lk->filename)-5);
+	strcat(lk->filename, ".lock");
 	fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= fd) {
 		if (!lock_file_list) {
-- 
1.5.3.rc2.30.g1c06-dirty

[PATCH 0/2] git-config should not replace symlink

From: Bradford C. Smith <hidden>
Date: 2016-06-15 22:43:23

These patches fix a problem that caused git-config to replace my
~/.gitconfig symlink with a real file.

[PATCH 1/2] resolve symlinks when creating lockfiles
[PATCH 2/2] use lockfile.c routines in git_commit_set_multivar()

[PATCH 2/2] use lockfile.c routines in git_commit_set_multivar()

From: Bradford C. Smith <hidden>
Date: 2016-06-15 22:43:23

From: Bradford C. Smith <redacted>

Changed git_commit_set_multivar() to use the routines provided by
lockfile.c to reduce code duplication and ensure consistent behavior.

Signed-off-by: "Bradford C. Smith" <redacted>
---
 config.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/config.c b/config.c
index f89a611..9101de9 100644
--- a/config.c
+++ b/config.c
@@ -715,7 +715,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	int fd = -1, in_fd;
 	int ret;
 	char* config_filename;
-	char* lock_file;
+	struct lock_file *lock = NULL;
 	const char* last_dot = strrchr(key, '.');
 
 	config_filename = getenv(CONFIG_ENVIRONMENT);
@@ -725,7 +725,6 @@ int git_config_set_multivar(const char* key, const char* value,
 			config_filename  = git_path("config");
 	}
 	config_filename = xstrdup(config_filename);
-	lock_file = xstrdup(mkpath("%s.lock", config_filename));
 
 	/*
 	 * Since "key" actually contains the section name and the real
@@ -770,11 +769,12 @@ int git_config_set_multivar(const char* key, const char* value,
 	store.key[i] = 0;
 
 	/*
-	 * The lock_file serves a purpose in addition to locking: the new
+	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	fd = open(lock_file, O_WRONLY | O_CREAT | O_EXCL, 0666);
-	if (fd < 0 || adjust_shared_perm(lock_file)) {
+	lock = xcalloc(sizeof(struct lock_file), 1);
+	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
 		free(store.key);
 		ret = -1;
@@ -914,25 +914,29 @@ int git_config_set_multivar(const char* key, const char* value,
 				goto write_err_out;
 
 		munmap(contents, contents_sz);
-		unlink(config_filename);
 	}
 
-	if (rename(lock_file, config_filename) < 0) {
-		fprintf(stderr, "Could not rename the lock file?\n");
+	if (close(fd) || commit_lock_file(lock) < 0) {
+		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
+	/* fd is closed, so don't try to close it below. */
+	fd = -1;
+	/* lock is committed, so don't try to roll it back below.
+	 * NOTE: Since lockfile.c keeps a linked list of all created
+	 * lock files, it isn't safe to free(lock).  It's better to just
+	 * leave it hanging around. */
+	lock = NULL;
 	ret = 0;
 
 out_free:
 	if (0 <= fd)
 		close(fd);
+	if (lock)
+		rollback_lock_file(lock);
 	free(config_filename);
-	if (lock_file) {
-		unlink(lock_file);
-		free(lock_file);
-	}
 	return ret;
 
 write_err_out:
-- 
1.5.3.rc2.30.g1c06-dirty
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help