curiosities with tempfile.active

[starting a new thread, as this ended up rather long;
 +cc René as there's an interesting twist at the end]

On Tue, Aug 23, 2022 at 11:12:21AM +0200, Johannes Schindelin wrote:

I agree it is confusing, but I think this turns out not to be a bug in practice. In ye olden days, tempfile structs lived on the global linked-list of entries to cleanup forever, so they were statically allocated or effectively leaked from the heap. And that's why we needed an "active" flag in the first place.

That changed around 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles on heap, 2017-09-05). Now calling deactivate_tempfile() unsets the active flag _and_ frees the struct. And in normal calling, that's the only way to unset the active flag (it also starts life unset, but the creation functions always activate on success, or deactivate+free on error).

So can we just get rid of the active flag? Possibly. I said "normal calling" above, because the exception is in our remove_tempfiles() cleanup routine, where we unset the "active" flag as we remove each file. We can't use the regular delete_tempfile(), or even deactivate_tempfile() here, because we may be in a signal handler, so things like free() are forbidden. So we just "leak" the memory, but it's OK because we're exiting (and even leak detection won't complain, because it's reachable from the global list).

But why do we care about the flag, then? We just iterate over the list once, so we should handle each entry once. But it can be called both as an atexit() handler, as well as a signal handler. So it's possible that we can be mid-iteration and get another signal (because the original was via exit, or because we hook multiple signals for the cleanup). So the active flag in theory is protecting us from that.

But it's not foolproof. We remove the tempfile and then unset the flag, so there's a moment (albeit small) where that other signal could come in. It shouldn't be a big deal because unlinking the tempfiles is mostly idempotent-ish; barring the unlikely chance that somebody else creates the same random filename as us, the second unlink will do nothing. So while removing the active flag would increase the size of that race (from handling a single entry to completing the whole atexit), that shouldn't matter much in practice.

But here's the interesting twist. Commit 2c2db194bd (tempfile: add mks_tempfile_dt(), 2022-04-20) recently taught remove_tempfiles() to drop a surrounding directory. And it does so by munging the filename buffer. It has to, because we can't allocate a new buffer in a signal handler.

But is it also idempotent(-ish)? The directory removal itself is, because it checks:

  tempfile->filename.buf[tempfile->directorylen] == '/'

before overwriting that byte with a NUL, so it should only be true once (though I note this violates the usual "volatile" rules for signal handlers, it's probably OK in practice since we know the NUL will be written before we call rmdir()).

But after that happens, the filename buffer now contains a C string that points to the directory. And if we end up in remove_tempfiles() again due to another signal, we'll try to unlink(p->filename.buf), which will now unlink the directory! I'm not sure how that behaves on all systems. On Linux it's a noop. And if it just deleted the directory, that would be OK. I seem to recall on old versions of SunOS it did bad things (maybe because it really did unlink it, but without checking for orphaned entries inside).

The good news is that it won't walk further up the tree. I was worried that a second round might then try to rmdir the parent (usually "/tmp" in this case). But the directory removal always uses the same length.

So...maybe it's all OK in practice? If not, this is an issue even without removing the active flag. But removing it would make the race window larger. I suspect it probably is OK in practice, but saying that about a race always feels like famous last words.

If we did want to keep it, I suspect we could get the same effect by munging the volatile "pid_t owner" field, but I think anything we munge is supposed to be a sig_atomic_t according to the letter of the law.

Patch below shows what it would look like to just drop "active" entirely. Test suite behaves as expected, but again, the real question is what it might be doing in a weird racy situation. The one above, but also a signal racing with adding an entry to the list. The volatile sig_atomic_t is what tells the removal process in the signal handler that it's OK to look at. But again, maybe that's OK in practice.

-Peff

---

Am 24.08.22 um 11:35 schrieb Jeff King:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html says of the unlink(2) parameter: "The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories." So we better not do that..

--- >8 ---
Subject: [PATCH] tempfile: avoid directory cleanup race

The temporary directory created by mks_tempfile_dt() is deleted by first deleting the file within, then truncating the filename strbuf and passing the resulting string to rmdir(2). When the cleanup routine is invoked concurrently by a signal handler we can end up passing the now truncated string to unlink(2), however, which could cause problems on some systems.

Avoid that issue by remembering the directory name separately. This way the paths stay unchanged. A signal handler can still race with normal cleanup, but deleting the same files and directories twice is harmless.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 tempfile.c | 14 ++++++--------
 tempfile.h |  2 +-
 2 files changed, 7 insertions(+), 9 deletions(-)

On Sat, Aug 27, 2022 at 12:46:29AM +0200, René Scharfe wrote:

Yeah, I saw that. It's a bit vague, and if the call returns ENOSYS or EISDIR, that would be perfectly fine. It's the "what happens on the implementations that do support it..." part that I'm more worried about. :)

That said...

Right, I'm a little embarrassed I didn't immediately come up with this same fix. This is definitely the right thing to do. The more we can treat data from signal-handlers are write-once, the better.

There's a slight extra memory cost to store the directory name twice, but it's a drop in the bucket overall.

The patch itself looks good to me.

-Peff

On Sat, Aug 27, 2022 at 6:05 AM Jeff King <peff@peff.net> wrote:

The history here is that pre-4.2BSD, Unix systems had no mkdir system call. You used mknod() to make a truly empty directory and the link() to create the "." and ".." entries within it, and all three of these operations were restricted to the super-user. There was no rmdir either, so again, unlink() as the super-user was permitted to do the job (with three calls to unlink the "." and ".." entries first and then remove the directory).

Unlinking a directory when it still contains "." leaves the link count at 1 and there's no GC, so it sits around occupying an inode.

If you have a mkdir() system call and don't need backwards compatibility, you get to have these return EISDIR errors...

Chris

On Sat, Aug 27, 2022 at 02:47:45PM -0700, Chris Torek wrote:

Thanks, that matches the sense of unease I had in the back of my mind. I seem to recall that maybe older versions of SunOS exhibited this, but that feels like a lifetime ago. At any rate, we should avoid that unlink() call, and René's patch neatly does so.

-Peff

On Sat, Aug 27, 2022 at 12:46:29AM +0200, René Scharfe wrote:

Thanks again for this patch. With it, I think it's reasonable to do the following cleanup on top.

  [1/2]: tempfile: drop active flag
  [2/2]: tempfile: update comment describing state transitions

 tempfile.c | 28 ++++++----------------------
 tempfile.h |  3 +--
 2 files changed, 7 insertions(+), 24 deletions(-)

-Peff

Our tempfile struct contains an "active" flag. Long ago, this flag was important: tempfile structs were always allocated for the lifetime of the program and added to a global linked list, and the active flag was what told us whether a struct's tempfile needed to be cleaned up on exit.

But since 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we actually remove items from the list, and the active flag is generally always set to true for any allocated struct. We set it to true in all of the creation functions, and in the normal code flow it becomes false only in deactivate_tempfile(), which then immediately frees the struct.

So the flag isn't performing that role anymore, and in fact makes things more confusing. Dscho noted that delete_tempfile() is a noop for an inactive struct. Since 076aa2cbda taught it to free the struct when deactivating, we'd leak any struct whose active flag is unset. But in practice it's not a leak, because again, we'll free when we unset the flag, and never see the allocated-but-inactive state.

Can we just get rid of the flag? The answer is yes, but it requires looking at a few other spots:

  1. I said above that the flag only becomes false before we deallocate,
     but there's one exception: when we call remove_tempfiles() from a
     signal or atexit handler, we unset the active flag as we remove
     each file. This isn't important for delete_tempfile(), as nobody
     would call it anymore, since we're exiting.

     It does in theory provide us some protection against racily
     double-removing a tempfile. If we receive a second signal while we
     are already in the cleanup routines, we'll start the cleanup loop
     again, and may visit the same tempfile. But this race already
     exists, because calling unlink() and unsetting the active flag
     aren't atomic! And it's OK in practice, because unlink() is
     idempotent (barring the unlikely event that some other process
     chooses our exact temp filename in that instant).

     So dropping the active flag widens the race a bit, but it was
     already there, and is fairly harmless in practice. If we really
     care about addressing it, the right thing is probably to block
     further signals while we're doing our cleanup (which we could
     actually do atomically).

  2. The active flag is declared as "volatile sig_atomic_t". The idea is
     that it's the final bit that gets set to tell the cleanup routines
     that the tempfile is ready to be used (or not used), and it's safe
     to receive a signal racing with regular code which adds or removes
     a tempfile from the list.

     In practice, I don't think this is buying us anything. The presence
     on the linked list is really what tells the cleanup routines to
     look at the struct. That is already marked as "volatile". It's not
     a sig_atomic_t, so it's possible that we could see a sheared write
     there as an entry is added or removed. But that is true of the
     current code, too! Before we can even look at the "active" flag,
     we'd have to follow a link to the struct itself. If we see a
     sheared write in the pointer to the struct, then we'll look at
     garbage memory anyway, and there's not much we can do.

This patch removes the active flag entirely, using presence on the global linked list as an indicator that a tempfile ought to be cleaned up. We are already careful to add to the list as the final step in activating. On deactivation, we'll make sure to remove from the list as the first step, before freeing any fields. The use of the volatile keyword should mean that those things happen in the expected order.

Signed-off-by: Jeff King <peff@peff.net>
---
I recognize this is a high-risk cleanup, as there's a lot of hand-waving
about volatile and sig_atomic_t. But I'm fairly convinced it's not
making anything materially worse. And while the number of dropped lines
is small, I hope it makes this tricky code easier to reason about.

 tempfile.c | 10 +---------
 tempfile.h |  3 +--
 2 files changed, 2 insertions(+), 11 deletions(-)

Back when 1a9d15db25 (tempfile: a new module for handling temporary files, 2015-08-10) added this comment, tempfile structs were held in memory for the life of a process, and there were various guarantees about which fields were valid in which states.

Since 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05) and 076aa2cbda (tempfile: auto-allocate tempfiles on heap, 2017-09-05), the flow is quite different: objects come and go from the list, and inactive ones are deallocated. And the previous commit removed the "active" flag from the struct entirely.

Let's bring the comment up to date with the current code.

Signed-off-by: Jeff King <peff@peff.net>
---
This could arguably come first in the series, minus the bit about the
"active" flag, since it is really putting the finishing touches on those
old commits from 2017. But it felt easier to reason about the end state
after dropping the flag entirely.

 tempfile.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)