From fcd12db6af118b70b5c15cf5fdd6800eeecc370a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:35:31 -0400 Subject: prefer git_pathdup to git_path in some possibly-dangerous cases Because git_path uses a static buffer that is shared with calls to git_path, mkpath, etc, it can be dangerous to assign the result to a variable or pass it to a non-trivial function. The value may change unexpectedly due to other calls. None of the cases changed here has a known bug, but they're worth converting away from git_path because: 1. It's easy to use git_pathdup in these cases. 2. They use constructs (like assignment) that make it hard to tell whether they're safe or not. The extra malloc overhead should be trivial, as an allocation should be an order of magnitude cheaper than a system call (which we are clearly about to make, since we are constructing a filename). The real cost is that we must remember to free the result. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 2db2975e08..93b250e754 100644 --- a/refs.c +++ b/refs.c @@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) */ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs) { - const char *packed_refs_file; + char *packed_refs_file; if (*refs->name) - packed_refs_file = git_path_submodule(refs->name, "packed-refs"); + packed_refs_file = git_pathdup_submodule(refs->name, "packed-refs"); else - packed_refs_file = git_path("packed-refs"); + packed_refs_file = git_pathdup("packed-refs"); if (refs->packed && !stat_validity_check(&refs->packed->validity, packed_refs_file)) @@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs) fclose(f); } } + free(packed_refs_file); return refs->packed; } @@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs, { int fd, len; char buffer[128], *p; - const char *path; + char *path; if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN) return -1; path = *refs->name - ? git_path_submodule(refs->name, "%s", refname) - : git_path("%s", refname); + ? git_pathdup_submodule(refs->name, "%s", refname) + : git_pathdup("%s", refname); fd = open(path, O_RDONLY); + free(path); if (fd < 0) return resolve_gitlink_packed_ref(refs, refname, sha1); -- cgit v1.2.1 From e3cf230324f740653d4fb4a3087c2daf9da62029 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:35:38 -0400 Subject: prefer mkpathdup to mkpath in assignments As with the previous commit to git_path, assigning the result of mkpath is suspicious, since it is not clear whether we will still depend on the value after it may have been overwritten by subsequent calls. This patch converts low-hanging fruit to use mkpathdup instead of mkpath (with the downside that we must remember to free the result). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 93b250e754..e70941a090 100644 --- a/refs.c +++ b/refs.c @@ -3380,7 +3380,7 @@ static int commit_ref_update(struct ref_lock *lock, int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { - const char *lockpath; + char *lockpath = NULL; char ref[1000]; int fd, len, written; char *git_HEAD = git_pathdup("%s", ref_target); @@ -3407,7 +3407,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, error("refname too long: %s", refs_heads_master); goto error_free_return; } - lockpath = mkpath("%s.lock", git_HEAD); + lockpath = mkpathdup("%s.lock", git_HEAD); fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd < 0) { error("Unable to open %s for writing", lockpath); @@ -3427,9 +3427,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master, error_unlink_return: unlink_or_warn(lockpath); error_free_return: + free(lockpath); free(git_HEAD); return -1; } + free(lockpath); #ifndef NO_SYMLINK_HEAD done: -- cgit v1.2.1 From f5b2dec1657e09a22f8b2aefa25d022988e3e467 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:36:19 -0400 Subject: refs.c: remove extra git_path calls from read_loose_refs In iterating over the loose refs in "refs/foo/", we keep a running strbuf with "refs/foo/one", "refs/foo/two", etc. But we also need to access these files in the filesystem, as ".git/refs/foo/one", etc. For this latter purpose, we make a series of independent calls to git_path(). These are safe (we only use the result to call stat()), but assigning the result of git_path is a suspicious pattern that we'd rather avoid. This patch keeps a running buffer with ".git/refs/foo/", and we can just append/reset each directory element as we loop. This matches how we handle the refnames. It should also be more efficient, as we do not keep formatting the same ".git/refs/foo" prefix (which can be arbitrarily deep). Technically we are dropping a call to strbuf_cleanup() on each generated filename, but that's OK; it wasn't doing anything, as we are putting in single-level names we read from the filesystem (so it could not possibly be cleaning up cruft like "./" in this instance). A clever reader may also note that the running refname buffer ("refs/foo/") is actually a subset of the filesystem path buffer (".git/refs/foo/"). We could get by with one buffer, indexing the length of $GIT_DIR when we want the refname. However, having tried this, the resulting code actually ends up a little more confusing, and the efficiency improvement is tiny (and almost certainly dwarfed by the system calls we are making). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index e70941a090..06f95c4c12 100644 --- a/refs.c +++ b/refs.c @@ -1352,19 +1352,23 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) { struct ref_cache *refs = dir->ref_cache; DIR *d; - const char *path; struct dirent *de; int dirnamelen = strlen(dirname); struct strbuf refname; + struct strbuf path = STRBUF_INIT; + size_t path_baselen; if (*refs->name) - path = git_path_submodule(refs->name, "%s", dirname); + strbuf_git_path_submodule(&path, refs->name, "%s", dirname); else - path = git_path("%s", dirname); + strbuf_git_path(&path, "%s", dirname); + path_baselen = path.len; - d = opendir(path); - if (!d) + d = opendir(path.buf); + if (!d) { + strbuf_release(&path); return; + } strbuf_init(&refname, dirnamelen + 257); strbuf_add(&refname, dirname, dirnamelen); @@ -1373,17 +1377,14 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) unsigned char sha1[20]; struct stat st; int flag; - const char *refdir; if (de->d_name[0] == '.') continue; if (ends_with(de->d_name, ".lock")) continue; strbuf_addstr(&refname, de->d_name); - refdir = *refs->name - ? git_path_submodule(refs->name, "%s", refname.buf) - : git_path("%s", refname.buf); - if (stat(refdir, &st) < 0) { + strbuf_addstr(&path, de->d_name); + if (stat(path.buf, &st) < 0) { ; /* silently ignore */ } else if (S_ISDIR(st.st_mode)) { strbuf_addch(&refname, '/'); @@ -1430,8 +1431,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) create_ref_entry(refname.buf, sha1, flag, 0)); } strbuf_setlen(&refname, dirnamelen); + strbuf_setlen(&path, path_baselen); } strbuf_release(&refname); + strbuf_release(&path); closedir(d); } -- cgit v1.2.1 From 54b418f6985568bcb71c354ada204efe103b870e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 08:26:38 -0400 Subject: refs.c: simplify strbufs in reflog setup and writing Commit 1a83c24 (git_snpath(): retire and replace with strbuf_git_path(), 2014-11-30) taught log_ref_setup and log_ref_write_1 to take a strbuf parameter, rather than a bare string. It then makes an alias to the strbuf's "buf" field under the original name. This made the original diff much shorter, but the resulting code is more complicated that it needs to be. Since we've aliased the pointer, we drop our reference to the strbuf to ensure we don't accidentally change it. But if we simply drop our alias and use "logfile.buf" directly, we do not have to worry about this aliasing. It's a larger diff, but the resulting code is simpler. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 06f95c4c12..105f271e48 100644 --- a/refs.c +++ b/refs.c @@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname) * should_autocreate_reflog returns non-zero. Otherwise, create it * regardless of the ref name. Fill in *err and return -1 on failure. */ -static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create) +static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create) { int logfd, oflags = O_APPEND | O_WRONLY; - char *logfile; - strbuf_git_path(sb_logfile, "logs/%s", refname); - logfile = sb_logfile->buf; - /* make sure the rest of the function can't change "logfile" */ - sb_logfile = NULL; + strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (safe_create_leading_directories(logfile) < 0) { + if (safe_create_leading_directories(logfile->buf) < 0) { strbuf_addf(err, "unable to create directory for %s: " - "%s", logfile, strerror(errno)); + "%s", logfile->buf, strerror(errno)); return -1; } oflags |= O_CREAT; } - logfd = open(logfile, oflags, 0666); + logfd = open(logfile->buf, oflags, 0666); if (logfd < 0) { if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR)) return 0; if (errno == EISDIR) { - if (remove_empty_directories(logfile)) { + if (remove_empty_directories(logfile->buf)) { strbuf_addf(err, "There are still logs under " - "'%s'", logfile); + "'%s'", logfile->buf); return -1; } - logfd = open(logfile, oflags, 0666); + logfd = open(logfile->buf, oflags, 0666); } if (logfd < 0) { strbuf_addf(err, "unable to append to %s: %s", - logfile, strerror(errno)); + logfile->buf, strerror(errno)); return -1; } } - adjust_shared_perm(logfile); + adjust_shared_perm(logfile->buf); close(logfd); return 0; } @@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, - struct strbuf *sb_log_file, int flags, + struct strbuf *logfile, int flags, struct strbuf *err) { int logfd, result, oflags = O_APPEND | O_WRONLY; - char *log_file; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG); + result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG); if (result) return result; - log_file = sb_log_file->buf; - /* make sure the rest of the function can't change "log_file" */ - sb_log_file = NULL; - logfd = open(log_file, oflags); + logfd = open(logfile->buf, oflags); if (logfd < 0) return 0; result = log_ref_write_fd(logfd, old_sha1, new_sha1, git_committer_info(0), msg); if (result) { - strbuf_addf(err, "unable to append to %s: %s", log_file, + strbuf_addf(err, "unable to append to %s: %s", logfile->buf, strerror(errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_addf(err, "unable to append to %s: %s", log_file, + strbuf_addf(err, "unable to append to %s: %s", logfile->buf, strerror(errno)); return -1; } -- cgit v1.2.1 From d6549f3655212bd2de52df0137ceb59180424061 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:36:53 -0400 Subject: refs.c: avoid repeated git_path calls in rename_tmp_log Because it's not safe to store the static-buffer results of git_path for a long time, we end up formatting the same filename over and over. We can fix this by using a function-local strbuf to store the formatted pathname and avoid repeating ourselves. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 105f271e48..086fb1ad02 100644 --- a/refs.c +++ b/refs.c @@ -2930,9 +2930,13 @@ out: static int rename_tmp_log(const char *newrefname) { int attempts_remaining = 4; + struct strbuf path = STRBUF_INIT; + int ret = -1; retry: - switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) { + strbuf_reset(&path); + strbuf_git_path(&path, "logs/%s", newrefname); + switch (safe_create_leading_directories_const(path.buf)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@ -2941,19 +2945,19 @@ static int rename_tmp_log(const char *newrefname) /* fall through */ default: error("unable to create directory for %s", newrefname); - return -1; + goto out; } - if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { + if (rename(git_path(TMP_RENAMED_LOG), path.buf)) { if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) { /* * rename(a, b) when b is an existing * directory ought to result in ISDIR, but * Solaris 5.8 gives ENOTDIR. Sheesh. */ - if (remove_empty_directories(git_path("logs/%s", newrefname))) { + if (remove_empty_directories(path.buf)) { error("Directory not empty: logs/%s", newrefname); - return -1; + goto out; } goto retry; } else if (errno == ENOENT && --attempts_remaining > 0) { @@ -2966,10 +2970,13 @@ static int rename_tmp_log(const char *newrefname) } else { error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", newrefname, strerror(errno)); - return -1; + goto out; } } - return 0; + ret = 0; +out: + strbuf_release(&path); + return ret; } static int rename_ref_available(const char *oldname, const char *newname) -- cgit v1.2.1 From 5f8ef5b84889e7792e929a0fc773cb0060a0a611 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:12 -0400 Subject: refs.c: avoid git_path assignment in lock_ref_sha1_basic Assigning the result of git_path is a bad pattern, because it's not immediately obvious how long you expect the content to stay valid (and it may be overwritten by subsequent calls). Let's use a function-local strbuf here instead, which we know is safe (we just have to remember to free it in all code paths). As a bonus, we get rid of a confusing variable-reuse ("ref_file" is used for two distinct purposes). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 086fb1ad02..4ed9f6b048 100644 --- a/refs.c +++ b/refs.c @@ -2408,7 +2408,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, unsigned int flags, int *type_p, struct strbuf *err) { - const char *ref_file; + struct strbuf ref_file = STRBUF_INIT; + struct strbuf orig_ref_file = STRBUF_INIT; const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; @@ -2432,20 +2433,19 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, refname = resolve_ref_unsafe(refname, resolve_flags, lock->old_oid.hash, &type); if (!refname && errno == EISDIR) { - /* we are trying to lock foo but we used to + /* + * we are trying to lock foo but we used to * have foo/bar which now does not exist; * it is normal for the empty directory 'foo' * to remain. */ - ref_file = git_path("%s", orig_refname); - if (remove_empty_directories(ref_file)) { + strbuf_git_path(&orig_ref_file, "%s", orig_refname); + if (remove_empty_directories(orig_ref_file.buf)) { last_errno = errno; - if (!verify_refname_available(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) strbuf_addf(err, "there are still refs under '%s'", orig_refname); - goto error_return; } refname = resolve_ref_unsafe(orig_refname, resolve_flags, @@ -2485,10 +2485,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, } lock->ref_name = xstrdup(refname); lock->orig_ref_name = xstrdup(orig_refname); - ref_file = git_path("%s", refname); + strbuf_git_path(&ref_file, "%s", refname); retry: - switch (safe_create_leading_directories_const(ref_file)) { + switch (safe_create_leading_directories_const(ref_file.buf)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@ -2497,11 +2497,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, /* fall through */ default: last_errno = errno; - strbuf_addf(err, "unable to create directory for %s", ref_file); + strbuf_addf(err, "unable to create directory for %s", + ref_file.buf); goto error_return; } - if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) { last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* @@ -2511,7 +2512,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else { - unable_to_lock_message(ref_file, errno, err); + unable_to_lock_message(ref_file.buf, errno, err); goto error_return; } } @@ -2519,12 +2520,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, last_errno = errno; goto error_return; } - return lock; + goto out; error_return: unlock_ref(lock); + lock = NULL; + + out: + strbuf_release(&ref_file); + strbuf_release(&orig_ref_file); errno = last_errno; - return NULL; + return lock; } /* -- cgit v1.2.1 From 470e28d4e16dd994cd985914377fa8ccb5f86227 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:27 -0400 Subject: refs.c: remove_empty_directories can take a strbuf The first thing we do in this function is copy the input into a strbuf. Of the 4 callers, 3 of them already have a strbuf we could use. Let's just take the strbuf, and convert the remaining caller to use a strbuf, rather than a raw git_path. This is safer, anyway, as remove_dir_recursively is a non-trivial function that might use the pathname buffers itself (this is _probably_ OK, as the likely culprit would be calling resolve_gitlink_ref, but we do not pass the proper flags to ask it to avoid blowing away gitlinks). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 4ed9f6b048..84b1aff842 100644 --- a/refs.c +++ b/refs.c @@ -2290,25 +2290,14 @@ static int verify_lock(struct ref_lock *lock, return 0; } -static int remove_empty_directories(const char *file) +static int remove_empty_directories(struct strbuf *path) { - /* we want to create a file but there is a directory there; + /* + * we want to create a file but there is a directory there; * if that is an empty directory (or a directory that contains * only empty directories), remove them. */ - struct strbuf path; - int result, save_errno; - - strbuf_init(&path, 20); - strbuf_addstr(&path, file); - - result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); - save_errno = errno; - - strbuf_release(&path); - errno = save_errno; - - return result; + return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); } /* @@ -2440,7 +2429,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, * to remain. */ strbuf_git_path(&orig_ref_file, "%s", orig_refname); - if (remove_empty_directories(orig_ref_file.buf)) { + if (remove_empty_directories(&orig_ref_file)) { last_errno = errno; if (!verify_refname_available(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) @@ -2961,7 +2950,7 @@ static int rename_tmp_log(const char *newrefname) * directory ought to result in ISDIR, but * Solaris 5.8 gives ENOTDIR. Sheesh. */ - if (remove_empty_directories(path.buf)) { + if (remove_empty_directories(&path)) { error("Directory not empty: logs/%s", newrefname); goto out; } @@ -3046,7 +3035,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) && delete_ref(newrefname, sha1, REF_NODEREF)) { if (errno==EISDIR) { - if (remove_empty_directories(git_path("%s", newrefname))) { + struct strbuf path = STRBUF_INIT; + int result; + + strbuf_git_path(&path, "%s", newrefname); + result = remove_empty_directories(&path); + strbuf_release(&path); + + if (result) { error("Directory not empty: %s", newrefname); goto rollback; } @@ -3183,7 +3179,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str return 0; if (errno == EISDIR) { - if (remove_empty_directories(logfile->buf)) { + if (remove_empty_directories(logfile)) { strbuf_addf(err, "There are still logs under " "'%s'", logfile->buf); return -1; -- cgit v1.2.1