Re: curiosities with tempfile.active

[Jeff King <peff@peff.net>]

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

> 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..

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...

> --- >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.

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.

>  tempfile.c | 14 ++++++--------
>  tempfile.h |  2 +-

The patch itself looks good to me.

-Peff