{"thread":{"id":"58355","subject":"curiosities with tempfile.active","startedAt":"2022-08-24T09:35:40Z","lastAt":"2022-08-30T19:46:09Z","messageCount":8,"participants":["Jeff King","René Scharfe","Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"461892","messageId":"YwXw2ytUlrXSSRh7@coredump.intra.peff.net","threadId":"58355","inReplyTo":null,"subject":"curiosities with tempfile.active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-24T09:35:23Z","receivedAt":"2022-08-24T09:35:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[starting a new thread, as this ended up rather long;\n +cc René as there's an interesting twist at the end]\n\nOn Tue, Aug 23, 2022 at 11:12:21AM +0200, Johannes Schindelin wrote:\n\n> Side note: I do notice that `delete_tempfile(&tmpedit)` seems to _not_\n> release memory when `tmpedit` is non-NULL when `tmpedit->active == 0`.\n> I consider this a bug in the `delete_tempfile()` code (in its `if\n> (!is_tempfile_active(tempfile))` clause, it should call\n> `deactivate_tempfile()` for non-NULL `tempfile`s and set `*tempfile_p =\n> NULL;`), but it is outside the scope of your patch to address that.\n\nI agree it is confusing, but I think this turns out not to be a bug in\npractice. In ye olden days, tempfile structs lived on the global\nlinked-list of entries to cleanup forever, so they were statically\nallocated or effectively leaked from the heap. And that's why we needed\nan \"active\" flag in the first place.\n\nThat changed around 422a21c6a0 (tempfile: remove deactivated list\nentries, 2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles\non heap, 2017-09-05). Now calling deactivate_tempfile() unsets the\nactive flag _and_ frees the struct. And in normal calling, that's the\nonly way to unset the active flag (it also starts life unset, but the\ncreation functions always activate on success, or deactivate+free on\nerror).\n\nSo can we just get rid of the active flag? Possibly. I said \"normal\ncalling\" above, because the exception is in our remove_tempfiles()\ncleanup routine, where we unset the \"active\" flag as we remove each\nfile. We can't use the regular delete_tempfile(), or even\ndeactivate_tempfile() here, because we may be in a signal handler, so\nthings like free() are forbidden. So we just \"leak\" the memory, but it's\nOK because we're exiting (and even leak detection won't complain,\nbecause it's reachable from the global list).\n\nBut why do we care about the flag, then? We just iterate over the list\nonce, so we should handle each entry once. But it can be called both as\nan atexit() handler, as well as a signal handler. So it's possible that\nwe can be mid-iteration and get another signal (because the original was\nvia exit, or because we hook multiple signals for the cleanup). So the\nactive flag in theory is protecting us from that.\n\nBut it's not foolproof. We remove the tempfile and then unset the flag,\nso there's a moment (albeit small) where that other signal could come\nin. It shouldn't be a big deal because unlinking the tempfiles is mostly\nidempotent-ish; barring the unlikely chance that somebody else creates\nthe same random filename as us, the second unlink will do nothing. So\nwhile removing the active flag would increase the size of that race\n(from handling a single entry to completing the whole atexit), that\nshouldn't matter much in practice.\n\nBut here's the interesting twist. Commit 2c2db194bd (tempfile: add\nmks_tempfile_dt(), 2022-04-20) recently taught remove_tempfiles() to\ndrop a surrounding directory. And it does so by munging the filename\nbuffer. It has to, because we can't allocate a new buffer in a signal\nhandler.\n\nBut is it also idempotent(-ish)? The directory removal itself is,\nbecause it checks:\n\n  tempfile->filename.buf[tempfile->directorylen] == '/'\n\nbefore overwriting that byte with a NUL, so it should only be true once\n(though I note this violates the usual \"volatile\" rules for signal\nhandlers, it's probably OK in practice since we know the NUL will be\nwritten before we call rmdir()).\n\nBut after that happens, the filename buffer now contains a C string that\npoints to the directory. And if we end up in remove_tempfiles() again\ndue to another signal, we'll try to unlink(p->filename.buf), which will\nnow unlink the directory! I'm not sure how that behaves on all systems.\nOn Linux it's a noop. And if it just deleted the directory, that would\nbe OK. I seem to recall on old versions of SunOS it did bad things\n(maybe because it really did unlink it, but without checking for\norphaned entries inside).\n\nThe good news is that it won't walk further up the tree. I was worried\nthat a second round might then try to rmdir the parent (usually \"/tmp\"\nin this case). But the directory removal always uses the same length.\n\nSo...maybe it's all OK in practice? If not, this is an issue even\nwithout removing the active flag. But removing it would make the race\nwindow larger. I suspect it probably is OK in practice, but saying that\nabout a race always feels like famous last words.\n\nIf we did want to keep it, I suspect we could get the same effect by\nmunging the volatile \"pid_t owner\" field, but I think anything we munge\nis supposed to be a sig_atomic_t according to the letter of the law.\n\nPatch below shows what it would look like to just drop \"active\"\nentirely. Test suite behaves as expected, but again, the real question\nis what it might be doing in a weird racy situation. The one above, but\nalso a signal racing with adding an entry to the list. The volatile\nsig_atomic_t is what tells the removal process in the signal handler\nthat it's OK to look at. But again, maybe that's OK in practice.\n\n-Peff\n\n---\ndiff --git a/tempfile.c b/tempfile.c\nindex 2024c82691..6134b73972 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -89,8 +89,6 @@ static void remove_tempfiles(int in_signal_handler)\n \t\telse\n \t\t\tunlink_or_warn(p->filename.buf);\n \t\tremove_template_directory(p, in_signal_handler);\n-\n-\t\tp->active = 0;\n \t}\n }\n \n@@ -111,7 +109,6 @@ static struct tempfile *new_tempfile(void)\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@@ -123,9 +120,6 @@ 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@@ -134,12 +128,10 @@ static void activate_tempfile(struct tempfile *tempfile)\n \n \tvolatile_list_add(&tempfile->list, &tempfile_list);\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n }\n \n static void deactivate_tempfile(struct tempfile *tempfile)\n {\n-\ttempfile->active = 0;\n \tstrbuf_release(&tempfile->filename);\n \tvolatile_list_del(&tempfile->list);\n \tfree(tempfile);\ndiff --git a/tempfile.h b/tempfile.h\nindex d7804a214a..f0bf59dbac 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -77,7 +77,6 @@\n \n struct tempfile {\n \tvolatile struct volatile_list_head list;\n-\tvolatile sig_atomic_t active;\n \tvolatile int fd;\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n@@ -221,7 +220,7 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);\n \n static inline int is_tempfile_active(struct tempfile *tempfile)\n {\n-\treturn tempfile && tempfile->active;\n+\treturn !!tempfile;\n }\n \n /*\n"},{"id":"462018","messageId":"526a174e-b179-c284-a21c-7afe0a0b4df2@web.de","threadId":"58355","inReplyTo":"YwXw2ytUlrXSSRh7@coredump.intra.peff.net","subject":"Re: curiosities with tempfile.active","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-26T22:46:29Z","receivedAt":"2022-08-26T22:46:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.08.22 um 11:35 schrieb Jeff King:\n> But here's the interesting twist. Commit 2c2db194bd (tempfile: add\n> mks_tempfile_dt(), 2022-04-20) recently taught remove_tempfiles() to\n> drop a surrounding directory. And it does so by munging the filename\n> buffer. It has to, because we can't allocate a new buffer in a signal\n> handler.\n>\n> But is it also idempotent(-ish)? The directory removal itself is,\n> because it checks:\n>\n>   tempfile->filename.buf[tempfile->directorylen] == '/'\n>\n> before overwriting that byte with a NUL, so it should only be true once\n> (though I note this violates the usual \"volatile\" rules for signal\n> handlers, it's probably OK in practice since we know the NUL will be\n> written before we call rmdir()).\n>\n> But after that happens, the filename buffer now contains a C string that\n> points to the directory. And if we end up in remove_tempfiles() again\n> due to another signal, we'll try to unlink(p->filename.buf), which will\n> now unlink the directory! I'm not sure how that behaves on all systems.\n> On Linux it's a noop. And if it just deleted the directory, that would\n> be OK. I seem to recall on old versions of SunOS it did bad things\n> (maybe because it really did unlink it, but without checking for\n> orphaned entries inside).\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html\nsays of the unlink(2) parameter: \"The path argument shall not name a\ndirectory unless the process has appropriate privileges and the\nimplementation supports using unlink() on directories.\"  So we better\nnot do that..\n\n--- >8 ---\nSubject: [PATCH] tempfile: avoid directory cleanup race\n\nThe temporary directory created by mks_tempfile_dt() is deleted by first\ndeleting the file within, then truncating the filename strbuf and\npassing the resulting string to rmdir(2).  When the cleanup routine is\ninvoked concurrently by a signal handler we can end up passing the now\ntruncated string to unlink(2), however, which could cause problems on\nsome systems.\n\nAvoid that issue by remembering the directory name separately.  This way\nthe paths stay unchanged.  A signal handler can still race with normal\ncleanup, but deleting the same files and directories twice is harmless.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n tempfile.c | 14 ++++++--------\n tempfile.h |  2 +-\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 2024c82691..7414c81e31 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -59,14 +59,11 @@ static VOLATILE_LIST_HEAD(tempfile_list);\n static void remove_template_directory(struct tempfile *tempfile,\n \t\t\t\t      int in_signal_handler)\n {\n-\tif (tempfile->directorylen > 0 &&\n-\t    tempfile->directorylen < tempfile->filename.len &&\n-\t    tempfile->filename.buf[tempfile->directorylen] == '/') {\n-\t\tstrbuf_setlen(&tempfile->filename, tempfile->directorylen);\n+\tif (tempfile->directory) {\n \t\tif (in_signal_handler)\n-\t\t\trmdir(tempfile->filename.buf);\n+\t\t\trmdir(tempfile->directory);\n \t\telse\n-\t\t\trmdir_or_warn(tempfile->filename.buf);\n+\t\t\trmdir_or_warn(tempfile->directory);\n \t}\n }\n\n@@ -115,7 +112,7 @@ static struct tempfile *new_tempfile(void)\n \ttempfile->owner = 0;\n \tINIT_LIST_HEAD(&tempfile->list);\n \tstrbuf_init(&tempfile->filename, 0);\n-\ttempfile->directorylen = 0;\n+\ttempfile->directory = NULL;\n \treturn tempfile;\n }\n\n@@ -141,6 +138,7 @@ static void deactivate_tempfile(struct tempfile *tempfile)\n {\n \ttempfile->active = 0;\n \tstrbuf_release(&tempfile->filename);\n+\tfree(tempfile->directory);\n \tvolatile_list_del(&tempfile->list);\n \tfree(tempfile);\n }\n@@ -254,7 +252,7 @@ struct tempfile *mks_tempfile_dt(const char *directory_template,\n\n \ttempfile = new_tempfile();\n \tstrbuf_swap(&tempfile->filename, &sb);\n-\ttempfile->directorylen = directorylen;\n+\ttempfile->directory = xmemdupz(tempfile->filename.buf, directorylen);\n \ttempfile->fd = fd;\n \tactivate_tempfile(tempfile);\n \treturn tempfile;\ndiff --git a/tempfile.h b/tempfile.h\nindex d7804a214a..5b9e8743dd 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -82,7 +82,7 @@ struct tempfile {\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n \tstruct strbuf filename;\n-\tsize_t directorylen;\n+\tchar *directory;\n };\n\n /*\n--\n2.37.2\n\n"},{"id":"462043","messageId":"YwoV9/xAcWTRbUBG@coredump.intra.peff.net","threadId":"58355","inReplyTo":"526a174e-b179-c284-a21c-7afe0a0b4df2@web.de","subject":"Re: curiosities with tempfile.active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-27T13:02:47Z","receivedAt":"2022-08-27T13:02:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 27, 2022 at 12:46:29AM +0200, René Scharfe wrote:\n\n> https://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html\n> says of the unlink(2) parameter: \"The path argument shall not name a\n> directory unless the process has appropriate privileges and the\n> implementation supports using unlink() on directories.\"  So we better\n> not do that..\n\nYeah, I saw that. It's a bit vague, and if the call returns ENOSYS or\nEISDIR, that would be perfectly fine. It's the \"what happens on the\nimplementations that do support it...\" part that I'm more worried about. :)\n\nThat said...\n\n> --- >8 ---\n> Subject: [PATCH] tempfile: avoid directory cleanup race\n> \n> The temporary directory created by mks_tempfile_dt() is deleted by first\n> deleting the file within, then truncating the filename strbuf and\n> passing the resulting string to rmdir(2).  When the cleanup routine is\n> invoked concurrently by a signal handler we can end up passing the now\n> truncated string to unlink(2), however, which could cause problems on\n> some systems.\n> \n> Avoid that issue by remembering the directory name separately.  This way\n> the paths stay unchanged.  A signal handler can still race with normal\n> cleanup, but deleting the same files and directories twice is harmless.\n\nRight, I'm a little embarrassed I didn't immediately come up with this\nsame fix. This is definitely the right thing to do. The more we can\ntreat data from signal-handlers are write-once, the better.\n\nThere's a slight extra memory cost to store the directory name twice,\nbut it's a drop in the bucket overall.\n\n>  tempfile.c | 14 ++++++--------\n>  tempfile.h |  2 +-\n\nThe patch itself looks good to me.\n\n-Peff\n"},{"id":"462045","messageId":"CAPx1Gvddmk8cfWGg7M8gJ=rxnoTgQRgQNX95BgYcQk1N3VUi8A@mail.gmail.com","threadId":"58355","inReplyTo":"YwoV9/xAcWTRbUBG@coredump.intra.peff.net","subject":"Re: curiosities with tempfile.active","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2022-08-27T21:47:45Z","receivedAt":"2022-08-27T21:48:01Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sat, Aug 27, 2022 at 6:05 AM Jeff King <peff@peff.net> wrote:\n> Yeah, I saw that. It's a bit vague, and if the call returns ENOSYS or\n> EISDIR, that would be perfectly fine. It's the \"what happens on the\n> implementations that do support it...\" part that I'm more worried about. :)\n\nThe history here is that pre-4.2BSD, Unix systems had no mkdir\nsystem call. You used mknod() to make a truly empty directory and\nthe link() to create the \".\" and \"..\" entries within it, and all three of\nthese operations were restricted to the super-user.  There was no\nrmdir either, so again, unlink() as the super-user was permitted to\ndo the job (with three calls to unlink the \".\" and \"..\" entries first and\nthen remove the directory).\n\nUnlinking a directory when it still contains \".\" leaves the link count\nat 1 and there's no GC, so it sits around occupying an inode.\n\nIf you have a mkdir() system call and don't need backwards\ncompatibility, you get to have these return EISDIR errors...\n\nChris\n"},{"id":"462229","messageId":"Yw5dT6I7QKkfuQAF@coredump.intra.peff.net","threadId":"58355","inReplyTo":"CAPx1Gvddmk8cfWGg7M8gJ=rxnoTgQRgQNX95BgYcQk1N3VUi8A@mail.gmail.com","subject":"Re: curiosities with tempfile.active","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T18:56:15Z","receivedAt":"2022-08-30T18:56:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 27, 2022 at 02:47:45PM -0700, Chris Torek wrote:\n\n> On Sat, Aug 27, 2022 at 6:05 AM Jeff King <peff@peff.net> wrote:\n> > Yeah, I saw that. It's a bit vague, and if the call returns ENOSYS or\n> > EISDIR, that would be perfectly fine. It's the \"what happens on the\n> > implementations that do support it...\" part that I'm more worried about. :)\n> \n> The history here is that pre-4.2BSD, Unix systems had no mkdir\n> system call. You used mknod() to make a truly empty directory and\n> the link() to create the \".\" and \"..\" entries within it, and all three of\n> these operations were restricted to the super-user.  There was no\n> rmdir either, so again, unlink() as the super-user was permitted to\n> do the job (with three calls to unlink the \".\" and \"..\" entries first and\n> then remove the directory).\n> \n> Unlinking a directory when it still contains \".\" leaves the link count\n> at 1 and there's no GC, so it sits around occupying an inode.\n\nThanks, that matches the sense of unease I had in the back of my mind. I\nseem to recall that maybe older versions of SunOS exhibited this, but\nthat feels like a lifetime ago. At any rate, we should avoid that\nunlink() call, and René's patch neatly does so.\n\n-Peff\n"},{"id":"462233","messageId":"Yw5nmNJSm9uzLOKL@coredump.intra.peff.net","threadId":"58355","inReplyTo":"526a174e-b179-c284-a21c-7afe0a0b4df2@web.de","subject":"[PATCH 0/2] cleaning up tempfile active flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T19:40:08Z","receivedAt":"2022-08-30T19:40:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 27, 2022 at 12:46:29AM +0200, René Scharfe wrote:\n\n> Subject: [PATCH] tempfile: avoid directory cleanup race\n\nThanks again for this patch. With it, I think it's reasonable to do the\nfollowing cleanup on top.\n\n  [1/2]: tempfile: drop active flag\n  [2/2]: tempfile: update comment describing state transitions\n\n tempfile.c | 28 ++++++----------------------\n tempfile.h |  3 +--\n 2 files changed, 7 insertions(+), 24 deletions(-)\n\n-Peff\n"},{"id":"462234","messageId":"Yw5owr6JrxE8L7jT@coredump.intra.peff.net","threadId":"58355","inReplyTo":"Yw5nmNJSm9uzLOKL@coredump.intra.peff.net","subject":"[PATCH 1/2] tempfile: drop active flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T19:45:06Z","receivedAt":"2022-08-30T19:45:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our tempfile struct contains an \"active\" flag. Long ago, this flag was\nimportant: tempfile structs were always allocated for the lifetime of\nthe program and added to a global linked list, and the active flag was\nwhat told us whether a struct's tempfile needed to be cleaned up on\nexit.\n\nBut since 422a21c6a0 (tempfile: remove deactivated list entries,\n2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles on heap,\n2017-09-05), we actually remove items from the list, and the active flag\nis generally always set to true for any allocated struct. We set it to\ntrue in all of the creation functions, and in the normal code flow it\nbecomes false only in deactivate_tempfile(), which then immediately\nfrees the struct.\n\nSo the flag isn't performing that role anymore, and in fact makes things\nmore confusing. Dscho noted that delete_tempfile() is a noop for an\ninactive struct. Since 076aa2cbda taught it to free the struct when\ndeactivating, we'd leak any struct whose active flag is unset. But in\npractice it's not a leak, because again, we'll free when we unset the\nflag, and never see the allocated-but-inactive state.\n\nCan we just get rid of the flag? The answer is yes, but it requires\nlooking at a few other spots:\n\n  1. I said above that the flag only becomes false before we deallocate,\n     but there's one exception: when we call remove_tempfiles() from a\n     signal or atexit handler, we unset the active flag as we remove\n     each file. This isn't important for delete_tempfile(), as nobody\n     would call it anymore, since we're exiting.\n\n     It does in theory provide us some protection against racily\n     double-removing a tempfile. If we receive a second signal while we\n     are already in the cleanup routines, we'll start the cleanup loop\n     again, and may visit the same tempfile. But this race already\n     exists, because calling unlink() and unsetting the active flag\n     aren't atomic! And it's OK in practice, because unlink() is\n     idempotent (barring the unlikely event that some other process\n     chooses our exact temp filename in that instant).\n\n     So dropping the active flag widens the race a bit, but it was\n     already there, and is fairly harmless in practice. If we really\n     care about addressing it, the right thing is probably to block\n     further signals while we're doing our cleanup (which we could\n     actually do atomically).\n\n  2. The active flag is declared as \"volatile sig_atomic_t\". The idea is\n     that it's the final bit that gets set to tell the cleanup routines\n     that the tempfile is ready to be used (or not used), and it's safe\n     to receive a signal racing with regular code which adds or removes\n     a tempfile from the list.\n\n     In practice, I don't think this is buying us anything. The presence\n     on the linked list is really what tells the cleanup routines to\n     look at the struct. That is already marked as \"volatile\". It's not\n     a sig_atomic_t, so it's possible that we could see a sheared write\n     there as an entry is added or removed. But that is true of the\n     current code, too! Before we can even look at the \"active\" flag,\n     we'd have to follow a link to the struct itself. If we see a\n     sheared write in the pointer to the struct, then we'll look at\n     garbage memory anyway, and there's not much we can do.\n\nThis patch removes the active flag entirely, using presence on the\nglobal linked list as an indicator that a tempfile ought to be cleaned\nup. We are already careful to add to the list as the final step in\nactivating. On deactivation, we'll make sure to remove from the list as\nthe first step, before freeing any fields. The use of the volatile\nkeyword should mean that those things happen in the expected order.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI recognize this is a high-risk cleanup, as there's a lot of hand-waving\nabout volatile and sig_atomic_t. But I'm fairly convinced it's not\nmaking anything materially worse. And while the number of dropped lines\nis small, I hope it makes this tricky code easier to reason about.\n\n tempfile.c | 10 +---------\n tempfile.h |  3 +--\n 2 files changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 7414c81e31..29e61077a8 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -86,8 +86,6 @@ static void remove_tempfiles(int in_signal_handler)\n \t\telse\n \t\t\tunlink_or_warn(p->filename.buf);\n \t\tremove_template_directory(p, in_signal_handler);\n-\n-\t\tp->active = 0;\n \t}\n }\n \n@@ -108,7 +106,6 @@ static struct tempfile *new_tempfile(void)\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@@ -120,9 +117,6 @@ 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@@ -131,15 +125,13 @@ static void activate_tempfile(struct tempfile *tempfile)\n \n \tvolatile_list_add(&tempfile->list, &tempfile_list);\n \ttempfile->owner = getpid();\n-\ttempfile->active = 1;\n }\n \n static void deactivate_tempfile(struct tempfile *tempfile)\n {\n-\ttempfile->active = 0;\n+\tvolatile_list_del(&tempfile->list);\n \tstrbuf_release(&tempfile->filename);\n \tfree(tempfile->directory);\n-\tvolatile_list_del(&tempfile->list);\n \tfree(tempfile);\n }\n \ndiff --git a/tempfile.h b/tempfile.h\nindex 5b9e8743dd..d0413af733 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -77,7 +77,6 @@\n \n struct tempfile {\n \tvolatile struct volatile_list_head list;\n-\tvolatile sig_atomic_t active;\n \tvolatile int fd;\n \tFILE *volatile fp;\n \tvolatile pid_t owner;\n@@ -221,7 +220,7 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);\n \n static inline int is_tempfile_active(struct tempfile *tempfile)\n {\n-\treturn tempfile && tempfile->active;\n+\treturn !!tempfile;\n }\n \n /*\n-- \n2.37.3.1051.g85dc4064ac\n\n"},{"id":"462235","messageId":"Yw5o+Wowr6LkMV0R@coredump.intra.peff.net","threadId":"58355","inReplyTo":"Yw5nmNJSm9uzLOKL@coredump.intra.peff.net","subject":"[PATCH 2/2] tempfile: update comment describing state transitions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T19:46:01Z","receivedAt":"2022-08-30T19:46:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Back when 1a9d15db25 (tempfile: a new module for handling temporary\nfiles, 2015-08-10) added this comment, tempfile structs were held in\nmemory for the life of a process, and there were various guarantees\nabout which fields were valid in which states.\n\nSince 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05)\nand 076aa2cbda (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\nthe flow is quite different: objects come and go from the list, and\ninactive ones are deallocated. And the previous commit removed the\n\"active\" flag from the struct entirely.\n\nLet's bring the comment up to date with the current code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis could arguably come first in the series, minus the bit about the\n\"active\" flag, since it is really putting the finishing touches on those\nold commits from 2017. But it felt easier to reason about the end state\nafter dropping the flag entirely.\n\n tempfile.c | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 29e61077a8..e27048f970 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -14,16 +14,14 @@\n  *\n  * The possible states of a `tempfile` object are as follows:\n  *\n- * - Uninitialized. In this state the object's `on_list` field must be\n- *   zero but the rest of its contents need not be initialized. As\n- *   soon as the object is used in any way, it is irrevocably\n- *   registered in `tempfile_list`, and `on_list` is set.\n+ * - Inactive/unallocated. The only way to get a tempfile is via a creation\n+ *   function like create_tempfile(). Once allocated, the tempfile is on the\n+ *   global tempfile_list and considered active.\n  *\n  * - Active, file open (after `create_tempfile()` or\n  *   `reopen_tempfile()`). In this state:\n  *\n  *   - the temporary file exists\n- *   - `active` is set\n  *   - `filename` holds the filename of the temporary file\n  *   - `fd` holds a file descriptor open for writing to it\n  *   - `fp` holds a pointer to an open `FILE` object if and only if\n@@ -35,14 +33,8 @@\n  *   `fd` is -1, and `fp` is `NULL`.\n  *\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- *     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 removed from `tempfile_list` (but could be used again)\n+ *   failed attempt to create a temporary file). The struct is removed from\n+ *   the global tempfile_list and deallocated.\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-- \n2.37.3.1051.g85dc4064ac\n"}]}