Am 24.08.22 um 11:35 schrieb Jeff King: > 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). 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@xxxxxxxx> Signed-off-by: René Scharfe <l.s.r@xxxxxx> --- tempfile.c | 14 ++++++-------- tempfile.h | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/tempfile.c b/tempfile.c index 2024c82691..7414c81e31 100644 --- a/tempfile.c +++ b/tempfile.c @@ -59,14 +59,11 @@ static VOLATILE_LIST_HEAD(tempfile_list); static void remove_template_directory(struct tempfile *tempfile, int in_signal_handler) { - if (tempfile->directorylen > 0 && - tempfile->directorylen < tempfile->filename.len && - tempfile->filename.buf[tempfile->directorylen] == '/') { - strbuf_setlen(&tempfile->filename, tempfile->directorylen); + if (tempfile->directory) { if (in_signal_handler) - rmdir(tempfile->filename.buf); + rmdir(tempfile->directory); else - rmdir_or_warn(tempfile->filename.buf); + rmdir_or_warn(tempfile->directory); } } @@ -115,7 +112,7 @@ static struct tempfile *new_tempfile(void) tempfile->owner = 0; INIT_LIST_HEAD(&tempfile->list); strbuf_init(&tempfile->filename, 0); - tempfile->directorylen = 0; + tempfile->directory = NULL; return tempfile; } @@ -141,6 +138,7 @@ static void deactivate_tempfile(struct tempfile *tempfile) { tempfile->active = 0; strbuf_release(&tempfile->filename); + free(tempfile->directory); volatile_list_del(&tempfile->list); free(tempfile); } @@ -254,7 +252,7 @@ struct tempfile *mks_tempfile_dt(const char *directory_template, tempfile = new_tempfile(); strbuf_swap(&tempfile->filename, &sb); - tempfile->directorylen = directorylen; + tempfile->directory = xmemdupz(tempfile->filename.buf, directorylen); tempfile->fd = fd; activate_tempfile(tempfile); return tempfile; diff --git a/tempfile.h b/tempfile.h index d7804a214a..5b9e8743dd 100644 --- a/tempfile.h +++ b/tempfile.h @@ -82,7 +82,7 @@ struct tempfile { FILE *volatile fp; volatile pid_t owner; struct strbuf filename; - size_t directorylen; + char *directory; }; /* -- 2.37.2
