From 5d5a7a67384ad03007eea1f365ee255c02a40fa3 Mon Sep 17 00:00:00 2001 From: "Bradford C. Smith" Date: Thu, 26 Jul 2007 13:34:14 -0400 Subject: [PATCH 1/3] fully resolve symlinks when creating lockfiles Make the code for resolving symlinks in lockfile.c more robust as follows: 1. Handle relative symlinks 2. recursively resolve symlink chains up to 5 [jc: removed lstat/stat calls to do things stupid way] Signed-off-by: Bradford C. Smith Signed-off-by: Junio C Hamano --- lockfile.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 13 deletions(-) diff --git a/lockfile.c b/lockfile.c index 9202472498..9a1f64d8d7 100644 --- a/lockfile.c +++ b/lockfile.c @@ -25,23 +25,111 @@ 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) +{ + /* r starts pointing to null at the end of the string */ + char *r = strchr(p, '\0'); + + if (r == p) + return p; /* just return empty string */ + + r--; /* back up to last non-null character */ + + /* back up past trailing slashes, if any */ + while (r > p && *r == '/') + r--; + + /* + * then go backwards until I hit a slash, or the beginning of + * the string + */ + while (r > p && *(r-1) != '/') + r--; + return r; +} + + +/* We allow "recursive" symbolic links. Only within reason, though */ +#define MAXDEPTH 5 + +/* + * p = path that may be a symlink + * s = full size of p + * + * If p is a symlink, attempt to overwrite p with a path to the real + * file or directory (which may or may not exist), following a chain of + * symlinks if necessary. Otherwise, leave p unmodified. + * + * This is a best-effort routine. If an error occurs, p will either be + * left unmodified or will name a different symlink in a symlink chain + * that started with p's initial contents. + * + * Always returns p. + */ + +static char *resolve_symlink(char *p, size_t s) +{ + int depth = MAXDEPTH; + + 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 (link[0] == '/') { + /* absolute path simply replaces p */ + if (link_len < s) + strcpy(p, link); + else { + warning("%s: symlink too long", p); + return p; + } + } 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; + } + } + } + return p; +} + + static int lock_file(struct lock_file *lk, const char *path) { int fd; - struct stat st; - if ((!lstat(path, &st)) && S_ISLNK(st.st_mode)) { - ssize_t sz; - static char target[PATH_MAX]; - sz = readlink(path, target, sizeof(target)); - if (sz < 0) - warning("Cannot readlink %s", path); - else if (target[0] != '/') - warning("Cannot lock target of relative symlink %s", path); - else - path = target; - } - 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) { From 6cbf973c9a6f06e9c45b3f6a76f0ceae8e7fd291 Mon Sep 17 00:00:00 2001 From: "Bradford C. Smith" Date: Thu, 26 Jul 2007 12:55:28 -0400 Subject: [PATCH 2/3] use lockfile.c routines in git_commit_set_multivar() 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 Signed-off-by: Junio C Hamano --- config.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/config.c b/config.c index f89a611915..dd2de6e38e 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,31 @@ 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_file structures, 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: From 65a5a21d024373674d6068a03c71558035775e0c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 26 Jul 2007 22:13:12 -0700 Subject: [PATCH 3/3] Add test for symlinked configuration file updates. Signed-off-by: Junio C Hamano --- t/t1300-repo-config.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 1c43cc333d..187ca2df5c 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -595,4 +595,19 @@ echo >>result test_expect_success '--null --get-regexp' 'cmp result expect' +test_expect_success 'symlinked configuration' ' + + ln -s notyet myconfig && + GIT_CONFIG=myconfig git config test.frotz nitfol && + test -h myconfig && + test -f notyet && + test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol && + GIT_CONFIG=myconfig git config test.xyzzy rezrov && + test -h myconfig && + test -f notyet && + test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol && + test "z$(GIT_CONFIG=notyet git config test.xyzzy)" = zrezrov + +' + test_done