From c82c75b9518750a94e38c84fc89741a51b815014 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:07 -0400 Subject: [PATCH 01/20] write_index_as_tree: cleanup tempfile on error If we failed to write our new index file, we rollback our lockfile to remove the temporary index. But if we fail before we even get to the write step (because reading the old index failed), we leave the lockfile in place, which makes no sense. In practice this hasn't been a big deal because failing at write_index_as_tree() typically results in the whole program exiting (and thus the tempfile handler kicking in and cleaning up the files). But this function should consistently take responsibility for the resources it allocates. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache-tree.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 2440d1dc89..2690113a6a 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -604,6 +604,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co { int entries, was_valid, newfd; struct lock_file *lock_file; + int ret = 0; /* * We can't free this memory, it becomes part of a linked list @@ -614,8 +615,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR); entries = read_index_from(index_state, index_path); - if (entries < 0) - return WRITE_TREE_UNREADABLE_INDEX; + if (entries < 0) { + ret = WRITE_TREE_UNREADABLE_INDEX; + goto out; + } if (flags & WRITE_TREE_IGNORE_CACHE_TREE) cache_tree_free(&index_state->cache_tree); @@ -624,8 +627,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co was_valid = cache_tree_fully_valid(index_state->cache_tree); if (!was_valid) { - if (cache_tree_update(index_state, flags) < 0) - return WRITE_TREE_UNMERGED_INDEX; + if (cache_tree_update(index_state, flags) < 0) { + ret = WRITE_TREE_UNMERGED_INDEX; + goto out; + } if (0 <= newfd) { if (!write_locked_index(index_state, lock_file, COMMIT_LOCK)) newfd = -1; @@ -641,17 +646,19 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co if (prefix) { struct cache_tree *subtree; subtree = cache_tree_find(index_state->cache_tree, prefix); - if (!subtree) - return WRITE_TREE_PREFIX_ERROR; + if (!subtree) { + ret = WRITE_TREE_PREFIX_ERROR; + goto out; + } hashcpy(sha1, subtree->oid.hash); } else hashcpy(sha1, index_state->cache_tree->oid.hash); +out: if (0 <= newfd) rollback_lock_file(lock_file); - - return 0; + return ret; } int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) From 0899013993cd5eb327bc1a40d7c629db37e5ea83 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:16 -0400 Subject: [PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile When there are no shallow entries to write, we skip creating the tempfile entirely and try to return the empty string. But we do so by calling get_tempfile_path() on the inactive tempfile object. This will trigger an assertion that kills the program. The bug was introduced by 6e122b449b (setup_temporary_shallow(): use tempfile module, 2015-08-10). But nobody seems to have noticed since then because we do not end up calling this function at all when there are no shallow items. In other words, this code path is completely unexercised. Since the tempfile object is a static global, it _is_ possible that we call the function twice, writing out shallow info the first time and then "reusing" our tempfile object the second time. But: 1. It seems unlikely that this was the intent, as hitting this code path would imply somebody clearing the shallow_info list between calls. And if somebody _did_ call the function multiple times without clearing the shallow_info list, we'd hit a different BUG for trying to reuse an already-active tempfile. 2. I verified by code inspection that the function is only called once per program. And also replacing this code with a BUG() and running the test suite demonstrates that it is not triggered there. So we could probably just replace this with an assertion and confirm that it's never called. However, the original intent does seem to be that you _could_ call it when the shallow_info is empty. And that's easy enough to do; since the return value doesn't need to point to a writable buffer, we can just return a string literal. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- shallow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shallow.c b/shallow.c index f5591e56da..29194b475a 100644 --- a/shallow.c +++ b/shallow.c @@ -307,7 +307,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra) * is_repository_shallow() sees empty string as "no shallow * file". */ - return get_tempfile_path(&temporary_shallow); + return ""; } void setup_alternate_shallow(struct lock_file *shallow_lock, From c0e963b77c6892d40d4ca81a71098aaa6c4eaed3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:19 -0400 Subject: [PATCH 03/20] setup_temporary_shallow: move tempfile struct into function The setup_temporary_shallow() function creates a temporary file, but we never access the tempfile struct outside of the function. This is OK, since it means we'll just clean up the tempfile on exit. But we can simplify the code a bit by moving the global tempfile struct to the only function in which it's used. Note that it must remain "static" due to tempfile.c's requirement that tempfile storage never goes away until program exit. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- shallow.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/shallow.c b/shallow.c index 29194b475a..c7fd68ace0 100644 --- a/shallow.c +++ b/shallow.c @@ -286,22 +286,21 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol, return write_shallow_commits_1(out, use_pack_protocol, extra, 0); } -static struct tempfile temporary_shallow; - const char *setup_temporary_shallow(const struct oid_array *extra) { + static struct tempfile temp; struct strbuf sb = STRBUF_INIT; int fd; if (write_shallow_commits(&sb, 0, extra)) { - fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX")); + fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX")); if (write_in_full(fd, sb.buf, sb.len) != sb.len) die_errno("failed to write to %s", - get_tempfile_path(&temporary_shallow)); - close_tempfile(&temporary_shallow); + get_tempfile_path(&temp)); + close_tempfile(&temp); strbuf_release(&sb); - return get_tempfile_path(&temporary_shallow); + return get_tempfile_path(&temp); } /* * is_repository_shallow() sees empty string as "no shallow From d88ef6605120fd75be38376ba147623cf427bf73 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:23 -0400 Subject: [PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close() We do a manual close() on the descriptor provided to us by mks_tempfile. But this runs contrary to the advice in tempfile.h, which notes that you should always use close_tempfile(). Otherwise the descriptor may be reused without the tempfile object knowing it, and the later call to delete_tempfile() could close a random descriptor. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- gpg-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index d936f3a32f..455b6c04b4 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -215,7 +215,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, delete_tempfile(&temp); return -1; } - close(fd); + close_tempfile(&temp); argv_array_pushl(&gpg.args, gpg_program, From 45c6b1ed24724f7f3041a60a4313df7d9c4b9909 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:26 -0400 Subject: [PATCH 05/20] always check return value of close_tempfile If close_tempfile() encounters an error, then it deletes the tempfile and resets the "struct tempfile". But many code paths ignore the return value and continue to use the tempfile. Instead, we should generally treat this the same as a write() error. Note that in the postimage of some of these cases our error message will be bogus after a failed close because we look at tempfile->filename (either directly or via get_tempfile_path). But after the failed close resets the tempfile object, this is guaranteed to be the empty string. That will be addressed in a future patch (because there are many more cases of the same problem than just these instances). Note also in the hunk in gpg-interface.c that it's fine to call delete_tempfile() in the error path, even if close_tempfile() failed and already deleted the file. The tempfile code is smart enough to know the second deletion is a noop. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 4 ++-- gpg-interface.c | 4 ++-- shallow.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index 3d3e553a98..fdb974014b 100644 --- a/diff.c +++ b/diff.c @@ -3738,9 +3738,9 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, blob = buf.buf; size = buf.len; } - if (write_in_full(fd, blob, size) != size) + if (write_in_full(fd, blob, size) != size || + close_tempfile(&temp->tempfile)) die_errno("unable to write temp-file"); - close_tempfile(&temp->tempfile); temp->name = get_tempfile_path(&temp->tempfile); oid_to_hex_r(temp->hex, oid); xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode); diff --git a/gpg-interface.c b/gpg-interface.c index 455b6c04b4..05ca6ecbfd 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -209,13 +209,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size, fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX"); if (fd < 0) return error_errno(_("could not create temporary file")); - if (write_in_full(fd, signature, signature_size) < 0) { + if (write_in_full(fd, signature, signature_size) < 0 || + close_tempfile(&temp) < 0) { error_errno(_("failed writing detached signature to '%s'"), temp.filename.buf); delete_tempfile(&temp); return -1; } - close_tempfile(&temp); argv_array_pushl(&gpg.args, gpg_program, diff --git a/shallow.c b/shallow.c index c7fd68ace0..f49e6ae122 100644 --- a/shallow.c +++ b/shallow.c @@ -295,10 +295,10 @@ const char *setup_temporary_shallow(const struct oid_array *extra) if (write_shallow_commits(&sb, 0, extra)) { fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX")); - if (write_in_full(fd, sb.buf, sb.len) != sb.len) + if (write_in_full(fd, sb.buf, sb.len) != sb.len || + close_tempfile(&temp) < 0) die_errno("failed to write to %s", get_tempfile_path(&temp)); - close_tempfile(&temp); strbuf_release(&sb); return get_tempfile_path(&temp); } From 49bd0fc2220eef17d8f5fd3ee76e391d03df8a6d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:30 -0400 Subject: [PATCH 06/20] tempfile: do not delete tempfile on failed close When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) || close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 2 +- gpg-interface.c | 2 +- lockfile.h | 8 +++++++- read-cache.c | 7 +++++-- shallow.c | 2 +- tempfile.c | 31 ++++++++++++------------------- tempfile.h | 25 +++++++++++++------------ 7 files changed, 40 insertions(+), 37 deletions(-) diff --git a/diff.c b/diff.c index fdb974014b..20f68ec389 100644 --- a/diff.c +++ b/diff.c @@ -3739,7 +3739,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, size = buf.len; } if (write_in_full(fd, blob, size) != size || - close_tempfile(&temp->tempfile)) + close_tempfile_gently(&temp->tempfile)) die_errno("unable to write temp-file"); temp->name = get_tempfile_path(&temp->tempfile); oid_to_hex_r(temp->hex, oid); diff --git a/gpg-interface.c b/gpg-interface.c index 05ca6ecbfd..4ea2597ff4 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -210,7 +210,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, if (fd < 0) return error_errno(_("could not create temporary file")); if (write_in_full(fd, signature, signature_size) < 0 || - close_tempfile(&temp) < 0) { + close_tempfile_gently(&temp) < 0) { error_errno(_("failed writing detached signature to '%s'"), temp.filename.buf); delete_tempfile(&temp); diff --git a/lockfile.h b/lockfile.h index 572064939c..dd4259bc40 100644 --- a/lockfile.h +++ b/lockfile.h @@ -246,7 +246,13 @@ extern char *get_locked_file_path(struct lock_file *lk); */ static inline int close_lock_file(struct lock_file *lk) { - return close_tempfile(&lk->tempfile); + int ret = close_tempfile_gently(&lk->tempfile); + if (ret) { + int saved_errno = errno; + delete_tempfile(&lk->tempfile); + errno = saved_errno; + } + return ret; } /* diff --git a/read-cache.c b/read-cache.c index 40da87ea71..51686518e0 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2309,8 +2309,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, if (ce_flush(&c, newfd, istate->sha1)) return -1; - if (close_tempfile(tempfile)) - return error(_("could not close '%s'"), tempfile->filename.buf); + if (close_tempfile_gently(tempfile)) { + error(_("could not close '%s'"), tempfile->filename.buf); + delete_tempfile(tempfile); + return -1; + } if (stat(tempfile->filename.buf, &st)) return -1; istate->timestamp.sec = (unsigned int)st.st_mtime; diff --git a/shallow.c b/shallow.c index f49e6ae122..36216febb6 100644 --- a/shallow.c +++ b/shallow.c @@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra) fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX")); if (write_in_full(fd, sb.buf, sb.len) != sb.len || - close_tempfile(&temp) < 0) + close_tempfile_gently(&temp) < 0) die_errno("failed to write to %s", get_tempfile_path(&temp)); strbuf_release(&sb); diff --git a/tempfile.c b/tempfile.c index 6843710670..c6740e562c 100644 --- a/tempfile.c +++ b/tempfile.c @@ -30,13 +30,12 @@ * `fdopen_tempfile()` has been called on the object * - `owner` holds the PID of the process that created the file * - * - Active, file closed (after successful `close_tempfile()`). Same + * - Active, file closed (after `close_tempfile_gently()`). Same * as the previous state, except that the temporary file is closed, * `fd` is -1, and `fp` is `NULL`. * - * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, a - * failed attempt to create a temporary file, or a failed - * `close_tempfile()`). In this state: + * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a + * failed attempt to create a temporary file). In this state: * * - `active` is unset * - `filename` is empty (usually, though there are transitory @@ -235,7 +234,7 @@ FILE *get_tempfile_fp(struct tempfile *tempfile) return tempfile->fp; } -int close_tempfile(struct tempfile *tempfile) +int close_tempfile_gently(struct tempfile *tempfile) { int fd = tempfile->fd; FILE *fp = tempfile->fp; @@ -258,14 +257,7 @@ int close_tempfile(struct tempfile *tempfile) err = close(fd); } - if (err) { - int save_errno = errno; - delete_tempfile(tempfile); - errno = save_errno; - return -1; - } - - return 0; + return err ? -1 : 0; } int reopen_tempfile(struct tempfile *tempfile) @@ -283,8 +275,10 @@ int rename_tempfile(struct tempfile *tempfile, const char *path) if (!tempfile->active) die("BUG: rename_tempfile called for inactive object"); - if (close_tempfile(tempfile)) + if (close_tempfile_gently(tempfile)) { + delete_tempfile(tempfile); return -1; + } if (rename(tempfile->filename.buf, path)) { int save_errno = errno; @@ -303,9 +297,8 @@ void delete_tempfile(struct tempfile *tempfile) if (!tempfile->active) return; - if (!close_tempfile(tempfile)) { - unlink_or_warn(tempfile->filename.buf); - tempfile->active = 0; - strbuf_reset(&tempfile->filename); - } + close_tempfile_gently(tempfile); + unlink_or_warn(tempfile->filename.buf); + tempfile->active = 0; + strbuf_reset(&tempfile->filename); } diff --git a/tempfile.h b/tempfile.h index 2f0038decd..d854dcdd3e 100644 --- a/tempfile.h +++ b/tempfile.h @@ -47,7 +47,7 @@ * control of the file. * * * Close the file descriptor without removing or renaming the - * temporary file by calling `close_tempfile()`, and later call + * temporary file by calling `close_tempfile_gently()`, and later call * `delete_tempfile()` or `rename_tempfile()`. * * Even after the temporary file is renamed or deleted, the `tempfile` @@ -59,7 +59,7 @@ * and remove the temporary file. * * If you need to close the file descriptor yourself, do so by calling - * `close_tempfile()`. You should never call `close(2)` or `fclose(3)` + * `close_tempfile_gently()`. You should never call `close(2)` or `fclose(3)` * yourself, otherwise the `struct tempfile` structure would still * think that the file descriptor needs to be closed, and a later * cleanup would result in duplicate calls to `close(2)`. Worse yet, @@ -74,9 +74,10 @@ * `create_tempfile()` returns a file descriptor on success or -1 on * failure. On errors, `errno` describes the reason for failure. * - * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile()` - * return 0 on success. On failure they set `errno` appropriately, do - * their best to delete the temporary file, and return -1. + * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()` + * return 0 on success. On failure they set `errno` appropriately and return + * -1. `delete` and `rename` (but not `close`) do their best to delete the + * temporary file before returning. */ struct tempfile { @@ -203,7 +204,7 @@ static inline int xmks_tempfile(struct tempfile *tempfile, /* * Associate a stdio stream with the temporary file (which must still * be open). Return `NULL` (*without* deleting the file) on error. The - * stream is closed automatically when `close_tempfile()` is called or + * stream is closed automatically when `close_tempfile_gently()` is called or * when the file is deleted or renamed. */ extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode); @@ -226,20 +227,20 @@ extern FILE *get_tempfile_fp(struct tempfile *tempfile); * If the temporary file is still open, close it (and the file pointer * too, if it has been opened using `fdopen_tempfile()`) without * deleting the file. Return 0 upon success. On failure to `close(2)`, - * return a negative value and delete the file. Usually - * `delete_tempfile()` or `rename_tempfile()` should eventually be - * called if `close_tempfile()` succeeds. + * return a negative value. Usually `delete_tempfile()` or `rename_tempfile()` + * should eventually be called regardless of whether `close_tempfile_gently()` + * succeeds. */ -extern int close_tempfile(struct tempfile *tempfile); +extern int close_tempfile_gently(struct tempfile *tempfile); /* * Re-open a temporary file that has been closed using - * `close_tempfile()` but not yet deleted or renamed. This can be used + * `close_tempfile_gently()` but not yet deleted or renamed. This can be used * to implement a sequence of operations like the following: * * * Create temporary file. * - * * Write new contents to file, then `close_tempfile()` to cause the + * * Write new contents to file, then `close_tempfile_gently()` to cause the * contents to be written to disk. * * * Pass the name of the temporary file to another program to allow From 83a3069a3895de81fea720ffa6a3e47f9400fe04 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:33 -0400 Subject: [PATCH 07/20] lockfile: do not rollback lock on failed close Since the lockfile code is based on the tempfile code, it has some of the same problems, including that close_lock_file() erases the tempfile's filename buf, making it hard for the caller to write a good error message. In practice this comes up less for lockfiles than for straight tempfiles, since we usually just report the refname. But there is at least one buggy case in write_ref_to_lockfile(). Besides, given the coupling between the lockfile and tempfile modules, it's less confusing if their close() functions have the same semantics. Just as the previous commit did for close_tempfile(), let's teach close_lock_file() and its wrapper close_ref() not to rollback on error. And just as before, we'll give them new "gently" names to catch any new callers that are added. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- lockfile.h | 30 ++++++++++++------------------ read-cache.c | 2 +- refs/files-backend.c | 13 +++++++------ refs/packed-backend.c | 3 ++- 4 files changed, 22 insertions(+), 26 deletions(-) diff --git a/lockfile.h b/lockfile.h index dd4259bc40..c45d2db324 100644 --- a/lockfile.h +++ b/lockfile.h @@ -69,7 +69,7 @@ * `rollback_lock_file()`. * * * Close the file descriptor without removing or renaming the - * lockfile by calling `close_lock_file()`, and later call + * lockfile by calling `close_lock_file_gently()`, and later call * `commit_lock_file()`, `commit_lock_file_to()`, * `rollback_lock_file()`, or `reopen_lock_file()`. * @@ -85,7 +85,7 @@ * * If you need to close the file descriptor you obtained from a * `hold_lock_file_for_*()` function yourself, do so by calling - * `close_lock_file()`. See "tempfile.h" for more information. + * `close_lock_file_gently()`. See "tempfile.h" for more information. * * * Under the covers, a lockfile is just a tempfile with a few helper @@ -104,8 +104,8 @@ * * Similarly, `commit_lock_file`, `commit_lock_file_to`, and * `close_lock_file` return 0 on success. On failure they set `errno` - * appropriately, do their best to roll back the lockfile, and return - * -1. + * appropriately and return -1. The `commit` variants (but not `close`) + * do their best to delete the temporary file before returning. */ #include "tempfile.h" @@ -202,8 +202,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err); /* * Associate a stdio stream with the lockfile (which must still be * open). Return `NULL` (*without* rolling back the lockfile) on - * error. The stream is closed automatically when `close_lock_file()` - * is called or when the file is committed or rolled back. + * error. The stream is closed automatically when + * `close_lock_file_gently()` is called or when the file is committed or + * rolled back. */ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) { @@ -241,28 +242,21 @@ extern char *get_locked_file_path(struct lock_file *lk); * lockfile over the file being locked. Return 0 upon success. On * failure to `close(2)`, return a negative value and roll back the * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`, - * or `rollback_lock_file()` should eventually be called if - * `close_lock_file()` succeeds. + * or `rollback_lock_file()` should eventually be called. */ -static inline int close_lock_file(struct lock_file *lk) +static inline int close_lock_file_gently(struct lock_file *lk) { - int ret = close_tempfile_gently(&lk->tempfile); - if (ret) { - int saved_errno = errno; - delete_tempfile(&lk->tempfile); - errno = saved_errno; - } - return ret; + return close_tempfile_gently(&lk->tempfile); } /* - * Re-open a lockfile that has been closed using `close_lock_file()` + * Re-open a lockfile that has been closed using `close_lock_file_gently()` * but not yet committed or rolled back. This can be used to implement * a sequence of operations like the following: * * * Lock file. * - * * Write new contents to lockfile, then `close_lock_file()` to + * * Write new contents to lockfile, then `close_lock_file_gently()` to * cause the contents to be written to disk. * * * Pass the name of the lockfile to another program to allow it (and diff --git a/read-cache.c b/read-cache.c index 51686518e0..c3be65f8b0 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2345,7 +2345,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l if (flags & COMMIT_LOCK) return commit_locked_index(lock); else if (flags & CLOSE_LOCK) - return close_lock_file(lock); + return close_lock_file_gently(lock); else return ret; } diff --git a/refs/files-backend.c b/refs/files-backend.c index fccbc24ac4..bc1899cd4a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store, return ret; } -static int close_ref(struct ref_lock *lock) +static int close_ref_gently(struct ref_lock *lock) { - if (close_lock_file(lock->lk)) + if (close_lock_file_gently(lock->lk)) return -1; return 0; } @@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, fd = get_lock_file_fd(lock->lk); if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_in_full(fd, &term, 1) != 1 || - close_ref(lock) < 0) { + close_ref_gently(lock) < 0) { strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(lock->lk)); unlock_ref(lock); @@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, * the lockfile is still open. Close it to * free up the file descriptor: */ - if (close_ref(lock)) { + if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); return TRANSACTION_GENERIC_ERROR; @@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store, !(type & REF_ISSYMREF) && !is_null_oid(&cb.last_kept_oid); - if (close_lock_file(&reflog_lock)) { + if (close_lock_file_gently(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, strerror(errno)); + rollback_lock_file(&reflog_lock); } else if (update && (write_in_full(get_lock_file_fd(lock->lk), oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || - close_ref(lock) < 0)) { + close_ref_gently(lock) < 0)) { status |= error("couldn't write %s", get_lock_file_path(lock->lk)); rollback_lock_file(&reflog_lock); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 412c85034f..dca887c0fe 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err) return -1; } - if (close_lock_file(&refs->lock)) { + if (close_lock_file_gently(&refs->lock)) { strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno)); + rollback_lock_file(&refs->lock); return -1; } From e6fc267314d24fe9a81875b85b28a4b5d0fb78b1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:36 -0400 Subject: [PATCH 08/20] tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tempfile.c b/tempfile.c index c6740e562c..964c66d504 100644 --- a/tempfile.c +++ b/tempfile.c @@ -95,7 +95,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile) atexit(remove_tempfiles_on_exit); } - if (tempfile->active) + if (is_tempfile_active(tempfile)) die("BUG: prepare_tempfile_object called for active object"); if (!tempfile->on_list) { /* Initialize *tempfile and add it to tempfile_list: */ @@ -204,7 +204,7 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode) FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: fdopen_tempfile() called for inactive object"); if (tempfile->fp) die("BUG: fdopen_tempfile() called for open object"); @@ -215,21 +215,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) const char *get_tempfile_path(struct tempfile *tempfile) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: get_tempfile_path() called for inactive object"); return tempfile->filename.buf; } int get_tempfile_fd(struct tempfile *tempfile) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: get_tempfile_fd() called for inactive object"); return tempfile->fd; } FILE *get_tempfile_fp(struct tempfile *tempfile) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: get_tempfile_fp() called for inactive object"); return tempfile->fp; } @@ -264,7 +264,7 @@ int reopen_tempfile(struct tempfile *tempfile) { if (0 <= tempfile->fd) die("BUG: reopen_tempfile called for an open object"); - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: reopen_tempfile called for an inactive object"); tempfile->fd = open(tempfile->filename.buf, O_WRONLY); return tempfile->fd; @@ -272,7 +272,7 @@ int reopen_tempfile(struct tempfile *tempfile) int rename_tempfile(struct tempfile *tempfile, const char *path) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) die("BUG: rename_tempfile called for inactive object"); if (close_tempfile_gently(tempfile)) { @@ -294,7 +294,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path) void delete_tempfile(struct tempfile *tempfile) { - if (!tempfile->active) + if (!is_tempfile_active(tempfile)) return; close_tempfile_gently(tempfile); From f5b4dc7668b6c8d71432af9f9ddad6f7c62d284e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:40 -0400 Subject: [PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully The tempfile functions all take pointers to tempfile objects, but do not check whether the argument is NULL. This isn't a big deal in practice, since the lifetime of any tempfile object is defined to last for the whole program. So even if we try to call delete_tempfile() on an already-deleted tempfile, our "active" check will tell us that it's a noop. In preparation for transitioning to a new system that loosens the "tempfile objects can never be freed" rule, let's tighten up our active checks: 1. A NULL pointer is now defined as "inactive" (so it will BUG for most functions, but works as a silent noop for things like delete_tempfile). 2. Functions should always do the "active" check before looking at any of the struct fields. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 12 +++++++----- tempfile.h | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tempfile.c b/tempfile.c index 964c66d504..861f817133 100644 --- a/tempfile.c +++ b/tempfile.c @@ -236,13 +236,15 @@ FILE *get_tempfile_fp(struct tempfile *tempfile) int close_tempfile_gently(struct tempfile *tempfile) { - int fd = tempfile->fd; - FILE *fp = tempfile->fp; + int fd; + FILE *fp; int err; - if (fd < 0) + if (!is_tempfile_active(tempfile) || tempfile->fd < 0) return 0; + fd = tempfile->fd; + fp = tempfile->fp; tempfile->fd = -1; if (fp) { tempfile->fp = NULL; @@ -262,10 +264,10 @@ int close_tempfile_gently(struct tempfile *tempfile) int reopen_tempfile(struct tempfile *tempfile) { - if (0 <= tempfile->fd) - die("BUG: reopen_tempfile called for an open object"); if (!is_tempfile_active(tempfile)) die("BUG: reopen_tempfile called for an inactive object"); + if (0 <= tempfile->fd) + die("BUG: reopen_tempfile called for an open object"); tempfile->fd = open(tempfile->filename.buf, O_WRONLY); return tempfile->fd; } diff --git a/tempfile.h b/tempfile.h index d854dcdd3e..d30663182d 100644 --- a/tempfile.h +++ b/tempfile.h @@ -211,7 +211,7 @@ extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode); static inline int is_tempfile_active(struct tempfile *tempfile) { - return tempfile->active; + return tempfile && tempfile->active; } /* From 9b028aa45a2016ae0dbdfeb85ad9d43f2017db0d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:43 -0400 Subject: [PATCH 10/20] tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tempfile.c b/tempfile.c index 861f817133..813cf6a81c 100644 --- a/tempfile.c +++ b/tempfile.c @@ -96,7 +96,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile) } if (is_tempfile_active(tempfile)) - die("BUG: prepare_tempfile_object called for active object"); + BUG("prepare_tempfile_object called for active object"); if (!tempfile->on_list) { /* Initialize *tempfile and add it to tempfile_list: */ tempfile->fd = -1; @@ -109,7 +109,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile) tempfile->on_list = 1; } else if (tempfile->filename.len) { /* This shouldn't happen, but better safe than sorry. */ - die("BUG: prepare_tempfile_object called for improperly-reset object"); + BUG("prepare_tempfile_object called for improperly-reset object"); } } @@ -205,9 +205,9 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode) FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) { if (!is_tempfile_active(tempfile)) - die("BUG: fdopen_tempfile() called for inactive object"); + BUG("fdopen_tempfile() called for inactive object"); if (tempfile->fp) - die("BUG: fdopen_tempfile() called for open object"); + BUG("fdopen_tempfile() called for open object"); tempfile->fp = fdopen(tempfile->fd, mode); return tempfile->fp; @@ -216,21 +216,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) const char *get_tempfile_path(struct tempfile *tempfile) { if (!is_tempfile_active(tempfile)) - die("BUG: get_tempfile_path() called for inactive object"); + BUG("get_tempfile_path() called for inactive object"); return tempfile->filename.buf; } int get_tempfile_fd(struct tempfile *tempfile) { if (!is_tempfile_active(tempfile)) - die("BUG: get_tempfile_fd() called for inactive object"); + BUG("get_tempfile_fd() called for inactive object"); return tempfile->fd; } FILE *get_tempfile_fp(struct tempfile *tempfile) { if (!is_tempfile_active(tempfile)) - die("BUG: get_tempfile_fp() called for inactive object"); + BUG("get_tempfile_fp() called for inactive object"); return tempfile->fp; } @@ -265,9 +265,9 @@ int close_tempfile_gently(struct tempfile *tempfile) int reopen_tempfile(struct tempfile *tempfile) { if (!is_tempfile_active(tempfile)) - die("BUG: reopen_tempfile called for an inactive object"); + BUG("reopen_tempfile called for an inactive object"); if (0 <= tempfile->fd) - die("BUG: reopen_tempfile called for an open object"); + BUG("reopen_tempfile called for an open object"); tempfile->fd = open(tempfile->filename.buf, O_WRONLY); return tempfile->fd; } @@ -275,7 +275,7 @@ int reopen_tempfile(struct tempfile *tempfile) int rename_tempfile(struct tempfile *tempfile, const char *path) { if (!is_tempfile_active(tempfile)) - die("BUG: rename_tempfile called for inactive object"); + BUG("rename_tempfile called for inactive object"); if (close_tempfile_gently(tempfile)) { delete_tempfile(tempfile); From 2933ebbac1967a677eaed1945068941bc3ff7751 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:47 -0400 Subject: [PATCH 11/20] tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tempfile.c b/tempfile.c index 813cf6a81c..0e6c6b9c18 100644 --- a/tempfile.c +++ b/tempfile.c @@ -113,6 +113,12 @@ static void prepare_tempfile_object(struct tempfile *tempfile) } } +static void activate_tempfile(struct tempfile *tempfile) +{ + tempfile->owner = getpid(); + tempfile->active = 1; +} + /* Make sure errno contains a meaningful value on error */ int create_tempfile(struct tempfile *tempfile, const char *path) { @@ -129,8 +135,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path) strbuf_reset(&tempfile->filename); return -1; } - tempfile->owner = getpid(); - tempfile->active = 1; + activate_tempfile(tempfile); if (adjust_shared_perm(tempfile->filename.buf)) { int save_errno = errno; error("cannot fix permission bits on %s", tempfile->filename.buf); @@ -145,8 +150,7 @@ void register_tempfile(struct tempfile *tempfile, const char *path) { prepare_tempfile_object(tempfile); strbuf_add_absolute_path(&tempfile->filename, path); - tempfile->owner = getpid(); - tempfile->active = 1; + activate_tempfile(tempfile); } int mks_tempfile_sm(struct tempfile *tempfile, @@ -160,8 +164,7 @@ int mks_tempfile_sm(struct tempfile *tempfile, strbuf_reset(&tempfile->filename); return -1; } - tempfile->owner = getpid(); - tempfile->active = 1; + activate_tempfile(tempfile); return tempfile->fd; } @@ -182,8 +185,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile, strbuf_reset(&tempfile->filename); return -1; } - tempfile->owner = getpid(); - tempfile->active = 1; + activate_tempfile(tempfile); return tempfile->fd; } From b5f4dcb598bb369c960f715fcd5c9ccf4112e026 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:50 -0400 Subject: [PATCH 12/20] tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tempfile.c b/tempfile.c index 0e6c6b9c18..9d7f0a2f2b 100644 --- a/tempfile.c +++ b/tempfile.c @@ -119,6 +119,12 @@ static void activate_tempfile(struct tempfile *tempfile) tempfile->active = 1; } +static void deactivate_tempfile(struct tempfile *tempfile) +{ + tempfile->active = 0; + strbuf_reset(&tempfile->filename); +} + /* Make sure errno contains a meaningful value on error */ int create_tempfile(struct tempfile *tempfile, const char *path) { @@ -132,7 +138,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path) tempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666); if (tempfile->fd < 0) { - strbuf_reset(&tempfile->filename); + deactivate_tempfile(tempfile); return -1; } activate_tempfile(tempfile); @@ -161,7 +167,7 @@ int mks_tempfile_sm(struct tempfile *tempfile, strbuf_add_absolute_path(&tempfile->filename, template); tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { - strbuf_reset(&tempfile->filename); + deactivate_tempfile(tempfile); return -1; } activate_tempfile(tempfile); @@ -182,7 +188,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile, strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, template); tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { - strbuf_reset(&tempfile->filename); + deactivate_tempfile(tempfile); return -1; } activate_tempfile(tempfile); @@ -291,8 +297,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path) return -1; } - tempfile->active = 0; - strbuf_reset(&tempfile->filename); + deactivate_tempfile(tempfile); return 0; } @@ -303,6 +308,5 @@ void delete_tempfile(struct tempfile *tempfile) close_tempfile_gently(tempfile); unlink_or_warn(tempfile->filename.buf); - tempfile->active = 0; - strbuf_reset(&tempfile->filename); + deactivate_tempfile(tempfile); } From 6b935066960d19713d85e7bbebd442dd8d35e0c6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:53 -0400 Subject: [PATCH 13/20] tempfile: robustify cleanup handler We may call remove_tempfiles() from an atexit handler, or from a signal handler. In the latter case we must take care to avoid functions which may deadlock if the process is in an unknown state, including looking at any stdio handles (which may be in the middle of doing I/O and locked) or calling malloc() or free(). The current implementation calls delete_tempfile(). We unset the tempfile's stdio handle (if any) to avoid deadlocking there. But delete_tempfile() still calls unlink_or_warn(), which can deadlock writing to stderr if the unlink fails. Since delete_tempfile() isn't very long, let's just open-code our own simple conservative version of the same thing. Notably: 1. The "skip_fclose" flag is now called "in_signal_handler", because it should inform more decisions than just the fclose handling. 2. We can replace close_tempfile() with just close(fd). That skips the fclose() question altogether. This is fine for the atexit() case, too; there's no point flushing data to a file which we're about to delete anyway. 3. We can choose between unlink/unlink_or_warn based on whether it's safe to use stderr. 4. We can replace the deactivate_tempfile() call with a simple setting of the active flag. There's no need to do any further cleanup since we know the program is exiting. And even though the current deactivation code is safe in a signal handler, this frees us up in future patches to make non-signal deactivation more complicated (e.g., by freeing resources). 5. There's no need to remove items from the tempfile_list. The "active" flag is the ultimate answer to whether an entry has been handled or not. Manipulating the list just introduces more chance of recursive signals stomping on each other, and the whole list will go away when the program exits anyway. Less is more. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/tempfile.c b/tempfile.c index 9d7f0a2f2b..3348ad59dd 100644 --- a/tempfile.c +++ b/tempfile.c @@ -57,18 +57,24 @@ static struct tempfile *volatile tempfile_list; -static void remove_tempfiles(int skip_fclose) +static void remove_tempfiles(int in_signal_handler) { pid_t me = getpid(); + struct tempfile *volatile p; - while (tempfile_list) { - if (tempfile_list->owner == me) { - /* fclose() is not safe to call in a signal handler */ - if (skip_fclose) - tempfile_list->fp = NULL; - delete_tempfile(tempfile_list); - } - tempfile_list = tempfile_list->next; + for (p = tempfile_list; p; p = p->next) { + if (!is_tempfile_active(p) || p->owner != me) + continue; + + if (p->fd >= 0) + close(p->fd); + + if (in_signal_handler) + unlink(p->filename.buf); + else + unlink_or_warn(p->filename.buf); + + p->active = 0; } } From 102cf7a6aa1f09177c09536a81dbaef44e13e038 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:14:56 -0400 Subject: [PATCH 14/20] tempfile: release deactivated strbufs instead of resetting When a tempfile is deactivated, we reset its strbuf to the empty string, which means we hold onto the memory for later reuse. Since we'd like to move to a system where tempfile structs can actually be freed, deactivating one should drop all resources it is currently using. And thus "release" rather than "reset" is the appropriate function to call. In theory the reset may have saved a malloc() when a tempfile (or a lockfile) is reused multiple times. But in practice this happened rarely. Most of our tempfiles are single-use, since in cases where we might actually use many (like ref locking) we xcalloc() a fresh one for each ref. In fact, we leak those locks (to appease the rule that tempfile storage can never be freed). Which means that using reset is actively hurting us: instead of leaking just the tempfile struct, we're leaking the extra heap chunk for the filename, too. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tempfile.c b/tempfile.c index 3348ad59dd..e655e28477 100644 --- a/tempfile.c +++ b/tempfile.c @@ -128,7 +128,7 @@ static void activate_tempfile(struct tempfile *tempfile) static void deactivate_tempfile(struct tempfile *tempfile) { tempfile->active = 0; - strbuf_reset(&tempfile->filename); + strbuf_release(&tempfile->filename); } /* Make sure errno contains a meaningful value on error */ From 24d82185d267daafc29ca61af1ed1dc746ba3463 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:00 -0400 Subject: [PATCH 15/20] tempfile: use list.h for linked list The tempfile API keeps to-be-cleaned tempfiles in a singly-linked list and never removes items from the list. A future patch would like to start removing items, but removal from a singly linked list is O(n), as we have to walk the list to find the predecessor element. This means that a process which takes "n" simultaneous lockfiles (for example, an atomic transaction on "n" refs) may end up quadratic in "n". Before we start allowing items to be removed, it would be nice to have a way to cover this case in linear time. The simplest solution is to make an assumption about the order in which tempfiles are added and removed from the list. If both operations iterate over the tempfiles in the same order, then by putting new items at the end of the list our removal search will always find its items at the beginning of the list. And indeed, that would work for the case of refs. But it creates a hidden dependency between unrelated parts of the code. If anybody changes the ref code (or if we add a new caller that opens multiple simultaneous tempfiles) they may unknowingly introduce a performance regression. Another solution is to use a better data structure. A doubly-linked list works fine, and we already have an implementation in list.h. But there's one snag: the elements of "struct tempfile" are all marked as "volatile", since a signal handler may interrupt us and iterate over the list at any moment (even if we were in the middle of adding a new entry). We can declare a "volatile struct list_head", but we can't actually use it with the normal list functions. The compiler complains about passing a pointer-to-volatile via a regular pointer argument. And rightfully so, as the sub-function would potentially need different code to deal with the volatile case. That leaves us with a few options: 1. Drop the "volatile" modifier for the list items. This is probably a bad idea. I checked the assembly output from "gcc -O2", and the "volatile" really does impact the order in which it updates memory. 2. Use macros instead of inline functions. The irony here is that list.h is entirely implemented as trivial inline functions. So we basically are already generating custom code for each call. But sadly there's no way in C to declare the inline function to take a more generic type. We could do so by switching the inline functions to macros, but it does make the end result harder to read. And it doesn't fully solve the problem (for instance, the declaration of list_head needs to change so that its "prev" and "next" pointers point to other volatile structs). 3. Don't use list.h, and just make our own ad-hoc doubly-linked list. It's not that much code to implement the basics that we need here. But if we're going to do so, why not add the few extra lines required to model it after the actual list.h interface? We can even reuse a few of the macro helpers. So this patch takes option 3, but actually implements a parallel "volatile list" interface in list.h, where it could potentially be reused by other code. This implements just enough for tempfile.c's use, though we could easily port other functions later if need be. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- list.h | 38 ++++++++++++++++++++++++++++++++++++++ tempfile.c | 13 +++++++------ tempfile.h | 4 +++- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/list.h b/list.h index a226a870dc..eb601192f4 100644 --- a/list.h +++ b/list.h @@ -163,4 +163,42 @@ static inline void list_replace_init(struct list_head *old, INIT_LIST_HEAD(old); } +/* + * This is exactly the same as a normal list_head, except that it can be + * declared volatile (e.g., if you have a list that may be accessed from signal + * handlers). + */ +struct volatile_list_head { + volatile struct volatile_list_head *next, *prev; +}; + +#define VOLATILE_LIST_HEAD(name) \ + volatile struct volatile_list_head name = { &(name), &(name) } + +static inline void __volatile_list_del(volatile struct volatile_list_head *prev, + volatile struct volatile_list_head *next) +{ + next->prev = prev; + prev->next = next; +} + +static inline void volatile_list_del(volatile struct volatile_list_head *elem) +{ + __volatile_list_del(elem->prev, elem->next); +} + +static inline int volatile_list_empty(volatile struct volatile_list_head *head) +{ + return head == head->next; +} + +static inline void volatile_list_add(volatile struct volatile_list_head *newp, + volatile struct volatile_list_head *head) +{ + head->next->prev = newp; + newp->next = head->next; + newp->prev = head; + head->next = newp; +} + #endif /* LIST_H */ diff --git a/tempfile.c b/tempfile.c index e655e28477..11bda824cf 100644 --- a/tempfile.c +++ b/tempfile.c @@ -55,14 +55,16 @@ #include "tempfile.h" #include "sigchain.h" -static struct tempfile *volatile tempfile_list; +static VOLATILE_LIST_HEAD(tempfile_list); static void remove_tempfiles(int in_signal_handler) { pid_t me = getpid(); - struct tempfile *volatile p; + volatile struct volatile_list_head *pos; + + list_for_each(pos, &tempfile_list) { + struct tempfile *p = list_entry(pos, struct tempfile, list); - for (p = tempfile_list; p; p = p->next) { if (!is_tempfile_active(p) || p->owner != me) continue; @@ -95,7 +97,7 @@ static void remove_tempfiles_on_signal(int signo) */ static void prepare_tempfile_object(struct tempfile *tempfile) { - if (!tempfile_list) { + if (volatile_list_empty(&tempfile_list)) { /* One-time initialization */ sigchain_push_common(remove_tempfiles_on_signal); atexit(remove_tempfiles_on_exit); @@ -110,8 +112,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile) tempfile->active = 0; tempfile->owner = 0; strbuf_init(&tempfile->filename, 0); - tempfile->next = tempfile_list; - tempfile_list = tempfile; + volatile_list_add(&tempfile->list, &tempfile_list); tempfile->on_list = 1; } else if (tempfile->filename.len) { /* This shouldn't happen, but better safe than sorry. */ diff --git a/tempfile.h b/tempfile.h index d30663182d..2ee24f4380 100644 --- a/tempfile.h +++ b/tempfile.h @@ -1,6 +1,8 @@ #ifndef TEMPFILE_H #define TEMPFILE_H +#include "list.h" + /* * Handle temporary files. * @@ -81,7 +83,7 @@ */ struct tempfile { - struct tempfile *volatile next; + volatile struct volatile_list_head list; volatile sig_atomic_t active; volatile int fd; FILE *volatile fp; From 422a21c6a086ec8c05c96b04bdccd960da141c04 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:04 -0400 Subject: [PATCH 16/20] tempfile: remove deactivated list entries Once a "struct tempfile" is added to the global cleanup list, it is never removed. This means that its storage must remain valid for the lifetime of the program. For single-use tempfiles and locks, this isn't a big deal: we just declare the struct static. But for library code which may take multiple simultaneous locks (like the ref code), they're forced to allocate a struct on the heap and leak it. This is mostly OK in practice. The size of the leak is bounded by the number of refs, and most programs exit after operating on a fixed number of refs (and allocate simultaneous memory proportional to the number of ref updates in the first place). But: 1. It isn't hard to imagine a real leak: a program which runs for a long time taking a series of ref update instructions and fulfilling them one by one. I don't think we have such a program now, but it's certainly plausible. 2. The leaked entries appear as false positives to tools like valgrind. Let's relax this rule by keeping only "active" tempfiles on the list. We can do this easily by moving the list-add operation from prepare_tempfile_object to activate_tempfile, and adding a deletion in deactivate_tempfile. Existing callers do not need to be updated immediately. They'll continue to leak any tempfile objects they may have allocated, but that's no different than the status quo. We can clean them up individually. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- tempfile.c | 46 ++++++++++++++++++++-------------------------- tempfile.h | 15 +++++---------- 2 files changed, 25 insertions(+), 36 deletions(-) diff --git a/tempfile.c b/tempfile.c index 11bda824cf..f82a5f3676 100644 --- a/tempfile.c +++ b/tempfile.c @@ -42,8 +42,7 @@ * states in which this condition doesn't hold). Client code should * *not* rely on the filename being empty in this state. * - `fd` is -1 and `fp` is `NULL` - * - the object is left registered in the `tempfile_list`, and - * `on_list` is set. + * - the object is removed from `tempfile_list` (but could be used again) * * A temporary file is owned by the process that created it. The * `tempfile` has an `owner` field that records the owner's PID. This @@ -92,36 +91,30 @@ static void remove_tempfiles_on_signal(int signo) raise(signo); } -/* - * Initialize *tempfile if necessary and add it to tempfile_list. - */ static void prepare_tempfile_object(struct tempfile *tempfile) { - if (volatile_list_empty(&tempfile_list)) { - /* One-time initialization */ - sigchain_push_common(remove_tempfiles_on_signal); - atexit(remove_tempfiles_on_exit); - } - - if (is_tempfile_active(tempfile)) - BUG("prepare_tempfile_object called for active object"); - if (!tempfile->on_list) { - /* Initialize *tempfile and add it to tempfile_list: */ - tempfile->fd = -1; - tempfile->fp = NULL; - tempfile->active = 0; - tempfile->owner = 0; - strbuf_init(&tempfile->filename, 0); - volatile_list_add(&tempfile->list, &tempfile_list); - tempfile->on_list = 1; - } else if (tempfile->filename.len) { - /* This shouldn't happen, but better safe than sorry. */ - BUG("prepare_tempfile_object called for improperly-reset object"); - } + tempfile->fd = -1; + tempfile->fp = NULL; + tempfile->active = 0; + tempfile->owner = 0; + INIT_LIST_HEAD(&tempfile->list); + strbuf_init(&tempfile->filename, 0); } static void activate_tempfile(struct tempfile *tempfile) { + static int initialized; + + if (is_tempfile_active(tempfile)) + BUG("activate_tempfile called for active object"); + + if (!initialized) { + sigchain_push_common(remove_tempfiles_on_signal); + atexit(remove_tempfiles_on_exit); + initialized = 1; + } + + volatile_list_add(&tempfile->list, &tempfile_list); tempfile->owner = getpid(); tempfile->active = 1; } @@ -130,6 +123,7 @@ static void deactivate_tempfile(struct tempfile *tempfile) { tempfile->active = 0; strbuf_release(&tempfile->filename); + volatile_list_del(&tempfile->list); } /* Make sure errno contains a meaningful value on error */ diff --git a/tempfile.h b/tempfile.h index 2ee24f4380..e32b4df092 100644 --- a/tempfile.h +++ b/tempfile.h @@ -17,12 +17,9 @@ * * The caller: * - * * Allocates a `struct tempfile` either as a static variable or on - * the heap, initialized to zeros. Once you use the structure to - * call `create_tempfile()`, it belongs to the tempfile subsystem - * and its storage must remain valid throughout the life of the - * program (i.e. you cannot use an on-stack variable to hold this - * structure). + * * Allocates a `struct tempfile`. Once the structure is passed to + * `create_tempfile()`, its storage must remain valid until + * `delete_tempfile()` or `rename_tempfile()` is called on it. * * * Attempts to create a temporary file by calling * `create_tempfile()`. @@ -52,9 +49,8 @@ * temporary file by calling `close_tempfile_gently()`, and later call * `delete_tempfile()` or `rename_tempfile()`. * - * Even after the temporary file is renamed or deleted, the `tempfile` - * object must not be freed or altered by the caller. However, it may - * be reused; just pass it to another call of `create_tempfile()`. + * After the temporary file is renamed or deleted, the `tempfile` + * object may be reused or freed. * * If the program exits before `rename_tempfile()` or * `delete_tempfile()` is called, an `atexit(3)` handler will close @@ -88,7 +84,6 @@ struct tempfile { volatile int fd; FILE *volatile fp; volatile pid_t owner; - char on_list; struct strbuf filename; }; From 076aa2cbda5782426c45cd65017b81d77876297a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:08 -0400 Subject: [PATCH 17/20] tempfile: auto-allocate tempfiles on heap The previous commit taught the tempfile code to give up ownership over tempfiles that have been renamed or deleted. That makes it possible to use a stack variable like this: struct tempfile t; create_tempfile(&t, ...); ... if (!err) rename_tempfile(&t, ...); else delete_tempfile(&t); But doing it this way has a high potential for creating memory errors. The tempfile we pass to create_tempfile() ends up on a global linked list, and it's not safe for it to go out of scope until we've called one of those two deactivation functions. Imagine that we add an early return from the function that forgets to call delete_tempfile(). With a static or heap tempfile variable, the worst case is that the tempfile hangs around until the program exits (and some functions like setup_shallow_temporary rely on this intentionally, creating a tempfile and then leaving it for later cleanup). But with a stack variable as above, this is a serious memory error: the variable goes out of scope and may be filled with garbage by the time the tempfile code looks at it. Let's see if we can make it harder to get this wrong. Since many callers need to allocate arbitrary numbers of tempfiles, we can't rely on static storage as a general solution. So we need to turn to the heap. We could just ask all callers to pass us a heap variable, but that puts the burden on them to call free() at the right time. Instead, let's have the tempfile code handle the heap allocation _and_ the deallocation (when the tempfile is deactivated and removed from the list). This changes the return value of all of the creation functions. For the cleanup functions (delete and rename), we'll add one extra bit of safety: instead of taking a tempfile pointer, we'll take a pointer-to-pointer and set it to NULL after freeing the object. This makes it safe to double-call functions like delete_tempfile(), as the second call treats the NULL input as a noop. Several callsites follow this pattern. The resulting patch does have a fair bit of noise, as each caller needs to be converted to handle: 1. Storing a pointer instead of the struct itself. 2. Passing the pointer instead of taking the struct address. 3. Handling a "struct tempfile *" return instead of a file descriptor. We could play games to make this less noisy. For example, by defining the tempfile like this: struct tempfile { struct heap_allocated_part_of_tempfile { int fd; ...etc } *actual_data; } Callers would continue to have a "struct tempfile", and it would be "active" only when the inner pointer was non-NULL. But that just makes things more awkward in the long run. There aren't that many callers, so we can simply bite the bullet and adjust all of them. And the compiler makes it easy for us to find them all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/gc.c | 8 ++-- credential-cache--daemon.c | 5 +-- diff.c | 15 ++++--- gpg-interface.c | 16 +++---- lockfile.c | 7 ++-- lockfile.h | 16 +++---- read-cache.c | 25 ++++++----- refs/files-backend.c | 4 +- refs/packed-backend.c | 11 ++--- shallow.c | 13 +++--- tempfile.c | 66 ++++++++++++++++------------- tempfile.h | 85 +++++++++++++++++--------------------- trailer.c | 6 +-- 13 files changed, 136 insertions(+), 141 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 3c78fcb9b1..c22787ac72 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -47,7 +47,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT; static struct argv_array prune_worktrees = ARGV_ARRAY_INIT; static struct argv_array rerere = ARGV_ARRAY_INIT; -static struct tempfile pidfile; +static struct tempfile *pidfile; static struct lock_file log_lock; static struct string_list pack_garbage = STRING_LIST_INIT_DUP; @@ -78,7 +78,7 @@ static void process_log_file(void) */ int saved_errno = errno; fprintf(stderr, _("Failed to fstat %s: %s"), - get_tempfile_path(&log_lock.tempfile), + get_tempfile_path(log_lock.tempfile), strerror(saved_errno)); fflush(stderr); commit_lock_file(&log_lock); @@ -242,7 +242,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) int fd; char *pidfile_path; - if (is_tempfile_active(&pidfile)) + if (is_tempfile_active(pidfile)) /* already locked */ return NULL; @@ -293,7 +293,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); commit_lock_file(&lock); - register_tempfile(&pidfile, pidfile_path); + pidfile = register_tempfile(pidfile_path); free(pidfile_path); return NULL; } diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 0d5c625094..4dfbc8c9f9 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -5,8 +5,6 @@ #include "unix-socket.h" #include "parse-options.h" -static struct tempfile socket_file; - struct credential_cache_entry { struct credential item; timestamp_t expiration; @@ -260,6 +258,7 @@ static void init_socket_directory(const char *path) int cmd_main(int argc, const char **argv) { + struct tempfile *socket_file; const char *socket_path; int ignore_sighup = 0; static const char *usage[] = { @@ -285,7 +284,7 @@ int cmd_main(int argc, const char **argv) die("socket directory must be an absolute path"); init_socket_directory(socket_path); - register_tempfile(&socket_file, socket_path); + socket_file = register_tempfile(socket_path); if (ignore_sighup) signal(SIGHUP, SIG_IGN); diff --git a/diff.c b/diff.c index 20f68ec389..7df2227a3c 100644 --- a/diff.c +++ b/diff.c @@ -459,7 +459,7 @@ static struct diff_tempfile { * If this diff_tempfile instance refers to a temporary file, * this tempfile object is used to manage its lifetime. */ - struct tempfile tempfile; + struct tempfile *tempfile; } diff_temp[2]; struct emit_callback { @@ -1414,7 +1414,7 @@ static void remove_tempfile(void) { int i; for (i = 0; i < ARRAY_SIZE(diff_temp); i++) { - if (is_tempfile_active(&diff_temp[i].tempfile)) + if (is_tempfile_active(diff_temp[i].tempfile)) delete_tempfile(&diff_temp[i].tempfile); diff_temp[i].name = NULL; } @@ -3720,7 +3720,6 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, const struct object_id *oid, int mode) { - int fd; struct strbuf buf = STRBUF_INIT; struct strbuf template = STRBUF_INIT; char *path_dup = xstrdup(path); @@ -3730,18 +3729,18 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, strbuf_addstr(&template, "XXXXXX_"); strbuf_addstr(&template, base); - fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1); - if (fd < 0) + temp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1); + if (!temp->tempfile) die_errno("unable to create temp-file"); if (convert_to_working_tree(path, (const char *)blob, (size_t)size, &buf)) { blob = buf.buf; size = buf.len; } - if (write_in_full(fd, blob, size) != size || - close_tempfile_gently(&temp->tempfile)) + if (write_in_full(temp->tempfile->fd, blob, size) != size || + close_tempfile_gently(temp->tempfile)) die_errno("unable to write temp-file"); - temp->name = get_tempfile_path(&temp->tempfile); + temp->name = get_tempfile_path(temp->tempfile); oid_to_hex_r(temp->hex, oid); xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode); strbuf_release(&buf); diff --git a/gpg-interface.c b/gpg-interface.c index 4ea2597ff4..4feacf16e5 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -202,17 +202,17 @@ int verify_signed_buffer(const char *payload, size_t payload_size, struct strbuf *gpg_output, struct strbuf *gpg_status) { struct child_process gpg = CHILD_PROCESS_INIT; - static struct tempfile temp; - int fd, ret; + struct tempfile *temp; + int ret; struct strbuf buf = STRBUF_INIT; - fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX"); - if (fd < 0) + temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); + if (!temp) return error_errno(_("could not create temporary file")); - if (write_in_full(fd, signature, signature_size) < 0 || - close_tempfile_gently(&temp) < 0) { + if (write_in_full(temp->fd, signature, signature_size) < 0 || + close_tempfile_gently(temp) < 0) { error_errno(_("failed writing detached signature to '%s'"), - temp.filename.buf); + temp->filename.buf); delete_tempfile(&temp); return -1; } @@ -221,7 +221,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, gpg_program, "--status-fd=1", "--keyid-format=long", - "--verify", temp.filename.buf, "-", + "--verify", temp->filename.buf, "-", NULL); if (!gpg_status) diff --git a/lockfile.c b/lockfile.c index aa69210d8b..efcb7d7dfe 100644 --- a/lockfile.c +++ b/lockfile.c @@ -72,7 +72,6 @@ static void resolve_symlink(struct strbuf *path) /* Make sure errno contains a meaningful value on error */ static int lock_file(struct lock_file *lk, const char *path, int flags) { - int fd; struct strbuf filename = STRBUF_INIT; strbuf_addstr(&filename, path); @@ -80,9 +79,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) resolve_symlink(&filename); strbuf_addstr(&filename, LOCK_SUFFIX); - fd = create_tempfile(&lk->tempfile, filename.buf); + lk->tempfile = create_tempfile(filename.buf); strbuf_release(&filename); - return fd; + return lk->tempfile ? lk->tempfile->fd : -1; } /* @@ -191,7 +190,7 @@ char *get_locked_file_path(struct lock_file *lk) { struct strbuf ret = STRBUF_INIT; - strbuf_addstr(&ret, get_tempfile_path(&lk->tempfile)); + strbuf_addstr(&ret, get_tempfile_path(lk->tempfile)); if (ret.len <= LOCK_SUFFIX_LEN || strcmp(ret.buf + ret.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX)) die("BUG: get_locked_file_path() called for malformed lock object"); diff --git a/lockfile.h b/lockfile.h index c45d2db324..6ed336e2bc 100644 --- a/lockfile.h +++ b/lockfile.h @@ -111,7 +111,7 @@ #include "tempfile.h" struct lock_file { - struct tempfile tempfile; + struct tempfile *tempfile; }; /* String appended to a filename to derive the lockfile name: */ @@ -180,7 +180,7 @@ static inline int hold_lock_file_for_update( */ static inline int is_lock_file_locked(struct lock_file *lk) { - return is_tempfile_active(&lk->tempfile); + return is_tempfile_active(lk->tempfile); } /* @@ -208,7 +208,7 @@ extern NORETURN void unable_to_lock_die(const char *path, int err); */ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) { - return fdopen_tempfile(&lk->tempfile, mode); + return fdopen_tempfile(lk->tempfile, mode); } /* @@ -217,17 +217,17 @@ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) */ static inline const char *get_lock_file_path(struct lock_file *lk) { - return get_tempfile_path(&lk->tempfile); + return get_tempfile_path(lk->tempfile); } static inline int get_lock_file_fd(struct lock_file *lk) { - return get_tempfile_fd(&lk->tempfile); + return get_tempfile_fd(lk->tempfile); } static inline FILE *get_lock_file_fp(struct lock_file *lk) { - return get_tempfile_fp(&lk->tempfile); + return get_tempfile_fp(lk->tempfile); } /* @@ -246,7 +246,7 @@ extern char *get_locked_file_path(struct lock_file *lk); */ static inline int close_lock_file_gently(struct lock_file *lk) { - return close_tempfile_gently(&lk->tempfile); + return close_tempfile_gently(lk->tempfile); } /* @@ -270,7 +270,7 @@ static inline int close_lock_file_gently(struct lock_file *lk) */ static inline int reopen_lock_file(struct lock_file *lk) { - return reopen_tempfile(&lk->tempfile); + return reopen_tempfile(lk->tempfile); } /* diff --git a/read-cache.c b/read-cache.c index c3be65f8b0..b211c57af6 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2311,7 +2311,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, return -1; if (close_tempfile_gently(tempfile)) { error(_("could not close '%s'"), tempfile->filename.buf); - delete_tempfile(tempfile); + delete_tempfile(&tempfile); return -1; } if (stat(tempfile->filename.buf, &st)) @@ -2337,7 +2337,7 @@ static int commit_locked_index(struct lock_file *lk) static int do_write_locked_index(struct index_state *istate, struct lock_file *lock, unsigned flags) { - int ret = do_write_index(istate, &lock->tempfile, 0); + int ret = do_write_index(istate, lock->tempfile, 0); if (ret) return ret; assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != @@ -2420,34 +2420,33 @@ static int clean_shared_index_files(const char *current_hex) return 0; } -static struct tempfile temporary_sharedindex; - static int write_shared_index(struct index_state *istate, struct lock_file *lock, unsigned flags) { + struct tempfile *temp; struct split_index *si = istate->split_index; - int fd, ret; + int ret; - fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX")); - if (fd < 0) { + temp = mks_tempfile(git_path("sharedindex_XXXXXX")); + if (!temp) { hashclr(si->base_sha1); return do_write_locked_index(istate, lock, flags); } move_cache_to_base_index(istate); - ret = do_write_index(si->base, &temporary_sharedindex, 1); + ret = do_write_index(si->base, temp, 1); if (ret) { - delete_tempfile(&temporary_sharedindex); + delete_tempfile(&temp); return ret; } - ret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex)); + ret = adjust_shared_perm(get_tempfile_path(temp)); if (ret) { int save_errno = errno; - error("cannot fix permission bits on %s", get_tempfile_path(&temporary_sharedindex)); - delete_tempfile(&temporary_sharedindex); + error("cannot fix permission bits on %s", get_tempfile_path(temp)); + delete_tempfile(&temp); errno = save_errno; return ret; } - ret = rename_tempfile(&temporary_sharedindex, + ret = rename_tempfile(&temp, git_path("sharedindex.%s", sha1_to_hex(si->base->sha1))); if (!ret) { hashcpy(si->base_sha1, si->base->sha1); diff --git a/refs/files-backend.c b/refs/files-backend.c index bc1899cd4a..36fe8a986b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1747,12 +1747,12 @@ static int create_symref_locked(struct files_ref_store *refs, if (!fdopen_lock_file(lock->lk, "w")) return error("unable to fdopen %s: %s", - lock->lk->tempfile.filename.buf, strerror(errno)); + lock->lk->tempfile->filename.buf, strerror(errno)); update_symref_reflog(refs, lock, refname, target, logmsg); /* no error check; commit_ref will check ferror */ - fprintf(lock->lk->tempfile.fp, "ref: %s\n", target); + fprintf(lock->lk->tempfile->fp, "ref: %s\n", target); if (commit_ref(lock) < 0) return error("unable to write symref for %s: %s", refname, strerror(errno)); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index dca887c0fe..321608a114 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -75,7 +75,7 @@ struct packed_ref_store { * "packed-refs" file. Note that this (and thus the enclosing * `packed_ref_store`) must not be freed. */ - struct tempfile tempfile; + struct tempfile *tempfile; }; struct ref_store *packed_ref_store_create(const char *path, @@ -628,7 +628,8 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) */ packed_refs_path = get_locked_file_path(&refs->lock); strbuf_addf(&sb, "%s.new", packed_refs_path); - if (create_tempfile(&refs->tempfile, sb.buf) < 0) { + refs->tempfile = create_tempfile(sb.buf); + if (!refs->tempfile) { strbuf_addf(err, "unable to create file %s: %s", sb.buf, strerror(errno)); strbuf_release(&sb); @@ -636,7 +637,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) } strbuf_release(&sb); - out = fdopen_tempfile(&refs->tempfile, "w"); + out = fdopen_tempfile(refs->tempfile, "w"); if (!out) { strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s", strerror(errno)); @@ -645,7 +646,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) { strbuf_addf(err, "error writing to %s: %s", - get_tempfile_path(&refs->tempfile), strerror(errno)); + get_tempfile_path(refs->tempfile), strerror(errno)); goto error; } @@ -657,7 +658,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) if (write_packed_entry(out, iter->refname, iter->oid->hash, peel_error ? NULL : peeled.hash)) { strbuf_addf(err, "error writing to %s: %s", - get_tempfile_path(&refs->tempfile), + get_tempfile_path(refs->tempfile), strerror(errno)); ref_iterator_abort(iter); goto error; diff --git a/shallow.c b/shallow.c index 36216febb6..1cc1c76415 100644 --- a/shallow.c +++ b/shallow.c @@ -288,19 +288,18 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol, const char *setup_temporary_shallow(const struct oid_array *extra) { - static struct tempfile temp; + struct tempfile *temp; struct strbuf sb = STRBUF_INIT; - int fd; if (write_shallow_commits(&sb, 0, extra)) { - fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX")); + temp = xmks_tempfile(git_path("shallow_XXXXXX")); - if (write_in_full(fd, sb.buf, sb.len) != sb.len || - close_tempfile_gently(&temp) < 0) + if (write_in_full(temp->fd, sb.buf, sb.len) != sb.len || + close_tempfile_gently(temp) < 0) die_errno("failed to write to %s", - get_tempfile_path(&temp)); + get_tempfile_path(temp)); strbuf_release(&sb); - return get_tempfile_path(&temp); + return get_tempfile_path(temp); } /* * is_repository_shallow() sees empty string as "no shallow diff --git a/tempfile.c b/tempfile.c index f82a5f3676..5fdafdd2d2 100644 --- a/tempfile.c +++ b/tempfile.c @@ -91,14 +91,16 @@ static void remove_tempfiles_on_signal(int signo) raise(signo); } -static void prepare_tempfile_object(struct tempfile *tempfile) +static struct tempfile *new_tempfile(void) { + struct tempfile *tempfile = xmalloc(sizeof(*tempfile)); tempfile->fd = -1; tempfile->fp = NULL; tempfile->active = 0; tempfile->owner = 0; INIT_LIST_HEAD(&tempfile->list); strbuf_init(&tempfile->filename, 0); + return tempfile; } static void activate_tempfile(struct tempfile *tempfile) @@ -124,12 +126,13 @@ static void deactivate_tempfile(struct tempfile *tempfile) tempfile->active = 0; strbuf_release(&tempfile->filename); volatile_list_del(&tempfile->list); + free(tempfile); } /* Make sure errno contains a meaningful value on error */ -int create_tempfile(struct tempfile *tempfile, const char *path) +struct tempfile *create_tempfile(const char *path) { - prepare_tempfile_object(tempfile); + struct tempfile *tempfile = new_tempfile(); strbuf_add_absolute_path(&tempfile->filename, path); tempfile->fd = open(tempfile->filename.buf, @@ -140,48 +143,47 @@ int create_tempfile(struct tempfile *tempfile, const char *path) O_RDWR | O_CREAT | O_EXCL, 0666); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); - return -1; + return NULL; } activate_tempfile(tempfile); if (adjust_shared_perm(tempfile->filename.buf)) { int save_errno = errno; error("cannot fix permission bits on %s", tempfile->filename.buf); - delete_tempfile(tempfile); + delete_tempfile(&tempfile); errno = save_errno; - return -1; + return NULL; } - return tempfile->fd; + + return tempfile; } -void register_tempfile(struct tempfile *tempfile, const char *path) +struct tempfile *register_tempfile(const char *path) { - prepare_tempfile_object(tempfile); + struct tempfile *tempfile = new_tempfile(); strbuf_add_absolute_path(&tempfile->filename, path); activate_tempfile(tempfile); + return tempfile; } -int mks_tempfile_sm(struct tempfile *tempfile, - const char *template, int suffixlen, int mode) +struct tempfile *mks_tempfile_sm(const char *template, int suffixlen, int mode) { - prepare_tempfile_object(tempfile); + struct tempfile *tempfile = new_tempfile(); strbuf_add_absolute_path(&tempfile->filename, template); tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); - return -1; + return NULL; } activate_tempfile(tempfile); - return tempfile->fd; + return tempfile; } -int mks_tempfile_tsm(struct tempfile *tempfile, - const char *template, int suffixlen, int mode) +struct tempfile *mks_tempfile_tsm(const char *template, int suffixlen, int mode) { + struct tempfile *tempfile = new_tempfile(); const char *tmpdir; - prepare_tempfile_object(tempfile); - tmpdir = getenv("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; @@ -190,25 +192,25 @@ int mks_tempfile_tsm(struct tempfile *tempfile, tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); - return -1; + return NULL; } activate_tempfile(tempfile); - return tempfile->fd; + return tempfile; } -int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode) +struct tempfile *xmks_tempfile_m(const char *template, int mode) { - int fd; + struct tempfile *tempfile; struct strbuf full_template = STRBUF_INIT; strbuf_add_absolute_path(&full_template, template); - fd = mks_tempfile_m(tempfile, full_template.buf, mode); - if (fd < 0) + tempfile = mks_tempfile_m(full_template.buf, mode); + if (!tempfile) die_errno("Unable to create temporary file '%s'", full_template.buf); strbuf_release(&full_template); - return fd; + return tempfile; } FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) @@ -281,33 +283,39 @@ int reopen_tempfile(struct tempfile *tempfile) return tempfile->fd; } -int rename_tempfile(struct tempfile *tempfile, const char *path) +int rename_tempfile(struct tempfile **tempfile_p, const char *path) { + struct tempfile *tempfile = *tempfile_p; + if (!is_tempfile_active(tempfile)) BUG("rename_tempfile called for inactive object"); if (close_tempfile_gently(tempfile)) { - delete_tempfile(tempfile); + delete_tempfile(tempfile_p); return -1; } if (rename(tempfile->filename.buf, path)) { int save_errno = errno; - delete_tempfile(tempfile); + delete_tempfile(tempfile_p); errno = save_errno; return -1; } deactivate_tempfile(tempfile); + *tempfile_p = NULL; return 0; } -void delete_tempfile(struct tempfile *tempfile) +void delete_tempfile(struct tempfile **tempfile_p) { + struct tempfile *tempfile = *tempfile_p; + if (!is_tempfile_active(tempfile)) return; close_tempfile_gently(tempfile); unlink_or_warn(tempfile->filename.buf); deactivate_tempfile(tempfile); + *tempfile_p = NULL; } diff --git a/tempfile.h b/tempfile.h index e32b4df092..b8f4b5e145 100644 --- a/tempfile.h +++ b/tempfile.h @@ -17,22 +17,18 @@ * * The caller: * - * * Allocates a `struct tempfile`. Once the structure is passed to - * `create_tempfile()`, its storage must remain valid until - * `delete_tempfile()` or `rename_tempfile()` is called on it. - * * * Attempts to create a temporary file by calling - * `create_tempfile()`. + * `create_tempfile()`. The resources used for the temporary file are + * managed by the tempfile API. * * * Writes new content to the file by either: * - * * writing to the file descriptor returned by `create_tempfile()` - * (also available via `tempfile->fd`). + * * writing to the `tempfile->fd` file descriptor * * * calling `fdopen_tempfile()` to get a `FILE` pointer for the * open file and writing to the file using stdio. * - * Note that the file descriptor returned by create_tempfile() + * Note that the file descriptor created by create_tempfile() * is marked O_CLOEXEC, so the new contents must be written by * the current process, not any spawned one. * @@ -50,7 +46,7 @@ * `delete_tempfile()` or `rename_tempfile()`. * * After the temporary file is renamed or deleted, the `tempfile` - * object may be reused or freed. + * object is no longer valid and should not be reused. * * If the program exits before `rename_tempfile()` or * `delete_tempfile()` is called, an `atexit(3)` handler will close @@ -69,8 +65,8 @@ * Error handling * -------------- * - * `create_tempfile()` returns a file descriptor on success or -1 on - * failure. On errors, `errno` describes the reason for failure. + * `create_tempfile()` returns an allocated tempfile on success or NULL + * on failure. On errors, `errno` describes the reason for failure. * * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()` * return 0 on success. On failure they set `errno` appropriately and return @@ -89,10 +85,10 @@ struct tempfile { /* * Attempt to create a temporary file at the specified `path`. Return - * a file descriptor for writing to it, or -1 on error. It is an error - * if a file already exists at that path. + * a tempfile (whose "fd" member can be used for writing to it), or + * NULL on error. It is an error if a file already exists at that path. */ -extern int create_tempfile(struct tempfile *tempfile, const char *path); +extern struct tempfile *create_tempfile(const char *path); /* * Register an existing file as a tempfile, meaning that it will be @@ -100,7 +96,7 @@ extern int create_tempfile(struct tempfile *tempfile, const char *path); * but it can be worked with like any other closed tempfile (for * example, it can be opened using reopen_tempfile()). */ -extern void register_tempfile(struct tempfile *tempfile, const char *path); +extern struct tempfile *register_tempfile(const char *path); /* @@ -132,70 +128,65 @@ extern void register_tempfile(struct tempfile *tempfile, const char *path); * know the (absolute) path of the file that was created, it can be * read from tempfile->filename. * - * On success, the functions return a file descriptor that is open for - * writing the temporary file. On errors, they return -1 and set errno - * appropriately (except for the "x" variants, which die() on errors). + * On success, the functions return a tempfile whose "fd" member is open + * for writing the temporary file. On errors, they return NULL and set + * errno appropriately (except for the "x" variants, which die() on + * errors). */ /* See "mks_tempfile functions" above. */ -extern int mks_tempfile_sm(struct tempfile *tempfile, - const char *template, int suffixlen, int mode); +extern struct tempfile *mks_tempfile_sm(const char *template, + int suffixlen, int mode); /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile_s(struct tempfile *tempfile, - const char *template, int suffixlen) +static inline struct tempfile *mks_tempfile_s(const char *template, + int suffixlen) { - return mks_tempfile_sm(tempfile, template, suffixlen, 0600); + return mks_tempfile_sm(template, suffixlen, 0600); } /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile_m(struct tempfile *tempfile, - const char *template, int mode) +static inline struct tempfile *mks_tempfile_m(const char *template, int mode) { - return mks_tempfile_sm(tempfile, template, 0, mode); + return mks_tempfile_sm(template, 0, mode); } /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile(struct tempfile *tempfile, - const char *template) +static inline struct tempfile *mks_tempfile(const char *template) { - return mks_tempfile_sm(tempfile, template, 0, 0600); + return mks_tempfile_sm(template, 0, 0600); } /* See "mks_tempfile functions" above. */ -extern int mks_tempfile_tsm(struct tempfile *tempfile, - const char *template, int suffixlen, int mode); +extern struct tempfile *mks_tempfile_tsm(const char *template, + int suffixlen, int mode); /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile_ts(struct tempfile *tempfile, - const char *template, int suffixlen) +static inline struct tempfile *mks_tempfile_ts(const char *template, + int suffixlen) { - return mks_tempfile_tsm(tempfile, template, suffixlen, 0600); + return mks_tempfile_tsm(template, suffixlen, 0600); } /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile_tm(struct tempfile *tempfile, - const char *template, int mode) +static inline struct tempfile *mks_tempfile_tm(const char *template, int mode) { - return mks_tempfile_tsm(tempfile, template, 0, mode); + return mks_tempfile_tsm(template, 0, mode); } /* See "mks_tempfile functions" above. */ -static inline int mks_tempfile_t(struct tempfile *tempfile, - const char *template) +static inline struct tempfile *mks_tempfile_t(const char *template) { - return mks_tempfile_tsm(tempfile, template, 0, 0600); + return mks_tempfile_tsm(template, 0, 0600); } /* See "mks_tempfile functions" above. */ -extern int xmks_tempfile_m(struct tempfile *tempfile, - const char *template, int mode); +extern struct tempfile *xmks_tempfile_m(const char *template, int mode); /* See "mks_tempfile functions" above. */ -static inline int xmks_tempfile(struct tempfile *tempfile, - const char *template) +static inline struct tempfile *xmks_tempfile(const char *template) { - return xmks_tempfile_m(tempfile, template, 0600); + return xmks_tempfile_m(template, 0600); } /* @@ -257,7 +248,7 @@ extern int reopen_tempfile(struct tempfile *tempfile); * `delete_tempfile()` for a `tempfile` object that has already been * deleted or renamed. */ -extern void delete_tempfile(struct tempfile *tempfile); +extern void delete_tempfile(struct tempfile **tempfile_p); /* * Close the file descriptor and/or file pointer if they are still @@ -268,6 +259,6 @@ extern void delete_tempfile(struct tempfile *tempfile); * `rename(2)`. It is a bug to call `rename_tempfile()` for a * `tempfile` object that is not currently active. */ -extern int rename_tempfile(struct tempfile *tempfile, const char *path); +extern int rename_tempfile(struct tempfile **tempfile_p, const char *path); #endif /* TEMPFILE_H */ diff --git a/trailer.c b/trailer.c index c30e3a0c04..3ba157ed0d 100644 --- a/trailer.c +++ b/trailer.c @@ -995,7 +995,7 @@ static void free_all(struct list_head *head) } } -static struct tempfile trailers_tempfile; +static struct tempfile *trailers_tempfile; static FILE *create_in_place_tempfile(const char *file) { @@ -1017,9 +1017,9 @@ static FILE *create_in_place_tempfile(const char *file) strbuf_add(&template, file, tail - file + 1); strbuf_addstr(&template, "git-interpret-trailers-XXXXXX"); - xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode); + trailers_tempfile = xmks_tempfile_m(template.buf, st.st_mode); strbuf_release(&template); - outfile = fdopen_tempfile(&trailers_tempfile, "w"); + outfile = fdopen_tempfile(trailers_tempfile, "w"); if (!outfile) die_errno(_("could not open temporary file")); From 5e7f01c93e510c9c8d775665324abba15f05c3ff Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:12 -0400 Subject: [PATCH 18/20] lockfile: update lifetime requirements in documentation Now that the tempfile system we rely on has loosened the lifetime requirements for storage, we can adjust our documentation to match. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- lockfile.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lockfile.h b/lockfile.h index 6ed336e2bc..7c1c484d7c 100644 --- a/lockfile.h +++ b/lockfile.h @@ -37,12 +37,12 @@ * * The caller: * - * * Allocates a `struct lock_file` either as a static variable or on - * the heap, initialized to zeros. Once you use the structure to - * call the `hold_lock_file_for_*()` family of functions, it belongs - * to the lockfile subsystem and its storage must remain valid - * throughout the life of the program (i.e. you cannot use an - * on-stack variable to hold this structure). + * * Allocates a `struct lock_file` with whatever storage duration you + * desire. The struct does not have to be initialized before being + * used, but it is good practice to do so using by setting it to + * all-zeros (or using the LOCK_INIT macro). This puts the object in a + * consistent state that allows you to call rollback_lock_file() even + * if the lock was never taken (in which case it is a noop). * * * Attempts to create a lockfile by calling `hold_lock_file_for_update()`. * @@ -73,10 +73,8 @@ * `commit_lock_file()`, `commit_lock_file_to()`, * `rollback_lock_file()`, or `reopen_lock_file()`. * - * Even after the lockfile is committed or rolled back, the - * `lock_file` object must not be freed or altered by the caller. - * However, it may be reused; just pass it to another call of - * `hold_lock_file_for_update()`. + * After the lockfile is committed or rolled back, the `lock_file` + * object can be discarded or reused. * * If the program exits before `commit_lock_file()`, * `commit_lock_file_to()`, or `rollback_lock_file()` is called, the @@ -114,6 +112,8 @@ struct lock_file { struct tempfile *tempfile; }; +#define LOCK_INIT { NULL } + /* String appended to a filename to derive the lockfile name: */ #define LOCK_SUFFIX ".lock" #define LOCK_SUFFIX_LEN 5 From ee4d8e455c0981269213eccfd788a9bb9adf77d9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:15 -0400 Subject: [PATCH 19/20] ref_lock: stop leaking lock_files Since the tempfile code recently relaxed the rule that tempfile structs (and thus locks) need to hang around forever, we no longer have to leak our lock_file structs. In fact, we don't even need to heap-allocate them anymore, since their lifetime can just match that of the surrounding ref_lock (and if we forget to delete a lock, the effect is the same as before: it will eventually go away at program exit). Note that there is a check in unlock_ref() to only rollback a lock file if it has been allocated. We don't need that check anymore; we zero the ref_lock (and thus the lock_file), so at worst we pass a NULL pointer to delete_tempfile(), which considers that a noop. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 36fe8a986b..f3455609d6 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -12,7 +12,7 @@ struct ref_lock { char *ref_name; - struct lock_file *lk; + struct lock_file lk; struct object_id old_oid; }; @@ -418,9 +418,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, static void unlock_ref(struct ref_lock *lock) { - /* Do not free lock->lk -- atexit() still looks at them */ - if (lock->lk) - rollback_lock_file(lock->lk); + rollback_lock_file(&lock->lk); free(lock->ref_name); free(lock); } @@ -534,11 +532,8 @@ static int lock_raw_ref(struct files_ref_store *refs, goto error_return; } - if (!lock->lk) - lock->lk = xcalloc(1, sizeof(struct lock_file)); - if (hold_lock_file_for_update_timeout( - lock->lk, ref_file.buf, LOCK_NO_DEREF, + &lock->lk, ref_file.buf, LOCK_NO_DEREF, get_files_ref_lock_timeout_ms()) < 0) { if (errno == ENOENT && --attempts_remaining > 0) { /* @@ -949,11 +944,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, goto error_return; } - lock->lk = xcalloc(1, sizeof(struct lock_file)); - lock->ref_name = xstrdup(refname); - if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) { + if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) { last_errno = errno; unable_to_lock_message(ref_file.buf, errno, err); goto error_return; @@ -1404,14 +1397,14 @@ static int files_rename_ref(struct ref_store *ref_store, static int close_ref_gently(struct ref_lock *lock) { - if (close_lock_file_gently(lock->lk)) + if (close_lock_file_gently(&lock->lk)) return -1; return 0; } static int commit_ref(struct ref_lock *lock) { - char *path = get_locked_file_path(lock->lk); + char *path = get_locked_file_path(&lock->lk); struct stat st; if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { @@ -1435,7 +1428,7 @@ static int commit_ref(struct ref_lock *lock) free(path); } - if (commit_lock_file(lock->lk)) + if (commit_lock_file(&lock->lk)) return -1; return 0; } @@ -1627,12 +1620,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock, unlock_ref(lock); return -1; } - fd = get_lock_file_fd(lock->lk); + fd = get_lock_file_fd(&lock->lk); if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_in_full(fd, &term, 1) != 1 || close_ref_gently(lock) < 0) { strbuf_addf(err, - "couldn't write '%s'", get_lock_file_path(lock->lk)); + "couldn't write '%s'", get_lock_file_path(&lock->lk)); unlock_ref(lock); return -1; } @@ -1709,7 +1702,7 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target) { int ret = -1; #ifndef NO_SYMLINK_HEAD - char *ref_path = get_locked_file_path(lock->lk); + char *ref_path = get_locked_file_path(&lock->lk); unlink(ref_path); ret = symlink(target, ref_path); free(ref_path); @@ -1745,14 +1738,14 @@ static int create_symref_locked(struct files_ref_store *refs, return 0; } - if (!fdopen_lock_file(lock->lk, "w")) + if (!fdopen_lock_file(&lock->lk, "w")) return error("unable to fdopen %s: %s", - lock->lk->tempfile->filename.buf, strerror(errno)); + lock->lk.tempfile->filename.buf, strerror(errno)); update_symref_reflog(refs, lock, refname, target, logmsg); /* no error check; commit_ref will check ferror */ - fprintf(lock->lk->tempfile->fp, "ref: %s\n", target); + fprintf(lock->lk.tempfile->fp, "ref: %s\n", target); if (commit_ref(lock) < 0) return error("unable to write symref for %s: %s", refname, strerror(errno)); @@ -2853,12 +2846,12 @@ static int files_reflog_expire(struct ref_store *ref_store, strerror(errno)); rollback_lock_file(&reflog_lock); } else if (update && - (write_in_full(get_lock_file_fd(lock->lk), + (write_in_full(get_lock_file_fd(&lock->lk), oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || - write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || + write_str_in_full(get_lock_file_fd(&lock->lk), "\n") != 1 || close_ref_gently(lock) < 0)) { status |= error("couldn't write %s", - get_lock_file_path(lock->lk)); + get_lock_file_path(&lock->lk)); rollback_lock_file(&reflog_lock); } else if (commit_lock_file(&reflog_lock)) { status |= error("unable to write reflog '%s' (%s)", From bfffb48c5d520a5f181716b0330774a03cd3fa39 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 5 Sep 2017 08:15:21 -0400 Subject: [PATCH 20/20] stop leaking lock structs in some simple cases Now that it's safe to declare a "struct lock_file" on the stack, we can do so (and avoid an intentional leak). These leaks were found by running t0000 and t0001 under valgrind (though certainly other similar leaks exist and just don't happen to be exercised by those tests). Initializing the lock_file's inner tempfile with NULL is not strictly necessary in these cases, but it's a good practice to model. It means that if we were to call a function like rollback_lock_file() on a lock that was never taken in the first place, it becomes a quiet noop (rather than undefined behavior). Likewise, it's always safe to rollback_lock_file() on a file that has already been committed or deleted, since that operation is a noop on an inactive lockfile (and that's why the case in config.c can drop the "if (lock)" check as we move away from using a pointer). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/reset.c | 6 +++--- builtin/update-index.c | 11 ++++------- cache-tree.c | 14 ++++---------- config.c | 24 +++++++----------------- 4 files changed, 18 insertions(+), 37 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index d72c7d1c96..f1af9345e4 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -367,8 +367,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die_if_unmerged_cache(reset_type); if (reset_type != SOFT) { - struct lock_file *lock = xcalloc(1, sizeof(*lock)); - hold_locked_index(lock, LOCK_DIE_ON_ERROR); + struct lock_file lock = LOCK_INIT; + hold_locked_index(&lock, LOCK_DIE_ON_ERROR); if (reset_type == MIXED) { int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN; if (read_from_tree(&pathspec, &oid, intent_to_add)) @@ -384,7 +384,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) die(_("Could not reset index file to revision '%s'."), rev); } - if (write_locked_index(&the_index, lock, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock, COMMIT_LOCK)) die(_("Could not write new index file.")); } diff --git a/builtin/update-index.c b/builtin/update-index.c index d562f2ec69..655e6d60d3 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -915,7 +915,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) struct refresh_params refresh_args = {0, &has_errors}; int lock_error = 0; int split_index = -1; - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; struct parse_opt_ctx_t ctx; strbuf_getline_fn getline_fn; int parseopt_state = PARSE_OPT_UNKNOWN; @@ -1014,11 +1014,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); - /* We can't free this memory, it becomes part of a linked list parsed atexit() */ - lock_file = xcalloc(1, sizeof(struct lock_file)); - /* we will diagnose later if it turns out that we need to update it */ - newfd = hold_locked_index(lock_file, 0); + newfd = hold_locked_index(&lock_file, 0); if (newfd < 0) lock_error = errno; @@ -1153,11 +1150,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) exit(128); unable_to_lock_die(get_index_file(), lock_error); } - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die("Unable to write new index file"); } - rollback_lock_file(lock_file); + rollback_lock_file(&lock_file); return has_errors ? 1 : 0; } diff --git a/cache-tree.c b/cache-tree.c index 2690113a6a..71d092ed51 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -603,16 +603,10 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix) { int entries, was_valid, newfd; - struct lock_file *lock_file; + struct lock_file lock_file = LOCK_INIT; int ret = 0; - /* - * We can't free this memory, it becomes part of a linked list - * parsed atexit() - */ - lock_file = xcalloc(1, sizeof(struct lock_file)); - - newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR); + newfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); entries = read_index_from(index_state, index_path); if (entries < 0) { @@ -632,7 +626,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co goto out; } if (0 <= newfd) { - if (!write_locked_index(index_state, lock_file, COMMIT_LOCK)) + if (!write_locked_index(index_state, &lock_file, COMMIT_LOCK)) newfd = -1; } /* Not being able to write is fine -- we are only interested @@ -657,7 +651,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co out: if (0 <= newfd) - rollback_lock_file(lock_file); + rollback_lock_file(&lock_file); return ret; } diff --git a/config.c b/config.c index d0d8ce823a..7931182a54 100644 --- a/config.c +++ b/config.c @@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, { int fd = -1, in_fd = -1; int ret; - struct lock_file *lock = NULL; + struct lock_file lock = LOCK_INIT; char *filename_buf = NULL; char *contents = NULL; size_t contents_sz; @@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, * The lock serves a purpose in addition to locking: the new * contents of .git/config will be written into it. */ - lock = xcalloc(1, sizeof(struct lock_file)); - fd = hold_lock_file_for_update(lock, config_filename, 0); + fd = hold_lock_file_for_update(&lock, config_filename, 0); if (fd < 0) { error_errno("could not lock config file %s", config_filename); free(store.key); @@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, close(in_fd); in_fd = -1; - if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) { - error_errno("chmod on %s failed", get_lock_file_path(lock)); + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) { + error_errno("chmod on %s failed", get_lock_file_path(&lock)); ret = CONFIG_NO_WRITE; goto out_free; } @@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, contents = NULL; } - if (commit_lock_file(lock) < 0) { + if (commit_lock_file(&lock) < 0) { error_errno("could not write config file %s", config_filename); ret = CONFIG_NO_WRITE; - lock = NULL; goto out_free; } - /* - * 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; /* Invalidate the config cache */ git_config_clear(); out_free: - if (lock) - rollback_lock_file(lock); + rollback_lock_file(&lock); free(filename_buf); if (contents) munmap(contents, contents_sz); @@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, return ret; write_err_out: - ret = write_error(get_lock_file_path(lock)); + ret = write_error(get_lock_file_path(&lock)); goto out_free; }