# git-config: replaces ~/.gitconfig symlink with real file 21 messages from 2007-07-15 to 2007-07-27. Participants: Bradford Smith, Johannes Schindelin, Nikolai Weibull, Junio C Hamano, Catalin Marinas, Matthieu Moy, Fredrik Tolf, Bradford C. Smith, Morten Welinder. Thread: https://gitlist.dev/t/9049 ## Bradford Smith, 2007-07-15 21:27 Subject: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/f158199e0707151427h52da3e38rae3be6e44e27e918%40mail.gmail.com ``` Since the number of dot-files and dot-directories that I have in my home directory these days is somewhat overwhelming, I like to keep those I directly edit all together in an ~/etc directory so I can easily back them up and/or copy them in bulk to new accounts. So, several of my home dot-files are just symlinks to something in ~/etc, including ~/.gitconfig. However, when I tried running 'git-config --global color.diff auto' today, it removed my symlink and replaced it with a real file. This left me briefly a bit confused when the changes I had made didn't show up in ~/etc/gitconfig, but git-config reported them anyway. If I were to fix this, I'd be tempted to use realpath(3) to follow the symlink, but I don't think it's very reliably available cross-platform. Certainly, it isn't used anywhere in the current git code. Can anyone suggest a more portable fix? Thanks, Bradford ``` ## Johannes Schindelin, 2007-07-15 23:30 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0707160029120.14781%40racer.site In-Reply-To: ``` Hi, On Sun, 15 Jul 2007, Bradford Smith wrote: > If I were to fix this, I'd be tempted to use realpath(3) to follow the > symlink, but I don't think it's very reliably available cross-platform. > Certainly, it isn't used anywhere in the current git code. Can anyone > suggest a more portable fix? I'd use readlink(2) and test for EINVAL to fall back to the current behaviour. Hth, Dscho ``` ## Nikolai Weibull, 2007-07-16 09:37 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/dbfc82860707160237v6772b5b8o541f2045ccd824d5%40mail.gmail.com In-Reply-To: ``` On 7/15/07, Bradford Smith wrote: > Since the number of dot-files and dot-directories that I have in my > home directory these days is somewhat overwhelming, I like to keep > those I directly edit all together in an ~/etc directory so I can > easily back them up and/or copy them in bulk to new accounts. So, > several of my home dot-files are just symlinks to something in ~/etc, > including ~/.gitconfig. How about adding an environment variable telling Git where to find user-global .gitconfig instead? nikolai ``` ## Bradford Smith, 2007-07-16 11:33 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/f158199e0707160433v27fe7073w9c550712c41c32e8%40mail.gmail.com In-Reply-To: ``` On 7/16/07, Nikolai Weibull wrote: > On 7/15/07, Bradford Smith wrote: > > Since the number of dot-files and dot-directories that I have in my > > home directory these days is somewhat overwhelming, I like to keep > > those I directly edit all together in an ~/etc directory so I can > > easily back them up and/or copy them in bulk to new accounts. So, > > several of my home dot-files are just symlinks to something in ~/etc, > > including ~/.gitconfig. > > How about adding an environment variable telling Git where to find > user-global .gitconfig instead? > > home directory these days is somewhat overwhelming, I like to keep > > those I directly edit all together in an ~/etc directory so I can > > easily back them up and/or copy them in bulk to new accounts. So, > > several of my home dot-files are just symlinks to something in ~/etc, > > including ~/.gitconfig. > > How about adding an environment variable telling Git where to find > user-global .gitconfig instead? Thanks for suggesting that. Actually, by looking at the code I discovered I could use the environment variable GIT_CONFIG to specify where the configuration file is, and I have already changed my setup to use this. Unfortunately, I found the documentation for this variable in git-config(1) confusing or I would have used it before. If I get the chance, I'll submit a patch for git-config.txt, and maybe for git.txt as well, since it lists lots of other environment variables but not GIT_CONFIG or GIT_CONFIG_LOCAL. Thanks, Bradford ``` ## Bradford Smith, 2007-07-16 13:26 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/f158199e0707160626j1025ab2cp3339ca6ab91d9af0%40mail.gmail.com In-Reply-To: ``` On 7/16/07, Bradford Smith wrote: > On 7/16/07, Nikolai Weibull wrote: > > On 7/15/07, Bradford Smith wrote: > > > Since the number of dot-files and dot-directories that I have in my > > > home directory these days is somewhat overwhelming, I like to keep > > > those I directly edit all together in an ~/etc directory so I can > > > easily back them up and/or copy them in bulk to new accounts. So, > > > several of my home dot-files are just symlinks to something in ~/etc, > > > including ~/.gitconfig. > > > > How about adding an environment variable telling Git where to find > > user-global .gitconfig instead? > > > home directory these days is somewhat overwhelming, I like to keep > > > those I directly edit all together in an ~/etc directory so I can > > > easily back them up and/or copy them in bulk to new accounts. So, > > > several of my home dot-files are just symlinks to something in ~/etc, > > > including ~/.gitconfig. > > > > How about adding an environment variable telling Git where to find > > user-global .gitconfig instead? > > Thanks for suggesting that. > > Actually, by looking at the code I discovered I could use the > environment variable GIT_CONFIG to specify where the configuration > file is, and I have already changed my setup to use this. > Unfortunately, I found the documentation for this variable in > git-config(1) confusing or I would have used it before. If I get the > chance, I'll submit a patch for git-config.txt, and maybe for git.txt > as well, since it lists lots of other environment variables but not > GIT_CONFIG or GIT_CONFIG_LOCAL. > > Thanks, > > Bradford > Drat! The documentation wasn't as wrong as I had hoped. If I set GIT_CONFIG, git will ignore $(prefix)/etc/gitconfig and ~/.git/config, which isn't what I want. So, I guess I need to add a GIT_CONFIG_HOME environment variable. If I get that done, I'll send a patch to the list including doc updates. Of course, if someone else wants to do it first, I won't complain. B') Thanks, Bradford ``` ## Junio C Hamano, 2007-07-16 22:46 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: <7vps2s2chy.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vps2s2chy.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` "Bradford Smith" 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. ``` ## Catalin Marinas, 2007-07-17 13:39 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/tnx4pk39mju.fsf%40arm.com In-Reply-To: ``` "Bradford Smith" wrote: > However, when I tried running 'git-config --global color.diff auto' > today, it removed my symlink and replaced it with a real file. This > left me briefly a bit confused when the changes I had made didn't show > up in ~/etc/gitconfig, but git-config reported them anyway. Another problem I have with 'git config --global' is that it changes the access permission bits of ~/.gitconfig. Since I use the same file to store global StGIT configuration like SMTP username and password, I'd like to make its access 0600 but it always goes back to 0644 after 'git config --global'. Maybe fixing the symlink case would solve my problem as well. Thanks. -- Catalin ``` ## Johannes Schindelin, 2007-07-17 13:56 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0707170834040.14781%40racer.site In-Reply-To: ``` Hi, On Mon, 16 Jul 2007, Bradford Smith wrote: > So, I guess I need to add a GIT_CONFIG_HOME environment variable. If I > get that done, I'll send a patch to the list including doc updates. Alternatively, you could actually not ignore my hint at readlink(2) and have a proper fix, instead of playing games with environment variables. Hth, Dscho ``` ## Matthieu Moy, 2007-07-17 14:27 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/vpqbqebt8ak.fsf%40bauges.imag.fr In-Reply-To: ``` Johannes Schindelin writes: > Hi, > > On Mon, 16 Jul 2007, Bradford Smith wrote: > >> So, I guess I need to add a GIT_CONFIG_HOME environment variable. If I >> get that done, I'll send a patch to the list including doc updates. > > Alternatively, you could actually not ignore my hint at readlink(2) and > have a proper fix, instead of playing games with environment variables. I second that. Using an environment variable means having a configuration which is about git in my shell's config file, and that's a source of a lot of troubles. Murphy's law implies that one day, the environment variable won't be set properly (because you changed your shell, because you launch git from something which isn't a shell, because you logged-in in a way that didn't read the config file in which the variable was set, ...). I can do with it, like many other software require an environment variable, but I find the symlink trick much more robust. -- Matthieu ``` ## Johannes Schindelin, 2007-07-17 16:09 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0707171708210.14781%40racer.site In-Reply-To: ``` Hi, On Tue, 17 Jul 2007, Catalin Marinas wrote: > "Bradford Smith" wrote: > > However, when I tried running 'git-config --global color.diff auto' > > today, it removed my symlink and replaced it with a real file. This > > left me briefly a bit confused when the changes I had made didn't show > > up in ~/etc/gitconfig, but git-config reported them anyway. > > Another problem I have with 'git config --global' is that it changes > the access permission bits of ~/.gitconfig. Since I use the same file > to store global StGIT configuration like SMTP username and password, > I'd like to make its access 0600 but it always goes back to 0644 after > 'git config --global'. > > Maybe fixing the symlink case would solve my problem as well. More likely not. The way to solve it would be to follow the link if the target path is one. As such, the _file_ would be rewritten. So your problem is unrelated, and would need a separate fix. Ciao, Dscho ``` ## Fredrik Tolf, 2007-07-17 20:35 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/m3wswyojj2.fsf%40pc7.dolda2000.com In-Reply-To: ``` Johannes Schindelin writes: > Hi, > > On Mon, 16 Jul 2007, Bradford Smith wrote: > >> So, I guess I need to add a GIT_CONFIG_HOME environment variable. If I >> get that done, I'll send a patch to the list including doc updates. > > Alternatively, you could actually not ignore my hint at readlink(2) and > have a proper fix, instead of playing games with environment variables. Wouldn't it be nicer to avoid a lot of the complexity in checking symlinks, environment variables and what not, and just overwrite the file in place (with open(..., O_TRUNC | O_CREAT))? Does it happen terribly often that git-config crashes in the middle and leaves the file broken? Fredrik Tolf ``` ## Johannes Schindelin, 2007-07-17 20:48 Subject: Re: git-config: replaces ~/.gitconfig symlink with real file Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0707172145590.14781%40racer.site In-Reply-To: ``` Hi, On Tue, 17 Jul 2007, Fredrik Tolf wrote: > Johannes Schindelin writes: > > > On Mon, 16 Jul 2007, Bradford Smith wrote: > > > >> So, I guess I need to add a GIT_CONFIG_HOME environment variable. If I > >> get that done, I'll send a patch to the list including doc updates. > > > > Alternatively, you could actually not ignore my hint at readlink(2) and > > have a proper fix, instead of playing games with environment variables. > > Wouldn't it be nicer to avoid a lot of the complexity in checking > symlinks, environment variables and what not, and just overwrite the > file in place (with open(..., O_TRUNC | O_CREAT))? Does it happen > terribly often that git-config crashes in the middle and leaves the file > broken? No, it does not. But when it does, I am not only annoyed. I am PISSED! The way we do it is the only safe way to do it, and I gladly spend some extra cycles for that. Too often, a small hard disk glitch (or just an empty laptop battery!) took some important data into the void. Too often, I _cursed_ at the machine, even if it was the programmers' fault. Ciao, Dscho ``` ## Bradford C. Smith, 2007-07-25 16:49 Subject: [PATCH 0/2] git-config should not replace symlink Message-ID: <11853821932079-git-send-email-bradford.carl.smith@gmail.com> URL: https://gitlist.dev/e/11853821932079-git-send-email-bradford.carl.smith%40gmail.com In-Reply-To: <7vps2s2chy.fsf@assigned-by-dhcp.cox.net> ``` 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() ``` ## Bradford C. Smith, 2007-07-25 16:49 Subject: [PATCH 1/2] resolve symlinks when creating lockfiles Message-ID: <11853821951367-git-send-email-bradford.carl.smith@gmail.com> URL: https://gitlist.dev/e/11853821951367-git-send-email-bradford.carl.smith%40gmail.com In-Reply-To: <11853821932079-git-send-email-bradford.carl.smith@gmail.com> ``` From: Bradford C. Smith Without this fix, the lockfile code will replace a symlink with a real file. Signed-off-by: "Bradford C. Smith" --- 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 ``` ## Bradford C. Smith, 2007-07-25 16:49 Subject: [PATCH 2/2] use lockfile.c routines in git_commit_set_multivar() Message-ID: <11853821962210-git-send-email-bradford.carl.smith@gmail.com> URL: https://gitlist.dev/e/11853821962210-git-send-email-bradford.carl.smith%40gmail.com In-Reply-To: <11853821951367-git-send-email-bradford.carl.smith@gmail.com> ``` From: Bradford C. Smith 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" --- 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 ``` ## Junio C Hamano, 2007-07-25 23:35 Subject: Re: [PATCH 1/2] resolve symlinks when creating lockfiles Message-ID: <7vbqe0cazy.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vbqe0cazy.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11853821951367-git-send-email-bradford.carl.smith@gmail.com> ``` This probably is going in the right direction, but the code is too densely formatted and unreviewable. Please imitate the layout convention of the other parts of the code. > +/** > + * 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. > + */ /* * multi-line comments look like this without the extra * asterisk at the beginning of the first line. */ > +static char * last_path_elm(char * p) char *last_path_elem(char *p) > +{ > + int p_len = strlen(p); > + char * r; > + > + if (p_len < 1) return p; char *r; int p_len = strlen(p); if (p_len < 1) return p; Aren't p and r of type "const char *", I wonder... > + /* r points to last non-null character in p */ > + r = p + p_len - 1; > + /* first skip any trailing slashes */ > + while (*r == '/' && r > p) r--; That is r = strrchr(p, '/'); isn't it? > +/** > + * 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. I suspect some callers use lockfile interface to create a new file. There will be a symlink to not-yet-created real file, that is. ``` ## Bradford C. Smith, 2007-07-26 17:34 Subject: [PATCH] fully resolve symlinks when creating lockfiles Message-ID: <11854712542350-git-send-email-bradford.carl.smith@gmail.com> URL: https://gitlist.dev/e/11854712542350-git-send-email-bradford.carl.smith%40gmail.com In-Reply-To: <7vbqe0cazy.fsf@assigned-by-dhcp.cox.net> ``` Make the code for resolving symlinks in lockfile.c more robust as follows: 1. Handle relative symlinks 2. recursively resolve symlink chains up to OS limit Signed-off-by: Bradford C. Smith --- I have updated this patch as follows based partly on Junio's comments. 1. Made comment and coding style consistent with existing git code base. 2. improved readability 3. rebased to latest version of master (2007-07-26) and updated commit message appropriately 4. added warning messages for error conditions 5. resolve symlinks to non-existent files lockfile.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 114 insertions(+), 14 deletions(-) diff --git a/lockfile.c b/lockfile.c index 9202472..864ce73 100644 --- a/lockfile.c +++ b/lockfile.c @@ -25,23 +25,123 @@ 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 const char *last_path_elm(const char *p) +{ + /* r starts pointing to null at the end of the string */ + const 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; +} + + +/* + * 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) +{ + struct stat stb; + char link[PATH_MAX]; + int link_len; + + /* + * leave p unchanged if it doesn't appear to be a valid path to + * a symlink. + */ + if (lstat(p, &stb) != 0 || !S_ISLNK(stb.st_mode)) { + return p; + } + /* + * don't attempt to resolve a chain or loop of symlinks the OS + * cannot resolve. + */ + if (stat(p, &stb) != 0 && ELOOP == errno) { + warning("%s: %s", p, strerror(ELOOP)); + return p; + } + + link_len = readlink(p, link, sizeof(link)); + if (link_len < 0) { + warning("%s: %s", p, strerror(errno)); + 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; + } + } + /* 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; - 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) { -- 1.5.3.rc3.9.g1b487 ``` ## Johannes Schindelin, 2007-07-26 18:35 Subject: Re: [PATCH] fully resolve symlinks when creating lockfiles Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0707261934080.14781%40racer.site In-Reply-To: <11854712542350-git-send-email-bradford.carl.smith@gmail.com> ``` Hi, On Thu, 26 Jul 2007, Bradford C. Smith wrote: > Make the code for resolving symlinks in lockfile.c more robust as > follows: > > 1. Handle relative symlinks > 2. recursively resolve symlink chains up to OS limit FWIW I like what it does, but how. What is so wrong with just relying on is_absolute_path() and make_absolute_path()? The code would be much shorter then, and we need those functions anyway, methinks. Ciao, Dscho ``` ## Morten Welinder, 2007-07-26 19:34 Subject: Re: [PATCH] fully resolve symlinks when creating lockfiles Message-ID: <118833cc0707261234u59e30bchc274ae29569d8500@mail.gmail.com> URL: https://gitlist.dev/e/118833cc0707261234u59e30bchc274ae29569d8500%40mail.gmail.com In-Reply-To: <11854712542350-git-send-email-bradford.carl.smith@gmail.com> ``` Why the lstat and that stat in the beginning? That's just asking for race condition. readlink will tell you if it wasn't a link, for example. Morten ``` ## Junio C Hamano, 2007-07-27 07:05 Subject: Re: [PATCH] fully resolve symlinks when creating lockfiles Message-ID: <7vk5sm2unt.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk5sm2unt.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11854712542350-git-send-email-bradford.carl.smith@gmail.com> ``` "Bradford C. Smith" writes: > Make the code for resolving symlinks in lockfile.c more robust as > follows: > > 1. Handle relative symlinks > 2. recursively resolve symlink chains up to OS limit I munged this patch with Morten's comments. Will queue for 'next'. Further polishing will be done in 'next' as needed. ``` ## Bradford Smith, 2007-07-27 16:50 Subject: Re: [PATCH] fully resolve symlinks when creating lockfiles Message-ID: URL: https://gitlist.dev/e/f158199e0707270950m638f6863t8272ca50430c304c%40mail.gmail.com In-Reply-To: <118833cc0707261234u59e30bchc274ae29569d8500@mail.gmail.com> ``` On 7/26/07, Morten Welinder wrote: > Why the lstat and that stat in the beginning? That's just asking for race > condition. readlink will tell you if it wasn't a link, for example. Here's an example of the sort of thing I'm trying to avoid: foo is a symlink to bar bar is a symlink back to foo readlink() on either one will succeed, but I'll end up with infinite recursion because I'll resolve foo to bar, then bar to foo, then foo back to bar, etc. To avoid craziness like this the OS refuses to follow a chain of more than a very small number of symlinks. By experimentation, I found the limit to be 8 on my Linux box. I am trying to avoid resolving symlinks manually that the OS would refuse to resolve anyway. However, I'm quite open to suggestions for a better way to do it. Thanks, Bradford ```