{"thread":{"id":"46703","subject":"[PATCH 0/20] allowing struct lock_file to be deleted","startedAt":"2017-09-05T12:14:03Z","lastAt":"2017-09-05T12:16:08Z","messageCount":21,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"327548","messageId":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","threadId":"46703","inReplyTo":null,"subject":"[PATCH 0/20] allowing struct lock_file to be deleted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:13:53Z","receivedAt":"2017-09-05T12:14:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the beginning of time, our code base has had the rule that the\nstorage for a \"struct lock_file\" can never go out of scope once it has\nbeen used. This leads to lots of intentional leaks of these structs.\n\nI think it's unlikely that any of the leaks produces truly terrible\nbehavior, simply because we tend to do a limited number of operations\nfrom a given program (and the structs aren't that big to begin with).\nBut they make finding _actual_ leaks much harder by polluting\nleak-checker output.\n\nAnd I have to admit that they've just always bugged me.\n\nThis series make it possible to free them (and in IMHO makes it harder\nto misuse the API in general). I also found a number of interesting\ncorner cases that turned out to be bugs in the existing code (albeit\nquite minor). I've floated the existing fixes to the front, followed by\nsome preparatory cleanups. The meat is in patches 17 and 18, and then\nthe last few patches use the new system to clean up some leaks.\n\nThere are almost certainly more leaks that can be cleaned. I didn't do\nan exhaustive search, but just covered all the ones triggered by t0000\nand t0001 (which I've been doggedly trying to get to run clean in a\nleak-checker; more on that in a moment).\n\n [01/20]: write_index_as_tree: cleanup tempfile on error\n [02/20]: setup_temporary_shallow: avoid using inactive tempfile\n [03/20]: setup_temporary_shallow: move tempfile struct into function\n [04/20]: verify_signed_buffer: prefer close_tempfile() to close()\n [05/20]: always check return value of close_tempfile\n [06/20]: tempfile: do not delete tempfile on failed close\n [07/20]: lockfile: do not rollback lock on failed close\n [08/20]: tempfile: prefer is_tempfile_active to bare access\n [09/20]: tempfile: handle NULL tempfile pointers gracefully\n [10/20]: tempfile: replace die(\"BUG\") with BUG()\n [11/20]: tempfile: factor out activation\n [12/20]: tempfile: factor out deactivation\n [13/20]: tempfile: robustify cleanup handler\n [14/20]: tempfile: release deactivated strbufs instead of resetting\n [15/20]: tempfile: use list.h for linked list\n [16/20]: tempfile: remove deactivated list entries\n [17/20]: tempfile: auto-allocate tempfiles on heap\n [18/20]: lockfile: update lifetime requirements in documentation\n [19/20]: ref_lock: stop leaking lock_files\n [20/20]: stop leaking lock structs in some simple cases\n\n builtin/gc.c | 8 +-\n builtin/reset.c | 6 +-\n builtin/update-index.c | 11 +--\n cache-tree.c | 37 ++++----\n config.c | 24 ++---\n credential-cache--daemon.c | 5 +-\n diff.c | 15 ++-\n gpg-interface.c | 16 ++--\n list.h | 38 ++++++++\n lockfile.c | 7 +-\n lockfile.h | 58 ++++++------\n read-cache.c | 32 ++++---\n refs/files-backend.c | 50 +++++-----\n refs/packed-backend.c | 14 +--\n shallow.c | 16 ++--\n tempfile.c | 232 +++++++++++++++++++++++----------------------\n tempfile.h | 123 +++++++++++-------------\n trailer.c | 6 +-\n 18 files changed, 358 insertions(+), 340 deletions(-)\n\n-Peff\n"},{"id":"327549","messageId":"20170905121407.4hom32etywlzaa26@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 01/20] write_index_as_tree: cleanup tempfile on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:07Z","receivedAt":"2017-09-05T12:14:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we failed to write our new index file, we rollback our\nlockfile to remove the temporary index. But if we fail\nbefore we even get to the write step (because reading the\nold index failed), we leave the lockfile in place, which\nmakes no sense.\n\nIn practice this hasn't been a big deal because failing at\nwrite_index_as_tree() typically results in the whole program\nexiting (and thus the tempfile handler kicking in and\ncleaning up the files). But this function should\nconsistently take responsibility for the resources it\nallocates.\n\nSigned-off-by: Jeff King \n---\n cache-tree.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2440d1dc89..2690113a6a 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -604,6 +604,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n {\n \tint entries, was_valid, newfd;\n \tstruct lock_file *lock_file;\n+\tint ret = 0;\n \n \t/*\n \t * We can't free this memory, it becomes part of a linked list\n@@ -614,8 +615,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \tnewfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n-\tif (entries < 0)\n-\t\treturn WRITE_TREE_UNREADABLE_INDEX;\n+\tif (entries < 0) {\n+\t\tret = WRITE_TREE_UNREADABLE_INDEX;\n+\t\tgoto out;\n+\t}\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE)\n \t\tcache_tree_free(&index_state->cache_tree);\n \n@@ -624,8 +627,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \n \twas_valid = cache_tree_fully_valid(index_state->cache_tree);\n \tif (!was_valid) {\n-\t\tif (cache_tree_update(index_state, flags) < 0)\n-\t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n+\t\tif (cache_tree_update(index_state, flags) < 0) {\n+\t\t\tret = WRITE_TREE_UNMERGED_INDEX;\n+\t\t\tgoto out;\n+\t\t}\n \t\tif (0 <= newfd) {\n \t\t\tif (!write_locked_index(index_state, lock_file, COMMIT_LOCK))\n \t\t\t\tnewfd = -1;\n@@ -641,17 +646,19 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \tif (prefix) {\n \t\tstruct cache_tree *subtree;\n \t\tsubtree = cache_tree_find(index_state->cache_tree, prefix);\n-\t\tif (!subtree)\n-\t\t\treturn WRITE_TREE_PREFIX_ERROR;\n+\t\tif (!subtree) {\n+\t\t\tret = WRITE_TREE_PREFIX_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \t\thashcpy(sha1, subtree->oid.hash);\n \t}\n \telse\n \t\thashcpy(sha1, index_state->cache_tree->oid.hash);\n \n+out:\n \tif (0 <= newfd)\n \t\trollback_lock_file(lock_file);\n-\n-\treturn 0;\n+\treturn ret;\n }\n \n int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327550","messageId":"20170905121415.xo64jttpbzae7fsj@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:16Z","receivedAt":"2017-09-05T12:14:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When there are no shallow entries to write, we skip creating\nthe tempfile entirely and try to return the empty string.\n\nBut we do so by calling get_tempfile_path() on the inactive\ntempfile object. This will trigger an assertion that kills\nthe program. The bug was introduced by 6e122b449b\n(setup_temporary_shallow(): use tempfile module,\n2015-08-10). But nobody seems to have noticed since then\nbecause we do not end up calling this function at all when\nthere are no shallow items. In other words, this code path\nis completely unexercised.\n\nSince the tempfile object is a static global, it _is_\npossible that we call the function twice, writing out\nshallow info the first time and then \"reusing\" our tempfile\nobject the second time. But:\n\n 1. It seems unlikely that this was the intent, as hitting\n this code path would imply somebody clearing the\n shallow_info list between calls.\n\n And if somebody _did_ call the function multiple times\n without clearing the shallow_info list, we'd hit a\n different BUG for trying to reuse an already-active\n tempfile.\n\n 2. I verified by code inspection that the function is only\n called once per program. And also replacing this code\n with a BUG() and running the test suite demonstrates\n that it is not triggered there.\n\nSo we could probably just replace this with an assertion and\nconfirm that it's never called. However, the original intent\ndoes seem to be that you _could_ call it when the\nshallow_info is empty. And that's easy enough to do; since\nthe return value doesn't need to point to a writable buffer,\nwe can just return a string literal.\n\nSigned-off-by: Jeff King \n---\n shallow.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex f5591e56da..29194b475a 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -307,7 +307,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \t * is_repository_shallow() sees empty string as \"no shallow\n \t * file\".\n \t */\n-\treturn get_tempfile_path(&temporary_shallow);\n+\treturn \"\";\n }\n \n void setup_alternate_shallow(struct lock_file *shallow_lock,\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327551","messageId":"20170905121419.kghzrffwgvuvrwaf@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 03/20] setup_temporary_shallow: move tempfile struct into function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:19Z","receivedAt":"2017-09-05T12:14:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The setup_temporary_shallow() function creates a temporary\nfile, but we never access the tempfile struct outside of the\nfunction. This is OK, since it means we'll just clean up the\ntempfile on exit. But we can simplify the code a bit by\nmoving the global tempfile struct to the only function in\nwhich it's used.\n\nNote that it must remain \"static\" due to tempfile.c's\nrequirement that tempfile storage never goes away until\nprogram exit.\n\nSigned-off-by: Jeff King \n---\n shallow.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex 29194b475a..c7fd68ace0 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -286,22 +286,21 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \treturn write_shallow_commits_1(out, use_pack_protocol, extra, 0);\n }\n \n-static struct tempfile temporary_shallow;\n-\n const char *setup_temporary_shallow(const struct oid_array *extra)\n {\n+\tstatic struct tempfile temp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\tfd = xmks_tempfile(&temporary_shallow, git_path(\"shallow_XXXXXX\"));\n+\t\tfd = xmks_tempfile(&temp, git_path(\"shallow_XXXXXX\"));\n \n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t get_tempfile_path(&temporary_shallow));\n-\t\tclose_tempfile(&temporary_shallow);\n+\t\t\t\t get_tempfile_path(&temp));\n+\t\tclose_tempfile(&temp);\n \t\tstrbuf_release(&sb);\n-\t\treturn get_tempfile_path(&temporary_shallow);\n+\t\treturn get_tempfile_path(&temp);\n \t}\n \t/*\n \t * is_repository_shallow() sees empty string as \"no shallow\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327552","messageId":"20170905121422.sf467ufqkone3xn7@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:23Z","receivedAt":"2017-09-05T12:14:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do a manual close() on the descriptor provided to us by\nmks_tempfile. But this runs contrary to the advice in\ntempfile.h, which notes that you should always use\nclose_tempfile(). Otherwise the descriptor may be reused\nwithout the tempfile object knowing it, and the later call\nto delete_tempfile() could close a random descriptor.\n\nSigned-off-by: Jeff King \n---\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex d936f3a32f..455b6c04b4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -215,7 +215,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\tdelete_tempfile(&temp);\n \t\treturn -1;\n \t}\n-\tclose(fd);\n+\tclose_tempfile(&temp);\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327553","messageId":"20170905121426.sc4u4unmwzhamjsa@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 05/20] always check return value of close_tempfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:26Z","receivedAt":"2017-09-05T12:14:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If close_tempfile() encounters an error, then it deletes the\ntempfile and resets the \"struct tempfile\". But many code\npaths ignore the return value and continue to use the\ntempfile. Instead, we should generally treat this the same\nas a write() error.\n\nNote that in the postimage of some of these cases our error\nmessage will be bogus after a failed close because we look\nat tempfile->filename (either directly or via get_tempfile_path).\nBut after the failed close resets the tempfile object, this\nis guaranteed to be the empty string. That will be addressed\nin a future patch (because there are many more cases of the\nsame problem than just these instances).\n\nNote also in the hunk in gpg-interface.c that it's fine to\ncall delete_tempfile() in the error path, even if\nclose_tempfile() failed and already deleted the file. The\ntempfile code is smart enough to know the second deletion is\na noop.\n\nSigned-off-by: Jeff King \n---\n diff.c | 4 ++--\n gpg-interface.c | 4 ++--\n shallow.c | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3d3e553a98..fdb974014b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3738,9 +3738,9 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\tblob = buf.buf;\n \t\tsize = buf.len;\n \t}\n-\tif (write_in_full(fd, blob, size) != size)\n+\tif (write_in_full(fd, blob, size) != size ||\n+\t close_tempfile(&temp->tempfile))\n \t\tdie_errno(\"unable to write temp-file\");\n-\tclose_tempfile(&temp->tempfile);\n \ttemp->name = get_tempfile_path(&temp->tempfile);\n \toid_to_hex_r(temp->hex, oid);\n \txsnprintf(temp->mode, sizeof(temp->mode), \"%06o\", mode);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 455b6c04b4..05ca6ecbfd 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -209,13 +209,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tfd = mks_tempfile_t(&temp, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n \t\treturn error_errno(_(\"could not create temporary file\"));\n-\tif (write_in_full(fd, signature, signature_size) < 0) {\n+\tif (write_in_full(fd, signature, signature_size) < 0 ||\n+\t close_tempfile(&temp) < 0) {\n \t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n \t\t\t temp.filename.buf);\n \t\tdelete_tempfile(&temp);\n \t\treturn -1;\n \t}\n-\tclose_tempfile(&temp);\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\ndiff --git a/shallow.c b/shallow.c\nindex c7fd68ace0..f49e6ae122 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -295,10 +295,10 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tfd = xmks_tempfile(&temp, git_path(\"shallow_XXXXXX\"));\n \n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len ||\n+\t\t close_tempfile(&temp) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_tempfile_path(&temp));\n-\t\tclose_tempfile(&temp);\n \t\tstrbuf_release(&sb);\n \t\treturn get_tempfile_path(&temp);\n \t}\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327554","messageId":"20170905121429.3qpx7ygtx6dpqnmm@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 06/20] tempfile: do not delete tempfile on failed close","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:30Z","receivedAt":"2017-09-05T12:14:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When close_tempfile() fails, we delete the tempfile and\nreset the fields of the tempfile struct. This makes it\neasier for callers to return without cleaning up, but it\nalso makes this common pattern:\n\n if (close_tempfile(tempfile))\n\treturn error_errno(\"error closing %s\", tempfile->filename.buf);\n\nwrong, because the \"filename\" field has been reset after the\nfailed close. And it's not easy to fix, as in many cases we\ndon't have another copy of the filename (e.g., if it was\ncreated via one of the mks_tempfile functions, and we just\nhave the original template string).\n\nLet's drop the feature that a failed close automatically\ndeletes the file. This puts the burden on the caller to do\nthe deletion themselves, but this isn't that big a deal.\nCallers which do:\n\n if (write(...) || close_tempfile(...)) {\n\tdelete_tempfile(...);\n\treturn -1;\n }\n\nalready had to call delete when the write() failed, and so\naren't affected. Likewise, any caller which just calls die()\nin the error path is OK; we'll delete the tempfile during\nthe atexit handler.\n\nBecause this patch changes the semantics of close_tempfile()\nwithout changing its signature, all callers need to be\nmanually checked and converted to the new scheme. This patch\ncovers all in-tree callers, but there may be others for\nnot-yet-merged topics. To catch these, we rename the\nfunction to close_tempfile_gently(), which will attract\ncompile-time attention to new callers. (Technically the\noriginal could be considered \"gentle\" already in that it\ndidn't die() on errors, but this one is even more so).\n\nSigned-off-by: Jeff King \n---\n diff.c | 2 +-\n gpg-interface.c | 2 +-\n lockfile.h | 8 +++++++-\n read-cache.c | 7 +++++--\n shallow.c | 2 +-\n tempfile.c | 31 ++++++++++++-------------------\n tempfile.h | 25 +++++++++++++------------\n 7 files changed, 40 insertions(+), 37 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex fdb974014b..20f68ec389 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3739,7 +3739,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\tsize = buf.len;\n \t}\n \tif (write_in_full(fd, blob, size) != size ||\n-\t close_tempfile(&temp->tempfile))\n+\t close_tempfile_gently(&temp->tempfile))\n \t\tdie_errno(\"unable to write temp-file\");\n \ttemp->name = get_tempfile_path(&temp->tempfile);\n \toid_to_hex_r(temp->hex, oid);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 05ca6ecbfd..4ea2597ff4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -210,7 +210,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tif (fd < 0)\n \t\treturn error_errno(_(\"could not create temporary file\"));\n \tif (write_in_full(fd, signature, signature_size) < 0 ||\n-\t close_tempfile(&temp) < 0) {\n+\t close_tempfile_gently(&temp) < 0) {\n \t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n \t\t\t temp.filename.buf);\n \t\tdelete_tempfile(&temp);\ndiff --git a/lockfile.h b/lockfile.h\nindex 572064939c..dd4259bc40 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -246,7 +246,13 @@ extern char *get_locked_file_path(struct lock_file *lk);\n */\n static inline int close_lock_file(struct lock_file *lk)\n {\n-\treturn close_tempfile(&lk->tempfile);\n+\tint ret = close_tempfile_gently(&lk->tempfile);\n+\tif (ret) {\n+\t\tint saved_errno = errno;\n+\t\tdelete_tempfile(&lk->tempfile);\n+\t\terrno = saved_errno;\n+\t}\n+\treturn ret;\n }\n \n /*\ndiff --git a/read-cache.c b/read-cache.c\nindex 40da87ea71..51686518e0 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2309,8 +2309,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tif (ce_flush(&c, newfd, istate->sha1))\n \t\treturn -1;\n-\tif (close_tempfile(tempfile))\n-\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n+\tif (close_tempfile_gently(tempfile)) {\n+\t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n+\t\tdelete_tempfile(tempfile);\n+\t\treturn -1;\n+\t}\n \tif (stat(tempfile->filename.buf, &st))\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\ndiff --git a/shallow.c b/shallow.c\nindex f49e6ae122..36216febb6 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \t\tfd = xmks_tempfile(&temp, git_path(\"shallow_XXXXXX\"));\n \n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len ||\n-\t\t close_tempfile(&temp) < 0)\n+\t\t close_tempfile_gently(&temp) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_tempfile_path(&temp));\n \t\tstrbuf_release(&sb);\ndiff --git a/tempfile.c b/tempfile.c\nindex 6843710670..c6740e562c 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -30,13 +30,12 @@\n * `fdopen_tempfile()` has been called on the object\n * - `owner` holds the PID of the process that created the file\n *\n- * - Active, file closed (after successful `close_tempfile()`). Same\n+ * - Active, file closed (after `close_tempfile_gently()`). Same\n * as the previous state, except that the temporary file is closed,\n * `fd` is -1, and `fp` is `NULL`.\n *\n- * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, a\n- * failed attempt to create a temporary file, or a failed\n- * `close_tempfile()`). In this state:\n+ * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a\n+ * failed attempt to create a temporary file). In this state:\n *\n * - `active` is unset\n * - `filename` is empty (usually, though there are transitory\n@@ -235,7 +234,7 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)\n \treturn tempfile->fp;\n }\n \n-int close_tempfile(struct tempfile *tempfile)\n+int close_tempfile_gently(struct tempfile *tempfile)\n {\n \tint fd = tempfile->fd;\n \tFILE *fp = tempfile->fp;\n@@ -258,14 +257,7 @@ int close_tempfile(struct tempfile *tempfile)\n \t\terr = close(fd);\n \t}\n \n-\tif (err) {\n-\t\tint save_errno = errno;\n-\t\tdelete_tempfile(tempfile);\n-\t\terrno = save_errno;\n-\t\treturn -1;\n-\t}\n-\n-\treturn 0;\n+\treturn err ? -1 : 0;\n }\n \n int reopen_tempfile(struct tempfile *tempfile)\n@@ -283,8 +275,10 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)\n \tif (!tempfile->active)\n \t\tdie(\"BUG: rename_tempfile called for inactive object\");\n \n-\tif (close_tempfile(tempfile))\n+\tif (close_tempfile_gently(tempfile)) {\n+\t\tdelete_tempfile(tempfile);\n \t\treturn -1;\n+\t}\n \n \tif (rename(tempfile->filename.buf, path)) {\n \t\tint save_errno = errno;\n@@ -303,9 +297,8 @@ void delete_tempfile(struct tempfile *tempfile)\n \tif (!tempfile->active)\n \t\treturn;\n \n-\tif (!close_tempfile(tempfile)) {\n-\t\tunlink_or_warn(tempfile->filename.buf);\n-\t\ttempfile->active = 0;\n-\t\tstrbuf_reset(&tempfile->filename);\n-\t}\n+\tclose_tempfile_gently(tempfile);\n+\tunlink_or_warn(tempfile->filename.buf);\n+\ttempfile->active = 0;\n+\tstrbuf_reset(&tempfile->filename);\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex 2f0038decd..d854dcdd3e 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -47,7 +47,7 @@\n * control of the file.\n *\n * * Close the file descriptor without removing or renaming the\n- * temporary file by calling `close_tempfile()`, and later call\n+ * temporary file by calling `close_tempfile_gently()`, and later call\n * `delete_tempfile()` or `rename_tempfile()`.\n *\n * Even after the temporary file is renamed or deleted, the `tempfile`\n@@ -59,7 +59,7 @@\n * and remove the temporary file.\n *\n * If you need to close the file descriptor yourself, do so by calling\n- * `close_tempfile()`. You should never call `close(2)` or `fclose(3)`\n+ * `close_tempfile_gently()`. You should never call `close(2)` or `fclose(3)`\n * yourself, otherwise the `struct tempfile` structure would still\n * think that the file descriptor needs to be closed, and a later\n * cleanup would result in duplicate calls to `close(2)`. Worse yet,\n@@ -74,9 +74,10 @@\n * `create_tempfile()` returns a file descriptor on success or -1 on\n * failure. On errors, `errno` describes the reason for failure.\n *\n- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile()`\n- * return 0 on success. On failure they set `errno` appropriately, do\n- * their best to delete the temporary file, and return -1.\n+ * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`\n+ * return 0 on success. On failure they set `errno` appropriately and return\n+ * -1. `delete` and `rename` (but not `close`) do their best to delete the\n+ * temporary file before returning.\n */\n \n struct tempfile {\n@@ -203,7 +204,7 @@ static inline int xmks_tempfile(struct tempfile *tempfile,\n /*\n * Associate a stdio stream with the temporary file (which must still\n * be open). Return `NULL` (*without* deleting the file) on error. The\n- * stream is closed automatically when `close_tempfile()` is called or\n+ * stream is closed automatically when `close_tempfile_gently()` is called or\n * when the file is deleted or renamed.\n */\n extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);\n@@ -226,20 +227,20 @@ extern FILE *get_tempfile_fp(struct tempfile *tempfile);\n * If the temporary file is still open, close it (and the file pointer\n * too, if it has been opened using `fdopen_tempfile()`) without\n * deleting the file. Return 0 upon success. On failure to `close(2)`,\n- * return a negative value and delete the file. Usually\n- * `delete_tempfile()` or `rename_tempfile()` should eventually be\n- * called if `close_tempfile()` succeeds.\n+ * return a negative value. Usually `delete_tempfile()` or `rename_tempfile()`\n+ * should eventually be called regardless of whether `close_tempfile_gently()`\n+ * succeeds.\n */\n-extern int close_tempfile(struct tempfile *tempfile);\n+extern int close_tempfile_gently(struct tempfile *tempfile);\n \n /*\n * Re-open a temporary file that has been closed using\n- * `close_tempfile()` but not yet deleted or renamed. This can be used\n+ * `close_tempfile_gently()` but not yet deleted or renamed. This can be used\n * to implement a sequence of operations like the following:\n *\n * * Create temporary file.\n *\n- * * Write new contents to file, then `close_tempfile()` to cause the\n+ * * Write new contents to file, then `close_tempfile_gently()` to cause the\n * contents to be written to disk.\n *\n * * Pass the name of the temporary file to another program to allow\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327555","messageId":"20170905121433.w2w72qybntk3kv6y@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 07/20] lockfile: do not rollback lock on failed close","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:33Z","receivedAt":"2017-09-05T12:14:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the lockfile code is based on the tempfile code, it\nhas some of the same problems, including that close_lock_file()\nerases the tempfile's filename buf, making it hard for the\ncaller to write a good error message.\n\nIn practice this comes up less for lockfiles than for\nstraight tempfiles, since we usually just report the\nrefname. But there is at least one buggy case in\nwrite_ref_to_lockfile(). Besides, given the coupling between\nthe lockfile and tempfile modules, it's less confusing if\ntheir close() functions have the same semantics.\n\nJust as the previous commit did for close_tempfile(), let's\nteach close_lock_file() and its wrapper close_ref() not to\nrollback on error. And just as before, we'll give them new\n\"gently\" names to catch any new callers that are added.\n\nSigned-off-by: Jeff King \n---\n lockfile.h | 30 ++++++++++++------------------\n read-cache.c | 2 +-\n refs/files-backend.c | 13 +++++++------\n refs/packed-backend.c | 3 ++-\n 4 files changed, 22 insertions(+), 26 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex dd4259bc40..c45d2db324 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -69,7 +69,7 @@\n * `rollback_lock_file()`.\n *\n * * Close the file descriptor without removing or renaming the\n- * lockfile by calling `close_lock_file()`, and later call\n+ * lockfile by calling `close_lock_file_gently()`, and later call\n * `commit_lock_file()`, `commit_lock_file_to()`,\n * `rollback_lock_file()`, or `reopen_lock_file()`.\n *\n@@ -85,7 +85,7 @@\n *\n * If you need to close the file descriptor you obtained from a\n * `hold_lock_file_for_*()` function yourself, do so by calling\n- * `close_lock_file()`. See \"tempfile.h\" for more information.\n+ * `close_lock_file_gently()`. See \"tempfile.h\" for more information.\n *\n *\n * Under the covers, a lockfile is just a tempfile with a few helper\n@@ -104,8 +104,8 @@\n *\n * Similarly, `commit_lock_file`, `commit_lock_file_to`, and\n * `close_lock_file` return 0 on success. On failure they set `errno`\n- * appropriately, do their best to roll back the lockfile, and return\n- * -1.\n+ * appropriately and return -1. The `commit` variants (but not `close`)\n+ * do their best to delete the temporary file before returning.\n */\n \n #include \"tempfile.h\"\n@@ -202,8 +202,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);\n /*\n * Associate a stdio stream with the lockfile (which must still be\n * open). Return `NULL` (*without* rolling back the lockfile) on\n- * error. The stream is closed automatically when `close_lock_file()`\n- * is called or when the file is committed or rolled back.\n+ * error. The stream is closed automatically when\n+ * `close_lock_file_gently()` is called or when the file is committed or\n+ * rolled back.\n */\n static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n {\n@@ -241,28 +242,21 @@ extern char *get_locked_file_path(struct lock_file *lk);\n * lockfile over the file being locked. Return 0 upon success. On\n * failure to `close(2)`, return a negative value and roll back the\n * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,\n- * or `rollback_lock_file()` should eventually be called if\n- * `close_lock_file()` succeeds.\n+ * or `rollback_lock_file()` should eventually be called.\n */\n-static inline int close_lock_file(struct lock_file *lk)\n+static inline int close_lock_file_gently(struct lock_file *lk)\n {\n-\tint ret = close_tempfile_gently(&lk->tempfile);\n-\tif (ret) {\n-\t\tint saved_errno = errno;\n-\t\tdelete_tempfile(&lk->tempfile);\n-\t\terrno = saved_errno;\n-\t}\n-\treturn ret;\n+\treturn close_tempfile_gently(&lk->tempfile);\n }\n \n /*\n- * Re-open a lockfile that has been closed using `close_lock_file()`\n+ * Re-open a lockfile that has been closed using `close_lock_file_gently()`\n * but not yet committed or rolled back. This can be used to implement\n * a sequence of operations like the following:\n *\n * * Lock file.\n *\n- * * Write new contents to lockfile, then `close_lock_file()` to\n+ * * Write new contents to lockfile, then `close_lock_file_gently()` to\n * cause the contents to be written to disk.\n *\n * * Pass the name of the lockfile to another program to allow it (and\ndiff --git a/read-cache.c b/read-cache.c\nindex 51686518e0..c3be65f8b0 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2345,7 +2345,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \tif (flags & COMMIT_LOCK)\n \t\treturn commit_locked_index(lock);\n \telse if (flags & CLOSE_LOCK)\n-\t\treturn close_lock_file(lock);\n+\t\treturn close_lock_file_gently(lock);\n \telse\n \t\treturn ret;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fccbc24ac4..bc1899cd4a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1402,9 +1402,9 @@ static int files_rename_ref(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int close_ref(struct ref_lock *lock)\n+static int close_ref_gently(struct ref_lock *lock)\n {\n-\tif (close_lock_file(lock->lk))\n+\tif (close_lock_file_gently(lock->lk))\n \t\treturn -1;\n \treturn 0;\n }\n@@ -1630,7 +1630,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \tfd = get_lock_file_fd(lock->lk);\n \tif (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n \t write_in_full(fd, &term, 1) != 1 ||\n-\t close_ref(lock) < 0) {\n+\t close_ref_gently(lock) < 0) {\n \t\tstrbuf_addf(err,\n \t\t\t \"couldn't write '%s'\", get_lock_file_path(lock->lk));\n \t\tunlock_ref(lock);\n@@ -2372,7 +2372,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t * the lockfile is still open. Close it to\n \t\t * free up the file descriptor:\n \t\t */\n-\t\tif (close_ref(lock)) {\n+\t\tif (close_ref_gently(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n \t\t\treturn TRANSACTION_GENERIC_ERROR;\n@@ -2848,14 +2848,15 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t\t!(type & REF_ISSYMREF) &&\n \t\t\t!is_null_oid(&cb.last_kept_oid);\n \n-\t\tif (close_lock_file(&reflog_lock)) {\n+\t\tif (close_lock_file_gently(&reflog_lock)) {\n \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n \t\t\t\t\tstrerror(errno));\n+\t\t\trollback_lock_file(&reflog_lock);\n \t\t} else if (update &&\n \t\t\t (write_in_full(get_lock_file_fd(lock->lk),\n \t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n \t\t\t write_str_in_full(get_lock_file_fd(lock->lk), \"\\n\") != 1 ||\n-\t\t\t close_ref(lock) < 0)) {\n+\t\t\t close_ref_gently(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n \t\t\t\t\tget_lock_file_path(lock->lk));\n \t\t\trollback_lock_file(&reflog_lock);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 412c85034f..dca887c0fe 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -545,8 +545,9 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t\treturn -1;\n \t}\n \n-\tif (close_lock_file(&refs->lock)) {\n+\tif (close_lock_file_gently(&refs->lock)) {\n \t\tstrbuf_addf(err, \"unable to close %s: %s\", refs->path, strerror(errno));\n+\t\trollback_lock_file(&refs->lock);\n \t\treturn -1;\n \t}\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327556","messageId":"20170905121436.rubh2yu6dgpscpnn@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 08/20] tempfile: prefer is_tempfile_active to bare access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:36Z","receivedAt":"2017-09-05T12:14:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tempfile code keeps an \"active\" flag, and we have a\nnumber of assertions to make sure that the objects are being\nused in the right order. Most of these directly check\n\"active\" rather than using the is_tempfile_active()\naccessor.\n\nLet's prefer using the accessor, in preparation for it\ngrowing more complicated logic (like checking for NULL).\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex c6740e562c..964c66d504 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -95,7 +95,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)\n \t\tatexit(remove_tempfiles_on_exit);\n \t}\n \n-\tif (tempfile->active)\n+\tif (is_tempfile_active(tempfile))\n \t\tdie(\"BUG: prepare_tempfile_object called for active object\");\n \tif (!tempfile->on_list) {\n \t\t/* Initialize *tempfile and add it to tempfile_list: */\n@@ -204,7 +204,7 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)\n \n FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: fdopen_tempfile() called for inactive object\");\n \tif (tempfile->fp)\n \t\tdie(\"BUG: fdopen_tempfile() called for open object\");\n@@ -215,21 +215,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n \n const char *get_tempfile_path(struct tempfile *tempfile)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: get_tempfile_path() called for inactive object\");\n \treturn tempfile->filename.buf;\n }\n \n int get_tempfile_fd(struct tempfile *tempfile)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: get_tempfile_fd() called for inactive object\");\n \treturn tempfile->fd;\n }\n \n FILE *get_tempfile_fp(struct tempfile *tempfile)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: get_tempfile_fp() called for inactive object\");\n \treturn tempfile->fp;\n }\n@@ -264,7 +264,7 @@ int reopen_tempfile(struct tempfile *tempfile)\n {\n \tif (0 <= tempfile->fd)\n \t\tdie(\"BUG: reopen_tempfile called for an open object\");\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: reopen_tempfile called for an inactive object\");\n \ttempfile->fd = open(tempfile->filename.buf, O_WRONLY);\n \treturn tempfile->fd;\n@@ -272,7 +272,7 @@ int reopen_tempfile(struct tempfile *tempfile)\n \n int rename_tempfile(struct tempfile *tempfile, const char *path)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: rename_tempfile called for inactive object\");\n \n \tif (close_tempfile_gently(tempfile)) {\n@@ -294,7 +294,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)\n \n void delete_tempfile(struct tempfile *tempfile)\n {\n-\tif (!tempfile->active)\n+\tif (!is_tempfile_active(tempfile))\n \t\treturn;\n \n \tclose_tempfile_gently(tempfile);\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327557","messageId":"20170905121440.hxbyspsruzrwigtq@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:40Z","receivedAt":"2017-09-05T12:15:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tempfile functions all take pointers to tempfile\nobjects, but do not check whether the argument is NULL.\nThis isn't a big deal in practice, since the lifetime of any\ntempfile object is defined to last for the whole program. So\neven if we try to call delete_tempfile() on an\nalready-deleted tempfile, our \"active\" check will tell us\nthat it's a noop.\n\nIn preparation for transitioning to a new system that\nloosens the \"tempfile objects can never be freed\" rule,\nlet's tighten up our active checks:\n\n 1. A NULL pointer is now defined as \"inactive\" (so it will\n BUG for most functions, but works as a silent noop for\n things like delete_tempfile).\n\n 2. Functions should always do the \"active\" check before\n looking at any of the struct fields.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 12 +++++++-----\n tempfile.h | 2 +-\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 964c66d504..861f817133 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -236,13 +236,15 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)\n \n int close_tempfile_gently(struct tempfile *tempfile)\n {\n-\tint fd = tempfile->fd;\n-\tFILE *fp = tempfile->fp;\n+\tint fd;\n+\tFILE *fp;\n \tint err;\n \n-\tif (fd < 0)\n+\tif (!is_tempfile_active(tempfile) || tempfile->fd < 0)\n \t\treturn 0;\n \n+\tfd = tempfile->fd;\n+\tfp = tempfile->fp;\n \ttempfile->fd = -1;\n \tif (fp) {\n \t\ttempfile->fp = NULL;\n@@ -262,10 +264,10 @@ int close_tempfile_gently(struct tempfile *tempfile)\n \n int reopen_tempfile(struct tempfile *tempfile)\n {\n-\tif (0 <= tempfile->fd)\n-\t\tdie(\"BUG: reopen_tempfile called for an open object\");\n \tif (!is_tempfile_active(tempfile))\n \t\tdie(\"BUG: reopen_tempfile called for an inactive object\");\n+\tif (0 <= tempfile->fd)\n+\t\tdie(\"BUG: reopen_tempfile called for an open object\");\n \ttempfile->fd = open(tempfile->filename.buf, O_WRONLY);\n \treturn tempfile->fd;\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex d854dcdd3e..d30663182d 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -211,7 +211,7 @@ extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);\n \n static inline int is_tempfile_active(struct tempfile *tempfile)\n {\n-\treturn tempfile->active;\n+\treturn tempfile && tempfile->active;\n }\n \n /*\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327558","messageId":"20170905121443.iwpm427f5j5hhgat@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 10/20] tempfile: replace die(\"BUG\") with BUG()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:43Z","receivedAt":"2017-09-05T12:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Compared to die(), using BUG() triggers abort(). That may\ngive us an actual coredump, which should make it easier to\nget a stack trace. And since the programming error for these\nassertions is not in the functions themselves but in their\ncallers, such a stack trace is needed to actually find the\nsource of the bug.\n\nIn addition, abort() raises SIGABRT, which is more likely to\nbe caught by our test suite.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 861f817133..813cf6a81c 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -96,7 +96,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)\n \t}\n \n \tif (is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: prepare_tempfile_object called for active object\");\n+\t\tBUG(\"prepare_tempfile_object called for active object\");\n \tif (!tempfile->on_list) {\n \t\t/* Initialize *tempfile and add it to tempfile_list: */\n \t\ttempfile->fd = -1;\n@@ -109,7 +109,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)\n \t\ttempfile->on_list = 1;\n \t} else if (tempfile->filename.len) {\n \t\t/* This shouldn't happen, but better safe than sorry. */\n-\t\tdie(\"BUG: prepare_tempfile_object called for improperly-reset object\");\n+\t\tBUG(\"prepare_tempfile_object called for improperly-reset object\");\n \t}\n }\n \n@@ -205,9 +205,9 @@ int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)\n FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: fdopen_tempfile() called for inactive object\");\n+\t\tBUG(\"fdopen_tempfile() called for inactive object\");\n \tif (tempfile->fp)\n-\t\tdie(\"BUG: fdopen_tempfile() called for open object\");\n+\t\tBUG(\"fdopen_tempfile() called for open object\");\n \n \ttempfile->fp = fdopen(tempfile->fd, mode);\n \treturn tempfile->fp;\n@@ -216,21 +216,21 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n const char *get_tempfile_path(struct tempfile *tempfile)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: get_tempfile_path() called for inactive object\");\n+\t\tBUG(\"get_tempfile_path() called for inactive object\");\n \treturn tempfile->filename.buf;\n }\n \n int get_tempfile_fd(struct tempfile *tempfile)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: get_tempfile_fd() called for inactive object\");\n+\t\tBUG(\"get_tempfile_fd() called for inactive object\");\n \treturn tempfile->fd;\n }\n \n FILE *get_tempfile_fp(struct tempfile *tempfile)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: get_tempfile_fp() called for inactive object\");\n+\t\tBUG(\"get_tempfile_fp() called for inactive object\");\n \treturn tempfile->fp;\n }\n \n@@ -265,9 +265,9 @@ int close_tempfile_gently(struct tempfile *tempfile)\n int reopen_tempfile(struct tempfile *tempfile)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: reopen_tempfile called for an inactive object\");\n+\t\tBUG(\"reopen_tempfile called for an inactive object\");\n \tif (0 <= tempfile->fd)\n-\t\tdie(\"BUG: reopen_tempfile called for an open object\");\n+\t\tBUG(\"reopen_tempfile called for an open object\");\n \ttempfile->fd = open(tempfile->filename.buf, O_WRONLY);\n \treturn tempfile->fd;\n }\n@@ -275,7 +275,7 @@ int reopen_tempfile(struct tempfile *tempfile)\n int rename_tempfile(struct tempfile *tempfile, const char *path)\n {\n \tif (!is_tempfile_active(tempfile))\n-\t\tdie(\"BUG: rename_tempfile called for inactive object\");\n+\t\tBUG(\"rename_tempfile called for inactive object\");\n \n \tif (close_tempfile_gently(tempfile)) {\n \t\tdelete_tempfile(tempfile);\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327559","messageId":"20170905121446.wzwfqlpludxurfdg@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 11/20] tempfile: factor out activation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:47Z","receivedAt":"2017-09-05T12:15:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few steps required to \"activate\" a tempfile\nstruct. Let's pull these out into a function. That saves a\nfew repeated lines now, but more importantly will make it\neasier to change the activation scheme later.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 813cf6a81c..0e6c6b9c18 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -113,6 +113,12 @@ static void prepare_tempfile_object(struct tempfile *tempfile)\n \t}\n }\n \n+static void activate_tempfile(struct tempfile *tempfile)\n+{\n+\ttempfile->owner = getpid();\n+\ttempfile->active = 1;\n+}\n+\n /* Make sure errno contains a meaningful value on error */\n int create_tempfile(struct tempfile *tempfile, const char *path)\n {\n@@ -129,8 +135,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path)\n \t\tstrbuf_reset(&tempfile->filename);\n \t\treturn -1;\n \t}\n-\ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \tif (adjust_shared_perm(tempfile->filename.buf)) {\n \t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", tempfile->filename.buf);\n@@ -145,8 +150,7 @@ void register_tempfile(struct tempfile *tempfile, const char *path)\n {\n \tprepare_tempfile_object(tempfile);\n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n-\ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n }\n \n int mks_tempfile_sm(struct tempfile *tempfile,\n@@ -160,8 +164,7 @@ int mks_tempfile_sm(struct tempfile *tempfile,\n \t\tstrbuf_reset(&tempfile->filename);\n \t\treturn -1;\n \t}\n-\ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \treturn tempfile->fd;\n }\n \n@@ -182,8 +185,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,\n \t\tstrbuf_reset(&tempfile->filename);\n \t\treturn -1;\n \t}\n-\ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n+\tactivate_tempfile(tempfile);\n \treturn tempfile->fd;\n }\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327560","messageId":"20170905121449.wnxj5tperwnoevch@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 12/20] tempfile: factor out deactivation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:50Z","receivedAt":"2017-09-05T12:15:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we deactivate a tempfile, we also have to clean up the\n\"filename\" strbuf. Let's pull this out into its own function\nto keep the logic in one place (which will become more\nimportant when a future patch makes it more complicated).\n\nNote that we can use the same function when deactivating an\nobject that _isn't_ actually active yet (like when we hit an\nerror creating a tempfile). These callsites don't currently\nreset the \"active\" flag to 0, but it's OK to do so (it's\njust a noop for these cases).\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 0e6c6b9c18..9d7f0a2f2b 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -119,6 +119,12 @@ static void activate_tempfile(struct tempfile *tempfile)\n \ttempfile->active = 1;\n }\n \n+static void deactivate_tempfile(struct tempfile *tempfile)\n+{\n+\ttempfile->active = 0;\n+\tstrbuf_reset(&tempfile->filename);\n+}\n+\n /* Make sure errno contains a meaningful value on error */\n int create_tempfile(struct tempfile *tempfile, const char *path)\n {\n@@ -132,7 +138,7 @@ int create_tempfile(struct tempfile *tempfile, const char *path)\n \t\ttempfile->fd = open(tempfile->filename.buf,\n \t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tdeactivate_tempfile(tempfile);\n \t\treturn -1;\n \t}\n \tactivate_tempfile(tempfile);\n@@ -161,7 +167,7 @@ int mks_tempfile_sm(struct tempfile *tempfile,\n \tstrbuf_add_absolute_path(&tempfile->filename, template);\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tdeactivate_tempfile(tempfile);\n \t\treturn -1;\n \t}\n \tactivate_tempfile(tempfile);\n@@ -182,7 +188,7 @@ int mks_tempfile_tsm(struct tempfile *tempfile,\n \tstrbuf_addf(&tempfile->filename, \"%s/%s\", tmpdir, template);\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n-\t\tstrbuf_reset(&tempfile->filename);\n+\t\tdeactivate_tempfile(tempfile);\n \t\treturn -1;\n \t}\n \tactivate_tempfile(tempfile);\n@@ -291,8 +297,7 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)\n \t\treturn -1;\n \t}\n \n-\ttempfile->active = 0;\n-\tstrbuf_reset(&tempfile->filename);\n+\tdeactivate_tempfile(tempfile);\n \treturn 0;\n }\n \n@@ -303,6 +308,5 @@ void delete_tempfile(struct tempfile *tempfile)\n \n \tclose_tempfile_gently(tempfile);\n \tunlink_or_warn(tempfile->filename.buf);\n-\ttempfile->active = 0;\n-\tstrbuf_reset(&tempfile->filename);\n+\tdeactivate_tempfile(tempfile);\n }\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327561","messageId":"20170905121452.eycytdbsabkbrlvx@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 13/20] tempfile: robustify cleanup handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:53Z","receivedAt":"2017-09-05T12:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may call remove_tempfiles() from an atexit handler, or\nfrom a signal handler. In the latter case we must take care\nto avoid functions which may deadlock if the process is in\nan unknown state, including looking at any stdio handles\n(which may be in the middle of doing I/O and locked) or\ncalling malloc() or free().\n\nThe current implementation calls delete_tempfile(). We unset\nthe tempfile's stdio handle (if any) to avoid deadlocking\nthere. But delete_tempfile() still calls unlink_or_warn(),\nwhich can deadlock writing to stderr if the unlink fails.\n\nSince delete_tempfile() isn't very long, let's just\nopen-code our own simple conservative version of the same\nthing. Notably:\n\n 1. The \"skip_fclose\" flag is now called \"in_signal_handler\",\n because it should inform more decisions than just the\n fclose handling.\n\n 2. We can replace close_tempfile() with just close(fd).\n That skips the fclose() question altogether. This is\n fine for the atexit() case, too; there's no point\n flushing data to a file which we're about to delete\n anyway.\n\n 3. We can choose between unlink/unlink_or_warn based on\n whether it's safe to use stderr.\n\n 4. We can replace the deactivate_tempfile() call with a\n simple setting of the active flag. There's no need to\n do any further cleanup since we know the program is\n exiting. And even though the current deactivation code\n is safe in a signal handler, this frees us up in future\n patches to make non-signal deactivation more\n complicated (e.g., by freeing resources).\n\n 5. There's no need to remove items from the tempfile_list.\n The \"active\" flag is the ultimate answer to whether an\n entry has been handled or not. Manipulating the list\n just introduces more chance of recursive signals\n stomping on each other, and the whole list will go away\n when the program exits anyway. Less is more.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 9d7f0a2f2b..3348ad59dd 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -57,18 +57,24 @@\n \n static struct tempfile *volatile tempfile_list;\n \n-static void remove_tempfiles(int skip_fclose)\n+static void remove_tempfiles(int in_signal_handler)\n {\n \tpid_t me = getpid();\n+\tstruct tempfile *volatile p;\n \n-\twhile (tempfile_list) {\n-\t\tif (tempfile_list->owner == me) {\n-\t\t\t/* fclose() is not safe to call in a signal handler */\n-\t\t\tif (skip_fclose)\n-\t\t\t\ttempfile_list->fp = NULL;\n-\t\t\tdelete_tempfile(tempfile_list);\n-\t\t}\n-\t\ttempfile_list = tempfile_list->next;\n+\tfor (p = tempfile_list; p; p = p->next) {\n+\t\tif (!is_tempfile_active(p) || p->owner != me)\n+\t\t\tcontinue;\n+\n+\t\tif (p->fd >= 0)\n+\t\t\tclose(p->fd);\n+\n+\t\tif (in_signal_handler)\n+\t\t\tunlink(p->filename.buf);\n+\t\telse\n+\t\t\tunlink_or_warn(p->filename.buf);\n+\n+\t\tp->active = 0;\n \t}\n }\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327562","messageId":"20170905121456.x4gwz3kuvvygpt55@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 14/20] tempfile: release deactivated strbufs instead of resetting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:14:56Z","receivedAt":"2017-09-05T12:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a tempfile is deactivated, we reset its strbuf to the\nempty string, which means we hold onto the memory for later\nreuse.\n\nSince we'd like to move to a system where tempfile structs\ncan actually be freed, deactivating one should drop all\nresources it is currently using. And thus \"release\" rather\nthan \"reset\" is the appropriate function to call.\n\nIn theory the reset may have saved a malloc() when a\ntempfile (or a lockfile) is reused multiple times. But in\npractice this happened rarely. Most of our tempfiles are\nsingle-use, since in cases where we might actually use many\n(like ref locking) we xcalloc() a fresh one for each ref. In\nfact, we leak those locks (to appease the rule that tempfile\nstorage can never be freed). Which means that using reset is\nactively hurting us: instead of leaking just the tempfile\nstruct, we're leaking the extra heap chunk for the filename,\ntoo.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 3348ad59dd..e655e28477 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -128,7 +128,7 @@ static void activate_tempfile(struct tempfile *tempfile)\n static void deactivate_tempfile(struct tempfile *tempfile)\n {\n \ttempfile->active = 0;\n-\tstrbuf_reset(&tempfile->filename);\n+\tstrbuf_release(&tempfile->filename);\n }\n \n /* Make sure errno contains a meaningful value on error */\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327563","messageId":"20170905121504.77xchlnjlvb2j73a@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 16/20] tempfile: remove deactivated list entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:04Z","receivedAt":"2017-09-05T12:15:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once a \"struct tempfile\" is added to the global cleanup\nlist, it is never removed. This means that its storage must\nremain valid for the lifetime of the program. For single-use\ntempfiles and locks, this isn't a big deal: we just declare\nthe struct static. But for library code which may take\nmultiple simultaneous locks (like the ref code), they're\nforced to allocate a struct on the heap and leak it.\n\nThis is mostly OK in practice. The size of the leak is\nbounded by the number of refs, and most programs exit after\noperating on a fixed number of refs (and allocate\nsimultaneous memory proportional to the number of ref\nupdates in the first place). But:\n\n 1. It isn't hard to imagine a real leak: a program which\n runs for a long time taking a series of ref update\n instructions and fulfilling them one by one. I don't\n think we have such a program now, but it's certainly\n plausible.\n\n 2. The leaked entries appear as false positives to\n tools like valgrind.\n\nLet's relax this rule by keeping only \"active\" tempfiles on\nthe list. We can do this easily by moving the list-add\noperation from prepare_tempfile_object to activate_tempfile,\nand adding a deletion in deactivate_tempfile.\n\nExisting callers do not need to be updated immediately.\nThey'll continue to leak any tempfile objects they may have\nallocated, but that's no different than the status quo. We\ncan clean them up individually.\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 46 ++++++++++++++++++++--------------------------\n tempfile.h | 15 +++++----------\n 2 files changed, 25 insertions(+), 36 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 11bda824cf..f82a5f3676 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -42,8 +42,7 @@\n * states in which this condition doesn't hold). Client code should\n * *not* rely on the filename being empty in this state.\n * - `fd` is -1 and `fp` is `NULL`\n- * - the object is left registered in the `tempfile_list`, and\n- * `on_list` is set.\n+ * - the object is removed from `tempfile_list` (but could be used again)\n *\n * A temporary file is owned by the process that created it. The\n * `tempfile` has an `owner` field that records the owner's PID. This\n@@ -92,36 +91,30 @@ static void remove_tempfiles_on_signal(int signo)\n \traise(signo);\n }\n \n-/*\n- * Initialize *tempfile if necessary and add it to tempfile_list.\n- */\n static void prepare_tempfile_object(struct tempfile *tempfile)\n {\n-\tif (volatile_list_empty(&tempfile_list)) {\n-\t\t/* One-time initialization */\n-\t\tsigchain_push_common(remove_tempfiles_on_signal);\n-\t\tatexit(remove_tempfiles_on_exit);\n-\t}\n-\n-\tif (is_tempfile_active(tempfile))\n-\t\tBUG(\"prepare_tempfile_object called for active object\");\n-\tif (!tempfile->on_list) {\n-\t\t/* Initialize *tempfile and add it to tempfile_list: */\n-\t\ttempfile->fd = -1;\n-\t\ttempfile->fp = NULL;\n-\t\ttempfile->active = 0;\n-\t\ttempfile->owner = 0;\n-\t\tstrbuf_init(&tempfile->filename, 0);\n-\t\tvolatile_list_add(&tempfile->list, &tempfile_list);\n-\t\ttempfile->on_list = 1;\n-\t} else if (tempfile->filename.len) {\n-\t\t/* This shouldn't happen, but better safe than sorry. */\n-\t\tBUG(\"prepare_tempfile_object called for improperly-reset object\");\n-\t}\n+\ttempfile->fd = -1;\n+\ttempfile->fp = NULL;\n+\ttempfile->active = 0;\n+\ttempfile->owner = 0;\n+\tINIT_LIST_HEAD(&tempfile->list);\n+\tstrbuf_init(&tempfile->filename, 0);\n }\n \n static void activate_tempfile(struct tempfile *tempfile)\n {\n+\tstatic int initialized;\n+\n+\tif (is_tempfile_active(tempfile))\n+\t\tBUG(\"activate_tempfile called for active object\");\n+\n+\tif (!initialized) {\n+\t\tsigchain_push_common(remove_tempfiles_on_signal);\n+\t\tatexit(remove_tempfiles_on_exit);\n+\t\tinitialized = 1;\n+\t}\n+\n+\tvolatile_list_add(&tempfile->list, &tempfile_list);\n \ttempfile->owner = getpid();\n \ttempfile->active = 1;\n }\n@@ -130,6 +123,7 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n {\n \ttempfile->active = 0;\n \tstrbuf_release(&tempfile->filename);\n+\tvolatile_list_del(&tempfile->list);\n }\n \n /* Make sure errno contains a meaningful value on error */\ndiff --git a/tempfile.h b/tempfile.h\nindex 2ee24f4380..e32b4df092 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -17,12 +17,9 @@\n *\n * The caller:\n *\n- * * Allocates a `struct tempfile` either as a static variable or on\n- * the heap, initialized to zeros. Once you use the structure to\n- * call `create_tempfile()`, it belongs to the tempfile subsystem\n- * and its storage must remain valid throughout the life of the\n- * program (i.e. you cannot use an on-stack variable to hold this\n- * structure).\n+ * * Allocates a `struct tempfile`. Once the structure is passed to\n+ * `create_tempfile()`, its storage must remain valid until\n+ * `delete_tempfile()` or `rename_tempfile()` is called on it.\n *\n * * Attempts to create a temporary file by calling\n * `create_tempfile()`.\n@@ -52,9 +49,8 @@\n * temporary file by calling `close_tempfile_gently()`, and later call\n * `delete_tempfile()` or `rename_tempfile()`.\n *\n- * Even after the temporary file is renamed or deleted, the `tempfile`\n- * object must not be freed or altered by the caller. However, it may\n- * be reused; just pass it to another call of `create_tempfile()`.\n+ * After the temporary file is renamed or deleted, the `tempfile`\n+ * object may be reused or freed.\n *\n * If the program exits before `rename_tempfile()` or\n * `delete_tempfile()` is called, an `atexit(3)` handler will close\n@@ -88,7 +84,6 @@ struct tempfile {\n \tvolatile int fd;\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n-\tchar on_list;\n \tstruct strbuf filename;\n };\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327564","messageId":"20170905121508.filu4vnf7jrgnj2l@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 17/20] tempfile: auto-allocate tempfiles on heap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:08Z","receivedAt":"2017-09-05T12:15:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit taught the tempfile code to give up\nownership over tempfiles that have been renamed or deleted.\nThat makes it possible to use a stack variable like this:\n\n struct tempfile t;\n\n create_tempfile(&t, ...);\n ...\n if (!err)\n rename_tempfile(&t, ...);\n else\n delete_tempfile(&t);\n\nBut doing it this way has a high potential for creating\nmemory errors. The tempfile we pass to create_tempfile()\nends up on a global linked list, and it's not safe for it to\ngo out of scope until we've called one of those two\ndeactivation functions.\n\nImagine that we add an early return from the function that\nforgets to call delete_tempfile(). With a static or heap\ntempfile variable, the worst case is that the tempfile hangs\naround until the program exits (and some functions like\nsetup_shallow_temporary rely on this intentionally, creating\na tempfile and then leaving it for later cleanup).\n\nBut with a stack variable as above, this is a serious memory\nerror: the variable goes out of scope and may be filled with\ngarbage by the time the tempfile code looks at it. Let's\nsee if we can make it harder to get this wrong.\n\nSince many callers need to allocate arbitrary numbers of\ntempfiles, we can't rely on static storage as a general\nsolution. So we need to turn to the heap. We could just ask\nall callers to pass us a heap variable, but that puts the\nburden on them to call free() at the right time.\n\nInstead, let's have the tempfile code handle the heap\nallocation _and_ the deallocation (when the tempfile is\ndeactivated and removed from the list).\n\nThis changes the return value of all of the creation\nfunctions. For the cleanup functions (delete and rename),\nwe'll add one extra bit of safety: instead of taking a\ntempfile pointer, we'll take a pointer-to-pointer and set it\nto NULL after freeing the object. This makes it safe to\ndouble-call functions like delete_tempfile(), as the second\ncall treats the NULL input as a noop. Several callsites\nfollow this pattern.\n\nThe resulting patch does have a fair bit of noise, as each\ncaller needs to be converted to handle:\n\n 1. Storing a pointer instead of the struct itself.\n\n 2. Passing the pointer instead of taking the struct\n address.\n\n 3. Handling a \"struct tempfile *\" return instead of a file\n descriptor.\n\nWe could play games to make this less noisy. For example, by\ndefining the tempfile like this:\n\n struct tempfile {\n\tstruct heap_allocated_part_of_tempfile {\n int fd;\n ...etc\n } *actual_data;\n }\n\nCallers would continue to have a \"struct tempfile\", and it\nwould be \"active\" only when the inner pointer was non-NULL.\nBut that just makes things more awkward in the long run.\nThere aren't that many callers, so we can simply bite\nthe bullet and adjust all of them. And the compiler makes it\neasy for us to find them all.\n\nSigned-off-by: Jeff King \n---\n builtin/gc.c | 8 ++---\n credential-cache--daemon.c | 5 ++-\n diff.c | 15 ++++----\n gpg-interface.c | 16 ++++-----\n lockfile.c | 7 ++--\n lockfile.h | 16 ++++-----\n read-cache.c | 25 +++++++-------\n refs/files-backend.c | 4 +--\n refs/packed-backend.c | 11 +++---\n shallow.c | 13 ++++---\n tempfile.c | 66 +++++++++++++++++++----------------\n tempfile.h | 85 +++++++++++++++++++++-------------------------\n trailer.c | 6 ++--\n 13 files changed, 136 insertions(+), 141 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3c78fcb9b1..c22787ac72 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -47,7 +47,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n-static struct tempfile pidfile;\n+static struct tempfile *pidfile;\n static struct lock_file log_lock;\n \n static struct string_list pack_garbage = STRING_LIST_INIT_DUP;\n@@ -78,7 +78,7 @@ static void process_log_file(void)\n \t\t */\n \t\tint saved_errno = errno;\n \t\tfprintf(stderr, _(\"Failed to fstat %s: %s\"),\n-\t\t\tget_tempfile_path(&log_lock.tempfile),\n+\t\t\tget_tempfile_path(log_lock.tempfile),\n \t\t\tstrerror(saved_errno));\n \t\tfflush(stderr);\n \t\tcommit_lock_file(&log_lock);\n@@ -242,7 +242,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tint fd;\n \tchar *pidfile_path;\n \n-\tif (is_tempfile_active(&pidfile))\n+\tif (is_tempfile_active(pidfile))\n \t\t/* already locked */\n \t\treturn NULL;\n \n@@ -293,7 +293,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \twrite_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \tcommit_lock_file(&lock);\n-\tregister_tempfile(&pidfile, pidfile_path);\n+\tpidfile = register_tempfile(pidfile_path);\n \tfree(pidfile_path);\n \treturn NULL;\n }\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 0d5c625094..4dfbc8c9f9 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -5,8 +5,6 @@\n #include \"unix-socket.h\"\n #include \"parse-options.h\"\n \n-static struct tempfile socket_file;\n-\n struct credential_cache_entry {\n \tstruct credential item;\n \ttimestamp_t expiration;\n@@ -260,6 +258,7 @@ static void init_socket_directory(const char *path)\n \n int cmd_main(int argc, const char **argv)\n {\n+\tstruct tempfile *socket_file;\n \tconst char *socket_path;\n \tint ignore_sighup = 0;\n \tstatic const char *usage[] = {\n@@ -285,7 +284,7 @@ int cmd_main(int argc, const char **argv)\n \t\tdie(\"socket directory must be an absolute path\");\n \n \tinit_socket_directory(socket_path);\n-\tregister_tempfile(&socket_file, socket_path);\n+\tsocket_file = register_tempfile(socket_path);\n \n \tif (ignore_sighup)\n \t\tsignal(SIGHUP, SIG_IGN);\ndiff --git a/diff.c b/diff.c\nindex 20f68ec389..7df2227a3c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -459,7 +459,7 @@ static struct diff_tempfile {\n \t * If this diff_tempfile instance refers to a temporary file,\n \t * this tempfile object is used to manage its lifetime.\n \t */\n-\tstruct tempfile tempfile;\n+\tstruct tempfile *tempfile;\n } diff_temp[2];\n \n struct emit_callback {\n@@ -1414,7 +1414,7 @@ static void remove_tempfile(void)\n {\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(diff_temp); i++) {\n-\t\tif (is_tempfile_active(&diff_temp[i].tempfile))\n+\t\tif (is_tempfile_active(diff_temp[i].tempfile))\n \t\t\tdelete_tempfile(&diff_temp[i].tempfile);\n \t\tdiff_temp[i].name = NULL;\n \t}\n@@ -3720,7 +3720,6 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\t\t const struct object_id *oid,\n \t\t\t int mode)\n {\n-\tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf template = STRBUF_INIT;\n \tchar *path_dup = xstrdup(path);\n@@ -3730,18 +3729,18 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \tstrbuf_addstr(&template, \"XXXXXX_\");\n \tstrbuf_addstr(&template, base);\n \n-\tfd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1);\n-\tif (fd < 0)\n+\ttemp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1);\n+\tif (!temp->tempfile)\n \t\tdie_errno(\"unable to create temp-file\");\n \tif (convert_to_working_tree(path,\n \t\t\t(const char *)blob, (size_t)size, &buf)) {\n \t\tblob = buf.buf;\n \t\tsize = buf.len;\n \t}\n-\tif (write_in_full(fd, blob, size) != size ||\n-\t close_tempfile_gently(&temp->tempfile))\n+\tif (write_in_full(temp->tempfile->fd, blob, size) != size ||\n+\t close_tempfile_gently(temp->tempfile))\n \t\tdie_errno(\"unable to write temp-file\");\n-\ttemp->name = get_tempfile_path(&temp->tempfile);\n+\ttemp->name = get_tempfile_path(temp->tempfile);\n \toid_to_hex_r(temp->hex, oid);\n \txsnprintf(temp->mode, sizeof(temp->mode), \"%06o\", mode);\n \tstrbuf_release(&buf);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 4ea2597ff4..4feacf16e5 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -202,17 +202,17 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tstatic struct tempfile temp;\n-\tint fd, ret;\n+\tstruct tempfile *temp;\n+\tint ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfd = mks_tempfile_t(&temp, \".git_vtag_tmpXXXXXX\");\n-\tif (fd < 0)\n+\ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n+\tif (!temp)\n \t\treturn error_errno(_(\"could not create temporary file\"));\n-\tif (write_in_full(fd, signature, signature_size) < 0 ||\n-\t close_tempfile_gently(&temp) < 0) {\n+\tif (write_in_full(temp->fd, signature, signature_size) < 0 ||\n+\t close_tempfile_gently(temp) < 0) {\n \t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n-\t\t\t temp.filename.buf);\n+\t\t\t temp->filename.buf);\n \t\tdelete_tempfile(&temp);\n \t\treturn -1;\n \t}\n@@ -221,7 +221,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t gpg_program,\n \t\t\t \"--status-fd=1\",\n \t\t\t \"--keyid-format=long\",\n-\t\t\t \"--verify\", temp.filename.buf, \"-\",\n+\t\t\t \"--verify\", temp->filename.buf, \"-\",\n \t\t\t NULL);\n \n \tif (!gpg_status)\ndiff --git a/lockfile.c b/lockfile.c\nindex aa69210d8b..efcb7d7dfe 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -72,7 +72,6 @@ static void resolve_symlink(struct strbuf *path)\n /* Make sure errno contains a meaningful value on error */\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\tint fd;\n \tstruct strbuf filename = STRBUF_INIT;\n \n \tstrbuf_addstr(&filename, path);\n@@ -80,9 +79,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(&filename);\n \n \tstrbuf_addstr(&filename, LOCK_SUFFIX);\n-\tfd = create_tempfile(&lk->tempfile, filename.buf);\n+\tlk->tempfile = create_tempfile(filename.buf);\n \tstrbuf_release(&filename);\n-\treturn fd;\n+\treturn lk->tempfile ? lk->tempfile->fd : -1;\n }\n \n /*\n@@ -191,7 +190,7 @@ char *get_locked_file_path(struct lock_file *lk)\n {\n \tstruct strbuf ret = STRBUF_INIT;\n \n-\tstrbuf_addstr(&ret, get_tempfile_path(&lk->tempfile));\n+\tstrbuf_addstr(&ret, get_tempfile_path(lk->tempfile));\n \tif (ret.len <= LOCK_SUFFIX_LEN ||\n \t strcmp(ret.buf + ret.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))\n \t\tdie(\"BUG: get_locked_file_path() called for malformed lock object\");\ndiff --git a/lockfile.h b/lockfile.h\nindex c45d2db324..6ed336e2bc 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -111,7 +111,7 @@\n #include \"tempfile.h\"\n \n struct lock_file {\n-\tstruct tempfile tempfile;\n+\tstruct tempfile *tempfile;\n };\n \n /* String appended to a filename to derive the lockfile name: */\n@@ -180,7 +180,7 @@ static inline int hold_lock_file_for_update(\n */\n static inline int is_lock_file_locked(struct lock_file *lk)\n {\n-\treturn is_tempfile_active(&lk->tempfile);\n+\treturn is_tempfile_active(lk->tempfile);\n }\n \n /*\n@@ -208,7 +208,7 @@ extern NORETURN void unable_to_lock_die(const char *path, int err);\n */\n static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n {\n-\treturn fdopen_tempfile(&lk->tempfile, mode);\n+\treturn fdopen_tempfile(lk->tempfile, mode);\n }\n \n /*\n@@ -217,17 +217,17 @@ static inline FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n */\n static inline const char *get_lock_file_path(struct lock_file *lk)\n {\n-\treturn get_tempfile_path(&lk->tempfile);\n+\treturn get_tempfile_path(lk->tempfile);\n }\n \n static inline int get_lock_file_fd(struct lock_file *lk)\n {\n-\treturn get_tempfile_fd(&lk->tempfile);\n+\treturn get_tempfile_fd(lk->tempfile);\n }\n \n static inline FILE *get_lock_file_fp(struct lock_file *lk)\n {\n-\treturn get_tempfile_fp(&lk->tempfile);\n+\treturn get_tempfile_fp(lk->tempfile);\n }\n \n /*\n@@ -246,7 +246,7 @@ extern char *get_locked_file_path(struct lock_file *lk);\n */\n static inline int close_lock_file_gently(struct lock_file *lk)\n {\n-\treturn close_tempfile_gently(&lk->tempfile);\n+\treturn close_tempfile_gently(lk->tempfile);\n }\n \n /*\n@@ -270,7 +270,7 @@ static inline int close_lock_file_gently(struct lock_file *lk)\n */\n static inline int reopen_lock_file(struct lock_file *lk)\n {\n-\treturn reopen_tempfile(&lk->tempfile);\n+\treturn reopen_tempfile(lk->tempfile);\n }\n \n /*\ndiff --git a/read-cache.c b/read-cache.c\nindex c3be65f8b0..b211c57af6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2311,7 +2311,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n-\t\tdelete_tempfile(tempfile);\n+\t\tdelete_tempfile(&tempfile);\n \t\treturn -1;\n \t}\n \tif (stat(tempfile->filename.buf, &st))\n@@ -2337,7 +2337,7 @@ static int commit_locked_index(struct lock_file *lk)\n static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\t\t\t unsigned flags)\n {\n-\tint ret = do_write_index(istate, &lock->tempfile, 0);\n+\tint ret = do_write_index(istate, lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n \tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n@@ -2420,34 +2420,33 @@ static int clean_shared_index_files(const char *current_hex)\n \treturn 0;\n }\n \n-static struct tempfile temporary_sharedindex;\n-\n static int write_shared_index(struct index_state *istate,\n \t\t\t struct lock_file *lock, unsigned flags)\n {\n+\tstruct tempfile *temp;\n \tstruct split_index *si = istate->split_index;\n-\tint fd, ret;\n+\tint ret;\n \n-\tfd = mks_tempfile(&temporary_sharedindex, git_path(\"sharedindex_XXXXXX\"));\n-\tif (fd < 0) {\n+\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\tif (!temp) {\n \t\thashclr(si->base_sha1);\n \t\treturn do_write_locked_index(istate, lock, flags);\n \t}\n \tmove_cache_to_base_index(istate);\n-\tret = do_write_index(si->base, &temporary_sharedindex, 1);\n+\tret = do_write_index(si->base, temp, 1);\n \tif (ret) {\n-\t\tdelete_tempfile(&temporary_sharedindex);\n+\t\tdelete_tempfile(&temp);\n \t\treturn ret;\n \t}\n-\tret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex));\n+\tret = adjust_shared_perm(get_tempfile_path(temp));\n \tif (ret) {\n \t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(&temporary_sharedindex));\n-\t\tdelete_tempfile(&temporary_sharedindex);\n+\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(temp));\n+\t\tdelete_tempfile(&temp);\n \t\terrno = save_errno;\n \t\treturn ret;\n \t}\n-\tret = rename_tempfile(&temporary_sharedindex,\n+\tret = rename_tempfile(&temp,\n \t\t\t git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n \tif (!ret) {\n \t\thashcpy(si->base_sha1, si->base->sha1);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bc1899cd4a..36fe8a986b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1747,12 +1747,12 @@ static int create_symref_locked(struct files_ref_store *refs,\n \n \tif (!fdopen_lock_file(lock->lk, \"w\"))\n \t\treturn error(\"unable to fdopen %s: %s\",\n-\t\t\t lock->lk->tempfile.filename.buf, strerror(errno));\n+\t\t\t lock->lk->tempfile->filename.buf, strerror(errno));\n \n \tupdate_symref_reflog(refs, lock, refname, target, logmsg);\n \n \t/* no error check; commit_ref will check ferror */\n-\tfprintf(lock->lk->tempfile.fp, \"ref: %s\\n\", target);\n+\tfprintf(lock->lk->tempfile->fp, \"ref: %s\\n\", target);\n \tif (commit_ref(lock) < 0)\n \t\treturn error(\"unable to write symref for %s: %s\", refname,\n \t\t\t strerror(errno));\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex dca887c0fe..321608a114 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -75,7 +75,7 @@ struct packed_ref_store {\n \t * \"packed-refs\" file. Note that this (and thus the enclosing\n \t * `packed_ref_store`) must not be freed.\n \t */\n-\tstruct tempfile tempfile;\n+\tstruct tempfile *tempfile;\n };\n \n struct ref_store *packed_ref_store_create(const char *path,\n@@ -628,7 +628,8 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \t */\n \tpacked_refs_path = get_locked_file_path(&refs->lock);\n \tstrbuf_addf(&sb, \"%s.new\", packed_refs_path);\n-\tif (create_tempfile(&refs->tempfile, sb.buf) < 0) {\n+\trefs->tempfile = create_tempfile(sb.buf);\n+\tif (!refs->tempfile) {\n \t\tstrbuf_addf(err, \"unable to create file %s: %s\",\n \t\t\t sb.buf, strerror(errno));\n \t\tstrbuf_release(&sb);\n@@ -636,7 +637,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \t}\n \tstrbuf_release(&sb);\n \n-\tout = fdopen_tempfile(&refs->tempfile, \"w\");\n+\tout = fdopen_tempfile(refs->tempfile, \"w\");\n \tif (!out) {\n \t\tstrbuf_addf(err, \"unable to fdopen packed-refs tempfile: %s\",\n \t\t\t strerror(errno));\n@@ -645,7 +646,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \n \tif (fprintf(out, \"%s\", PACKED_REFS_HEADER) < 0) {\n \t\tstrbuf_addf(err, \"error writing to %s: %s\",\n-\t\t\t get_tempfile_path(&refs->tempfile), strerror(errno));\n+\t\t\t get_tempfile_path(refs->tempfile), strerror(errno));\n \t\tgoto error;\n \t}\n \n@@ -657,7 +658,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \t\tif (write_packed_entry(out, iter->refname, iter->oid->hash,\n \t\t\t\t peel_error ? NULL : peeled.hash)) {\n \t\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n-\t\t\t\t get_tempfile_path(&refs->tempfile),\n+\t\t\t\t get_tempfile_path(refs->tempfile),\n \t\t\t\t strerror(errno));\n \t\t\tref_iterator_abort(iter);\n \t\t\tgoto error;\ndiff --git a/shallow.c b/shallow.c\nindex 36216febb6..1cc1c76415 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -288,19 +288,18 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \n const char *setup_temporary_shallow(const struct oid_array *extra)\n {\n-\tstatic struct tempfile temp;\n+\tstruct tempfile *temp;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint fd;\n \n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\tfd = xmks_tempfile(&temp, git_path(\"shallow_XXXXXX\"));\n+\t\ttemp = xmks_tempfile(git_path(\"shallow_XXXXXX\"));\n \n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len ||\n-\t\t close_tempfile_gently(&temp) < 0)\n+\t\tif (write_in_full(temp->fd, sb.buf, sb.len) != sb.len ||\n+\t\t close_tempfile_gently(temp) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t get_tempfile_path(&temp));\n+\t\t\t\t get_tempfile_path(temp));\n \t\tstrbuf_release(&sb);\n-\t\treturn get_tempfile_path(&temp);\n+\t\treturn get_tempfile_path(temp);\n \t}\n \t/*\n \t * is_repository_shallow() sees empty string as \"no shallow\ndiff --git a/tempfile.c b/tempfile.c\nindex f82a5f3676..5fdafdd2d2 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -91,14 +91,16 @@ static void remove_tempfiles_on_signal(int signo)\n \traise(signo);\n }\n \n-static void prepare_tempfile_object(struct tempfile *tempfile)\n+static struct tempfile *new_tempfile(void)\n {\n+\tstruct tempfile *tempfile = xmalloc(sizeof(*tempfile));\n \ttempfile->fd = -1;\n \ttempfile->fp = NULL;\n \ttempfile->active = 0;\n \ttempfile->owner = 0;\n \tINIT_LIST_HEAD(&tempfile->list);\n \tstrbuf_init(&tempfile->filename, 0);\n+\treturn tempfile;\n }\n \n static void activate_tempfile(struct tempfile *tempfile)\n@@ -124,12 +126,13 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n \ttempfile->active = 0;\n \tstrbuf_release(&tempfile->filename);\n \tvolatile_list_del(&tempfile->list);\n+\tfree(tempfile);\n }\n \n /* Make sure errno contains a meaningful value on error */\n-int create_tempfile(struct tempfile *tempfile, const char *path)\n+struct tempfile *create_tempfile(const char *path)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tstruct tempfile *tempfile = new_tempfile();\n \n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \ttempfile->fd = open(tempfile->filename.buf,\n@@ -140,48 +143,47 @@ int create_tempfile(struct tempfile *tempfile, const char *path)\n \t\t\t\t O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n-\t\treturn -1;\n+\t\treturn NULL;\n \t}\n \tactivate_tempfile(tempfile);\n \tif (adjust_shared_perm(tempfile->filename.buf)) {\n \t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", tempfile->filename.buf);\n-\t\tdelete_tempfile(tempfile);\n+\t\tdelete_tempfile(&tempfile);\n \t\terrno = save_errno;\n-\t\treturn -1;\n+\t\treturn NULL;\n \t}\n-\treturn tempfile->fd;\n+\n+\treturn tempfile;\n }\n \n-void register_tempfile(struct tempfile *tempfile, const char *path)\n+struct tempfile *register_tempfile(const char *path)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tstruct tempfile *tempfile = new_tempfile();\n \tstrbuf_add_absolute_path(&tempfile->filename, path);\n \tactivate_tempfile(tempfile);\n+\treturn tempfile;\n }\n \n-int mks_tempfile_sm(struct tempfile *tempfile,\n-\t\t const char *template, int suffixlen, int mode)\n+struct tempfile *mks_tempfile_sm(const char *template, int suffixlen, int mode)\n {\n-\tprepare_tempfile_object(tempfile);\n+\tstruct tempfile *tempfile = new_tempfile();\n \n \tstrbuf_add_absolute_path(&tempfile->filename, template);\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n-\t\treturn -1;\n+\t\treturn NULL;\n \t}\n \tactivate_tempfile(tempfile);\n-\treturn tempfile->fd;\n+\treturn tempfile;\n }\n \n-int mks_tempfile_tsm(struct tempfile *tempfile,\n-\t\t const char *template, int suffixlen, int mode)\n+struct tempfile *mks_tempfile_tsm(const char *template, int suffixlen, int mode)\n {\n+\tstruct tempfile *tempfile = new_tempfile();\n \tconst char *tmpdir;\n \n-\tprepare_tempfile_object(tempfile);\n-\n \ttmpdir = getenv(\"TMPDIR\");\n \tif (!tmpdir)\n \t\ttmpdir = \"/tmp\";\n@@ -190,25 +192,25 @@ int mks_tempfile_tsm(struct tempfile *tempfile,\n \ttempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);\n \tif (tempfile->fd < 0) {\n \t\tdeactivate_tempfile(tempfile);\n-\t\treturn -1;\n+\t\treturn NULL;\n \t}\n \tactivate_tempfile(tempfile);\n-\treturn tempfile->fd;\n+\treturn tempfile;\n }\n \n-int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)\n+struct tempfile *xmks_tempfile_m(const char *template, int mode)\n {\n-\tint fd;\n+\tstruct tempfile *tempfile;\n \tstruct strbuf full_template = STRBUF_INIT;\n \n \tstrbuf_add_absolute_path(&full_template, template);\n-\tfd = mks_tempfile_m(tempfile, full_template.buf, mode);\n-\tif (fd < 0)\n+\ttempfile = mks_tempfile_m(full_template.buf, mode);\n+\tif (!tempfile)\n \t\tdie_errno(\"Unable to create temporary file '%s'\",\n \t\t\t full_template.buf);\n \n \tstrbuf_release(&full_template);\n-\treturn fd;\n+\treturn tempfile;\n }\n \n FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)\n@@ -281,33 +283,39 @@ int reopen_tempfile(struct tempfile *tempfile)\n \treturn tempfile->fd;\n }\n \n-int rename_tempfile(struct tempfile *tempfile, const char *path)\n+int rename_tempfile(struct tempfile **tempfile_p, const char *path)\n {\n+\tstruct tempfile *tempfile = *tempfile_p;\n+\n \tif (!is_tempfile_active(tempfile))\n \t\tBUG(\"rename_tempfile called for inactive object\");\n \n \tif (close_tempfile_gently(tempfile)) {\n-\t\tdelete_tempfile(tempfile);\n+\t\tdelete_tempfile(tempfile_p);\n \t\treturn -1;\n \t}\n \n \tif (rename(tempfile->filename.buf, path)) {\n \t\tint save_errno = errno;\n-\t\tdelete_tempfile(tempfile);\n+\t\tdelete_tempfile(tempfile_p);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \n \tdeactivate_tempfile(tempfile);\n+\t*tempfile_p = NULL;\n \treturn 0;\n }\n \n-void delete_tempfile(struct tempfile *tempfile)\n+void delete_tempfile(struct tempfile **tempfile_p)\n {\n+\tstruct tempfile *tempfile = *tempfile_p;\n+\n \tif (!is_tempfile_active(tempfile))\n \t\treturn;\n \n \tclose_tempfile_gently(tempfile);\n \tunlink_or_warn(tempfile->filename.buf);\n \tdeactivate_tempfile(tempfile);\n+\t*tempfile_p = NULL;\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex e32b4df092..b8f4b5e145 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -17,22 +17,18 @@\n *\n * The caller:\n *\n- * * Allocates a `struct tempfile`. Once the structure is passed to\n- * `create_tempfile()`, its storage must remain valid until\n- * `delete_tempfile()` or `rename_tempfile()` is called on it.\n- *\n * * Attempts to create a temporary file by calling\n- * `create_tempfile()`.\n+ * `create_tempfile()`. The resources used for the temporary file are\n+ * managed by the tempfile API.\n *\n * * Writes new content to the file by either:\n *\n- * * writing to the file descriptor returned by `create_tempfile()`\n- * (also available via `tempfile->fd`).\n+ * * writing to the `tempfile->fd` file descriptor\n *\n * * calling `fdopen_tempfile()` to get a `FILE` pointer for the\n * open file and writing to the file using stdio.\n *\n- * Note that the file descriptor returned by create_tempfile()\n+ * Note that the file descriptor created by create_tempfile()\n * is marked O_CLOEXEC, so the new contents must be written by\n * the current process, not any spawned one.\n *\n@@ -50,7 +46,7 @@\n * `delete_tempfile()` or `rename_tempfile()`.\n *\n * After the temporary file is renamed or deleted, the `tempfile`\n- * object may be reused or freed.\n+ * object is no longer valid and should not be reused.\n *\n * If the program exits before `rename_tempfile()` or\n * `delete_tempfile()` is called, an `atexit(3)` handler will close\n@@ -69,8 +65,8 @@\n * Error handling\n * --------------\n *\n- * `create_tempfile()` returns a file descriptor on success or -1 on\n- * failure. On errors, `errno` describes the reason for failure.\n+ * `create_tempfile()` returns an allocated tempfile on success or NULL\n+ * on failure. On errors, `errno` describes the reason for failure.\n *\n * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`\n * return 0 on success. On failure they set `errno` appropriately and return\n@@ -89,10 +85,10 @@ struct tempfile {\n \n /*\n * Attempt to create a temporary file at the specified `path`. Return\n- * a file descriptor for writing to it, or -1 on error. It is an error\n- * if a file already exists at that path.\n+ * a tempfile (whose \"fd\" member can be used for writing to it), or\n+ * NULL on error. It is an error if a file already exists at that path.\n */\n-extern int create_tempfile(struct tempfile *tempfile, const char *path);\n+extern struct tempfile *create_tempfile(const char *path);\n \n /*\n * Register an existing file as a tempfile, meaning that it will be\n@@ -100,7 +96,7 @@ extern int create_tempfile(struct tempfile *tempfile, const char *path);\n * but it can be worked with like any other closed tempfile (for\n * example, it can be opened using reopen_tempfile()).\n */\n-extern void register_tempfile(struct tempfile *tempfile, const char *path);\n+extern struct tempfile *register_tempfile(const char *path);\n \n \n /*\n@@ -132,70 +128,65 @@ extern void register_tempfile(struct tempfile *tempfile, const char *path);\n * know the (absolute) path of the file that was created, it can be\n * read from tempfile->filename.\n *\n- * On success, the functions return a file descriptor that is open for\n- * writing the temporary file. On errors, they return -1 and set errno\n- * appropriately (except for the \"x\" variants, which die() on errors).\n+ * On success, the functions return a tempfile whose \"fd\" member is open\n+ * for writing the temporary file. On errors, they return NULL and set\n+ * errno appropriately (except for the \"x\" variants, which die() on\n+ * errors).\n */\n \n /* See \"mks_tempfile functions\" above. */\n-extern int mks_tempfile_sm(struct tempfile *tempfile,\n-\t\t\t const char *template, int suffixlen, int mode);\n+extern struct tempfile *mks_tempfile_sm(const char *template,\n+\t\t\t\t\tint suffixlen, int mode);\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile_s(struct tempfile *tempfile,\n-\t\t\t\t const char *template, int suffixlen)\n+static inline struct tempfile *mks_tempfile_s(const char *template,\n+\t\t\t\t\t int suffixlen)\n {\n-\treturn mks_tempfile_sm(tempfile, template, suffixlen, 0600);\n+\treturn mks_tempfile_sm(template, suffixlen, 0600);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile_m(struct tempfile *tempfile,\n-\t\t\t\t const char *template, int mode)\n+static inline struct tempfile *mks_tempfile_m(const char *template, int mode)\n {\n-\treturn mks_tempfile_sm(tempfile, template, 0, mode);\n+\treturn mks_tempfile_sm(template, 0, mode);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile(struct tempfile *tempfile,\n-\t\t\t const char *template)\n+static inline struct tempfile *mks_tempfile(const char *template)\n {\n-\treturn mks_tempfile_sm(tempfile, template, 0, 0600);\n+\treturn mks_tempfile_sm(template, 0, 0600);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-extern int mks_tempfile_tsm(struct tempfile *tempfile,\n-\t\t\t const char *template, int suffixlen, int mode);\n+extern struct tempfile *mks_tempfile_tsm(const char *template,\n+\t\t\t\t\t int suffixlen, int mode);\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile_ts(struct tempfile *tempfile,\n-\t\t\t\t const char *template, int suffixlen)\n+static inline struct tempfile *mks_tempfile_ts(const char *template,\n+\t\t\t\t\t int suffixlen)\n {\n-\treturn mks_tempfile_tsm(tempfile, template, suffixlen, 0600);\n+\treturn mks_tempfile_tsm(template, suffixlen, 0600);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile_tm(struct tempfile *tempfile,\n-\t\t\t\t const char *template, int mode)\n+static inline struct tempfile *mks_tempfile_tm(const char *template, int mode)\n {\n-\treturn mks_tempfile_tsm(tempfile, template, 0, mode);\n+\treturn mks_tempfile_tsm(template, 0, mode);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int mks_tempfile_t(struct tempfile *tempfile,\n-\t\t\t\t const char *template)\n+static inline struct tempfile *mks_tempfile_t(const char *template)\n {\n-\treturn mks_tempfile_tsm(tempfile, template, 0, 0600);\n+\treturn mks_tempfile_tsm(template, 0, 0600);\n }\n \n /* See \"mks_tempfile functions\" above. */\n-extern int xmks_tempfile_m(struct tempfile *tempfile,\n-\t\t\t const char *template, int mode);\n+extern struct tempfile *xmks_tempfile_m(const char *template, int mode);\n \n /* See \"mks_tempfile functions\" above. */\n-static inline int xmks_tempfile(struct tempfile *tempfile,\n-\t\t\t\tconst char *template)\n+static inline struct tempfile *xmks_tempfile(const char *template)\n {\n-\treturn xmks_tempfile_m(tempfile, template, 0600);\n+\treturn xmks_tempfile_m(template, 0600);\n }\n \n /*\n@@ -257,7 +248,7 @@ extern int reopen_tempfile(struct tempfile *tempfile);\n * `delete_tempfile()` for a `tempfile` object that has already been\n * deleted or renamed.\n */\n-extern void delete_tempfile(struct tempfile *tempfile);\n+extern void delete_tempfile(struct tempfile **tempfile_p);\n \n /*\n * Close the file descriptor and/or file pointer if they are still\n@@ -268,6 +259,6 @@ extern void delete_tempfile(struct tempfile *tempfile);\n * `rename(2)`. It is a bug to call `rename_tempfile()` for a\n * `tempfile` object that is not currently active.\n */\n-extern int rename_tempfile(struct tempfile *tempfile, const char *path);\n+extern int rename_tempfile(struct tempfile **tempfile_p, const char *path);\n \n #endif /* TEMPFILE_H */\ndiff --git a/trailer.c b/trailer.c\nindex c30e3a0c04..3ba157ed0d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -995,7 +995,7 @@ static void free_all(struct list_head *head)\n \t}\n }\n \n-static struct tempfile trailers_tempfile;\n+static struct tempfile *trailers_tempfile;\n \n static FILE *create_in_place_tempfile(const char *file)\n {\n@@ -1017,9 +1017,9 @@ static FILE *create_in_place_tempfile(const char *file)\n \t\tstrbuf_add(&template, file, tail - file + 1);\n \tstrbuf_addstr(&template, \"git-interpret-trailers-XXXXXX\");\n \n-\txmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);\n+\ttrailers_tempfile = xmks_tempfile_m(template.buf, st.st_mode);\n \tstrbuf_release(&template);\n-\toutfile = fdopen_tempfile(&trailers_tempfile, \"w\");\n+\toutfile = fdopen_tempfile(trailers_tempfile, \"w\");\n \tif (!outfile)\n \t\tdie_errno(_(\"could not open temporary file\"));\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327565","messageId":"20170905121511.7s5ukj5jnj2mitnn@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 18/20] lockfile: update lifetime requirements in documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:12Z","receivedAt":"2017-09-05T12:15:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that the tempfile system we rely on has loosened the\nlifetime requirements for storage, we can adjust our\ndocumentation to match.\n\nSigned-off-by: Jeff King \n---\n lockfile.h | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 6ed336e2bc..7c1c484d7c 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -37,12 +37,12 @@\n *\n * The caller:\n *\n- * * Allocates a `struct lock_file` either as a static variable or on\n- * the heap, initialized to zeros. Once you use the structure to\n- * call the `hold_lock_file_for_*()` family of functions, it belongs\n- * to the lockfile subsystem and its storage must remain valid\n- * throughout the life of the program (i.e. you cannot use an\n- * on-stack variable to hold this structure).\n+ * * Allocates a `struct lock_file` with whatever storage duration you\n+ * desire. The struct does not have to be initialized before being\n+ * used, but it is good practice to do so using by setting it to\n+ * all-zeros (or using the LOCK_INIT macro). This puts the object in a\n+ * consistent state that allows you to call rollback_lock_file() even\n+ * if the lock was never taken (in which case it is a noop).\n *\n * * Attempts to create a lockfile by calling `hold_lock_file_for_update()`.\n *\n@@ -73,10 +73,8 @@\n * `commit_lock_file()`, `commit_lock_file_to()`,\n * `rollback_lock_file()`, or `reopen_lock_file()`.\n *\n- * Even after the lockfile is committed or rolled back, the\n- * `lock_file` object must not be freed or altered by the caller.\n- * However, it may be reused; just pass it to another call of\n- * `hold_lock_file_for_update()`.\n+ * After the lockfile is committed or rolled back, the `lock_file`\n+ * object can be discarded or reused.\n *\n * If the program exits before `commit_lock_file()`,\n * `commit_lock_file_to()`, or `rollback_lock_file()` is called, the\n@@ -114,6 +112,8 @@ struct lock_file {\n \tstruct tempfile *tempfile;\n };\n \n+#define LOCK_INIT { NULL }\n+\n /* String appended to a filename to derive the lockfile name: */\n #define LOCK_SUFFIX \".lock\"\n #define LOCK_SUFFIX_LEN 5\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327566","messageId":"20170905121515.xejuaiadexhmya2f@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 19/20] ref_lock: stop leaking lock_files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:15Z","receivedAt":"2017-09-05T12:15:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the tempfile code recently relaxed the rule that\ntempfile structs (and thus locks) need to hang around\nforever, we no longer have to leak our lock_file structs.\n\nIn fact, we don't even need to heap-allocate them anymore,\nsince their lifetime can just match that of the surrounding\nref_lock (and if we forget to delete a lock, the effect is\nthe same as before: it will eventually go away at program\nexit).\n\nNote that there is a check in unlock_ref() to only rollback\na lock file if it has been allocated. We don't need that\ncheck anymore; we zero the ref_lock (and thus the\nlock_file), so at worst we pass a NULL pointer to\ndelete_tempfile(), which considers that a noop.\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 39 ++++++++++++++++-----------------------\n 1 file changed, 16 insertions(+), 23 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 36fe8a986b..f3455609d6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -12,7 +12,7 @@\n \n struct ref_lock {\n \tchar *ref_name;\n-\tstruct lock_file *lk;\n+\tstruct lock_file lk;\n \tstruct object_id old_oid;\n };\n \n@@ -418,9 +418,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \n static void unlock_ref(struct ref_lock *lock)\n {\n-\t/* Do not free lock->lk -- atexit() still looks at them */\n-\tif (lock->lk)\n-\t\trollback_lock_file(lock->lk);\n+\trollback_lock_file(&lock->lk);\n \tfree(lock->ref_name);\n \tfree(lock);\n }\n@@ -534,11 +532,8 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (!lock->lk)\n-\t\tlock->lk = xcalloc(1, sizeof(struct lock_file));\n-\n \tif (hold_lock_file_for_update_timeout(\n-\t\t\t lock->lk, ref_file.buf, LOCK_NO_DEREF,\n+\t\t\t &lock->lk, ref_file.buf, LOCK_NO_DEREF,\n \t\t\t get_files_ref_lock_timeout_ms()) < 0) {\n \t\tif (errno == ENOENT && --attempts_remaining > 0) {\n \t\t\t/*\n@@ -949,11 +944,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tlock->lk = xcalloc(1, sizeof(struct lock_file));\n-\n \tlock->ref_name = xstrdup(refname);\n \n-\tif (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {\n+\tif (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {\n \t\tlast_errno = errno;\n \t\tunable_to_lock_message(ref_file.buf, errno, err);\n \t\tgoto error_return;\n@@ -1404,14 +1397,14 @@ static int files_rename_ref(struct ref_store *ref_store,\n \n static int close_ref_gently(struct ref_lock *lock)\n {\n-\tif (close_lock_file_gently(lock->lk))\n+\tif (close_lock_file_gently(&lock->lk))\n \t\treturn -1;\n \treturn 0;\n }\n \n static int commit_ref(struct ref_lock *lock)\n {\n-\tchar *path = get_locked_file_path(lock->lk);\n+\tchar *path = get_locked_file_path(&lock->lk);\n \tstruct stat st;\n \n \tif (!lstat(path, &st) && S_ISDIR(st.st_mode)) {\n@@ -1435,7 +1428,7 @@ static int commit_ref(struct ref_lock *lock)\n \t\tfree(path);\n \t}\n \n-\tif (commit_lock_file(lock->lk))\n+\tif (commit_lock_file(&lock->lk))\n \t\treturn -1;\n \treturn 0;\n }\n@@ -1627,12 +1620,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tfd = get_lock_file_fd(lock->lk);\n+\tfd = get_lock_file_fd(&lock->lk);\n \tif (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n \t write_in_full(fd, &term, 1) != 1 ||\n \t close_ref_gently(lock) < 0) {\n \t\tstrbuf_addf(err,\n-\t\t\t \"couldn't write '%s'\", get_lock_file_path(lock->lk));\n+\t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n@@ -1709,7 +1702,7 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)\n {\n \tint ret = -1;\n #ifndef NO_SYMLINK_HEAD\n-\tchar *ref_path = get_locked_file_path(lock->lk);\n+\tchar *ref_path = get_locked_file_path(&lock->lk);\n \tunlink(ref_path);\n \tret = symlink(target, ref_path);\n \tfree(ref_path);\n@@ -1745,14 +1738,14 @@ static int create_symref_locked(struct files_ref_store *refs,\n \t\treturn 0;\n \t}\n \n-\tif (!fdopen_lock_file(lock->lk, \"w\"))\n+\tif (!fdopen_lock_file(&lock->lk, \"w\"))\n \t\treturn error(\"unable to fdopen %s: %s\",\n-\t\t\t lock->lk->tempfile->filename.buf, strerror(errno));\n+\t\t\t lock->lk.tempfile->filename.buf, strerror(errno));\n \n \tupdate_symref_reflog(refs, lock, refname, target, logmsg);\n \n \t/* no error check; commit_ref will check ferror */\n-\tfprintf(lock->lk->tempfile->fp, \"ref: %s\\n\", target);\n+\tfprintf(lock->lk.tempfile->fp, \"ref: %s\\n\", target);\n \tif (commit_ref(lock) < 0)\n \t\treturn error(\"unable to write symref for %s: %s\", refname,\n \t\t\t strerror(errno));\n@@ -2853,12 +2846,12 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t\t\t\tstrerror(errno));\n \t\t\trollback_lock_file(&reflog_lock);\n \t\t} else if (update &&\n-\t\t\t (write_in_full(get_lock_file_fd(lock->lk),\n+\t\t\t (write_in_full(get_lock_file_fd(&lock->lk),\n \t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n-\t\t\t write_str_in_full(get_lock_file_fd(lock->lk), \"\\n\") != 1 ||\n+\t\t\t write_str_in_full(get_lock_file_fd(&lock->lk), \"\\n\") != 1 ||\n \t\t\t close_ref_gently(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n-\t\t\t\t\tget_lock_file_path(lock->lk));\n+\t\t\t\t\tget_lock_file_path(&lock->lk));\n \t\t\trollback_lock_file(&reflog_lock);\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"unable to write reflog '%s' (%s)\",\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327567","messageId":"20170905121521.67kehubcat5klvix@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 20/20] stop leaking lock structs in some simple cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:21Z","receivedAt":"2017-09-05T12:16:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that it's safe to declare a \"struct lock_file\" on the\nstack, we can do so (and avoid an intentional leak). These\nleaks were found by running t0000 and t0001 under valgrind\n(though certainly other similar leaks exist and just don't\nhappen to be exercised by those tests).\n\nInitializing the lock_file's inner tempfile with NULL is not\nstrictly necessary in these cases, but it's a good practice\nto model. It means that if we were to call a function like\nrollback_lock_file() on a lock that was never taken in the\nfirst place, it becomes a quiet noop (rather than undefined\nbehavior).\n\nLikewise, it's always safe to rollback_lock_file() on a file\nthat has already been committed or deleted, since that\noperation is a noop on an inactive lockfile (and that's why\nthe case in config.c can drop the \"if (lock)\" check as we\nmove away from using a pointer).\n\nSigned-off-by: Jeff King \n---\n builtin/reset.c | 6 +++---\n builtin/update-index.c | 11 ++++-------\n cache-tree.c | 14 ++++----------\n config.c | 24 +++++++-----------------\n 4 files changed, 18 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex d72c7d1c96..f1af9345e4 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -367,8 +367,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\tdie_if_unmerged_cache(reset_type);\n \n \tif (reset_type != SOFT) {\n-\t\tstruct lock_file *lock = xcalloc(1, sizeof(*lock));\n-\t\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n+\t\tstruct lock_file lock = LOCK_INIT;\n+\t\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \t\tif (reset_type == MIXED) {\n \t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n@@ -384,7 +384,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\tdie(_(\"Could not reset index file to revision '%s'.\"), rev);\n \t\t}\n \n-\t\tif (write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\t\tdie(_(\"Could not write new index file.\"));\n \t}\n \ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex d562f2ec69..655e6d60d3 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -915,7 +915,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tstruct parse_opt_ctx_t ctx;\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n@@ -1014,11 +1014,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n \t/* we will diagnose later if it turns out that we need to update it */\n-\tnewfd = hold_locked_index(lock_file, 0);\n+\tnewfd = hold_locked_index(&lock_file, 0);\n \tif (newfd < 0)\n \t\tlock_error = errno;\n \n@@ -1153,11 +1150,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t\t}\n-\t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n \t}\n \n-\trollback_lock_file(lock_file);\n+\trollback_lock_file(&lock_file);\n \n \treturn has_errors ? 1 : 0;\n }\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2690113a6a..71d092ed51 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -603,16 +603,10 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n \tint entries, was_valid, newfd;\n-\tstruct lock_file *lock_file;\n+\tstruct lock_file lock_file = LOCK_INIT;\n \tint ret = 0;\n \n-\t/*\n-\t * We can't free this memory, it becomes part of a linked list\n-\t * parsed atexit()\n-\t */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n-\tnewfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);\n+\tnewfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n \tentries = read_index_from(index_state, index_path);\n \tif (entries < 0) {\n@@ -632,7 +626,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \t\t\tgoto out;\n \t\t}\n \t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(index_state, lock_file, COMMIT_LOCK))\n+\t\t\tif (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))\n \t\t\t\tnewfd = -1;\n \t\t}\n \t\t/* Not being able to write is fine -- we are only interested\n@@ -657,7 +651,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co\n \n out:\n \tif (0 <= newfd)\n-\t\trollback_lock_file(lock_file);\n+\t\trollback_lock_file(&lock_file);\n \treturn ret;\n }\n \ndiff --git a/config.c b/config.c\nindex d0d8ce823a..7931182a54 100644\n--- a/config.c\n+++ b/config.c\n@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n {\n \tint fd = -1, in_fd = -1;\n \tint ret;\n-\tstruct lock_file *lock = NULL;\n+\tstruct lock_file lock = LOCK_INIT;\n \tchar *filename_buf = NULL;\n \tchar *contents = NULL;\n \tsize_t contents_sz;\n@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tlock = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror_errno(\"could not lock config file %s\", config_filename);\n \t\tfree(store.key);\n@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tclose(in_fd);\n \t\tin_fd = -1;\n \n-\t\tif (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {\n-\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(lock));\n+\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n+\t\t\terror_errno(\"chmod on %s failed\", get_lock_file_path(&lock));\n \t\t\tret = CONFIG_NO_WRITE;\n \t\t\tgoto out_free;\n \t\t}\n@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tcontents = NULL;\n \t}\n \n-\tif (commit_lock_file(lock) < 0) {\n+\tif (commit_lock_file(&lock) < 0) {\n \t\terror_errno(\"could not write config file %s\", config_filename);\n \t\tret = CONFIG_NO_WRITE;\n-\t\tlock = NULL;\n \t\tgoto out_free;\n \t}\n \n-\t/*\n-\t * lock is committed, so don't try to roll it back below.\n-\t * NOTE: Since lockfile.c keeps a linked list of all created\n-\t * lock_file structures, it isn't safe to free(lock). It's\n-\t * better to just leave it hanging around.\n-\t */\n-\tlock = NULL;\n \tret = 0;\n \n \t/* Invalidate the config cache */\n \tgit_config_clear();\n \n out_free:\n-\tif (lock)\n-\t\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \tfree(filename_buf);\n \tif (contents)\n \t\tmunmap(contents, contents_sz);\n@@ -2669,7 +2659,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(get_lock_file_path(lock));\n+\tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n \n }\n-- \n2.14.1.721.gc5bc1565f1\n"},{"id":"327568","messageId":"20170905121500.sl5vvgnguux4mtn5@sigill.intra.peff.net","threadId":"46703","inReplyTo":"20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net","subject":"[PATCH 15/20] tempfile: use list.h for linked list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T12:15:00Z","receivedAt":"2017-09-05T12:16:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tempfile API keeps to-be-cleaned tempfiles in a\nsingly-linked list and never removes items from the list. A\nfuture patch would like to start removing items, but removal\nfrom a singly linked list is O(n), as we have to walk the\nlist to find the predecessor element. This means that a\nprocess which takes \"n\" simultaneous lockfiles (for example,\nan atomic transaction on \"n\" refs) may end up quadratic in\n\"n\".\n\nBefore we start allowing items to be removed, it would be\nnice to have a way to cover this case in linear time.\n\nThe simplest solution is to make an assumption about the\norder in which tempfiles are added and removed from the\nlist. If both operations iterate over the tempfiles in the\nsame order, then by putting new items at the end of the list\nour removal search will always find its items at the\nbeginning of the list. And indeed, that would work for the\ncase of refs. But it creates a hidden dependency between\nunrelated parts of the code. If anybody changes the ref code\n(or if we add a new caller that opens multiple simultaneous\ntempfiles) they may unknowingly introduce a performance\nregression.\n\nAnother solution is to use a better data structure. A\ndoubly-linked list works fine, and we already have an\nimplementation in list.h. But there's one snag: the elements\nof \"struct tempfile\" are all marked as \"volatile\", since a\nsignal handler may interrupt us and iterate over the list at\nany moment (even if we were in the middle of adding a new\nentry).\n\nWe can declare a \"volatile struct list_head\", but we can't\nactually use it with the normal list functions. The compiler\ncomplains about passing a pointer-to-volatile via a regular\npointer argument. And rightfully so, as the sub-function\nwould potentially need different code to deal with the\nvolatile case.\n\nThat leaves us with a few options:\n\n 1. Drop the \"volatile\" modifier for the list items.\n\n This is probably a bad idea. I checked the assembly\n output from \"gcc -O2\", and the \"volatile\" really does\n impact the order in which it updates memory.\n\n 2. Use macros instead of inline functions. The irony here\n is that list.h is entirely implemented as trivial\n inline functions. So we basically are already\n generating custom code for each call. But sadly there's no\n way in C to declare the inline function to take a more\n generic type.\n\n We could do so by switching the inline functions to\n macros, but it does make the end result harder to read.\n And it doesn't fully solve the problem (for instance,\n the declaration of list_head needs to change so that\n its \"prev\" and \"next\" pointers point to other volatile\n structs).\n\n 3. Don't use list.h, and just make our own ad-hoc\n doubly-linked list. It's not that much code to\n implement the basics that we need here. But if we're\n going to do so, why not add the few extra lines\n required to model it after the actual list.h interface?\n We can even reuse a few of the macro helpers.\n\nSo this patch takes option 3, but actually implements a\nparallel \"volatile list\" interface in list.h, where it could\npotentially be reused by other code. This implements just\nenough for tempfile.c's use, though we could easily port\nother functions later if need be.\n\nSigned-off-by: Jeff King \n---\n list.h | 38 ++++++++++++++++++++++++++++++++++++++\n tempfile.c | 13 +++++++------\n tempfile.h | 4 +++-\n 3 files changed, 48 insertions(+), 7 deletions(-)\n\ndiff --git a/list.h b/list.h\nindex a226a870dc..eb601192f4 100644\n--- a/list.h\n+++ b/list.h\n@@ -163,4 +163,42 @@ static inline void list_replace_init(struct list_head *old,\n \tINIT_LIST_HEAD(old);\n }\n \n+/*\n+ * This is exactly the same as a normal list_head, except that it can be\n+ * declared volatile (e.g., if you have a list that may be accessed from signal\n+ * handlers).\n+ */\n+struct volatile_list_head {\n+\tvolatile struct volatile_list_head *next, *prev;\n+};\n+\n+#define VOLATILE_LIST_HEAD(name) \\\n+\tvolatile struct volatile_list_head name = { &(name), &(name) }\n+\n+static inline void __volatile_list_del(volatile struct volatile_list_head *prev,\n+\t\t\t\t volatile struct volatile_list_head *next)\n+{\n+\tnext->prev = prev;\n+\tprev->next = next;\n+}\n+\n+static inline void volatile_list_del(volatile struct volatile_list_head *elem)\n+{\n+\t__volatile_list_del(elem->prev, elem->next);\n+}\n+\n+static inline int volatile_list_empty(volatile struct volatile_list_head *head)\n+{\n+\treturn head == head->next;\n+}\n+\n+static inline void volatile_list_add(volatile struct volatile_list_head *newp,\n+\t\t\t\t volatile struct volatile_list_head *head)\n+{\n+\thead->next->prev = newp;\n+\tnewp->next = head->next;\n+\tnewp->prev = head;\n+\thead->next = newp;\n+}\n+\n #endif /* LIST_H */\ndiff --git a/tempfile.c b/tempfile.c\nindex e655e28477..11bda824cf 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -55,14 +55,16 @@\n #include \"tempfile.h\"\n #include \"sigchain.h\"\n \n-static struct tempfile *volatile tempfile_list;\n+static VOLATILE_LIST_HEAD(tempfile_list);\n \n static void remove_tempfiles(int in_signal_handler)\n {\n \tpid_t me = getpid();\n-\tstruct tempfile *volatile p;\n+\tvolatile struct volatile_list_head *pos;\n+\n+\tlist_for_each(pos, &tempfile_list) {\n+\t\tstruct tempfile *p = list_entry(pos, struct tempfile, list);\n \n-\tfor (p = tempfile_list; p; p = p->next) {\n \t\tif (!is_tempfile_active(p) || p->owner != me)\n \t\t\tcontinue;\n \n@@ -95,7 +97,7 @@ static void remove_tempfiles_on_signal(int signo)\n */\n static void prepare_tempfile_object(struct tempfile *tempfile)\n {\n-\tif (!tempfile_list) {\n+\tif (volatile_list_empty(&tempfile_list)) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_tempfiles_on_signal);\n \t\tatexit(remove_tempfiles_on_exit);\n@@ -110,8 +112,7 @@ static void prepare_tempfile_object(struct tempfile *tempfile)\n \t\ttempfile->active = 0;\n \t\ttempfile->owner = 0;\n \t\tstrbuf_init(&tempfile->filename, 0);\n-\t\ttempfile->next = tempfile_list;\n-\t\ttempfile_list = tempfile;\n+\t\tvolatile_list_add(&tempfile->list, &tempfile_list);\n \t\ttempfile->on_list = 1;\n \t} else if (tempfile->filename.len) {\n \t\t/* This shouldn't happen, but better safe than sorry. */\ndiff --git a/tempfile.h b/tempfile.h\nindex d30663182d..2ee24f4380 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -1,6 +1,8 @@\n #ifndef TEMPFILE_H\n #define TEMPFILE_H\n \n+#include \"list.h\"\n+\n /*\n * Handle temporary files.\n *\n@@ -81,7 +83,7 @@\n */\n \n struct tempfile {\n-\tstruct tempfile *volatile next;\n+\tvolatile struct volatile_list_head list;\n \tvolatile sig_atomic_t active;\n \tvolatile int fd;\n \tFILE *volatile fp;\n-- \n2.14.1.721.gc5bc1565f1\n\n"}]}