[Libguestfs] [nbdkit PATCH v2 06/17] cow, cache: Better mkostemp fallback

In 2018, we added fallbacks for Haiku lacking mkostemp (one of the atomic CLOEXEC interfaces proposed for future POSIX [1]); however, that fallback has minor bugs: if mkstemp() fails, we blindly call fcntl(-1) (which probably changes the errno later reported against "mkostemp: %s: %m"); and although it is historically unlikely that any other bits will be set, F_SETFD should be used in a read-modify-write pattern rather than a blind overwrite pattern. [1] http://austingroupbugs.net/view.php?id=411 Update these fallbacks to use our newly-added set_cloexec utility function; neither place needs atomicity because they occur during .load (where we are still more-or-less single-threaded), rather than while another thread may be actively inside a plugin (such as sh) using fork(). Fixes: 60076fbcfd Fixes: b962272a56 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- common/utils/utils.c | 5 ++++- filters/cache/blk.c | 12 +++++++++++- filters/cow/blk.c | 12 +++++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/common/utils/utils.c b/common/utils/utils.c index 0548058c..7b63b4d4 100644 --- a/common/utils/utils.c +++ b/common/utils/utils.c @@ -140,12 +140,15 @@ exit_status_to_nbd_error (int status, const char *cmd) */ int set_cloexec (int fd) { -#ifdef SOCK_CLOEXEC +#if defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP nbdkit_error ("prefer creating fds with CLOEXEC atomically set"); close (fd); errno = EBADF; return -1; #else +# if defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP +# error "Unexpected: your system has incomplete atomic CLOEXEC support" +# endif int f; int err; diff --git a/filters/cache/blk.c b/filters/cache/blk.c index cf7145d8..272d176e 100644 --- a/filters/cache/blk.c +++ b/filters/cache/blk.c @@ -109,8 +109,18 @@ blk_init (void) #ifdef HAVE_MKOSTEMP fd = mkostemp (template, O_CLOEXEC); #else + /* Not atomic, but this is only invoked during .load, so the race + * won't affect any plugin actions trying to fork + */ fd = mkstemp (template); - fcntl (fd, F_SETFD, FD_CLOEXEC); + if (fd >= 0) { + fd = set_cloexec (fd); + if (fd < 0) { + int e = errno; + unlink (template); + errno = e; + } + } #endif if (fd == -1) { nbdkit_error ("mkostemp: %s: %m", tmpdir); diff --git a/filters/cow/blk.c b/filters/cow/blk.c index be43f2ff..2cae1122 100644 --- a/filters/cow/blk.c +++ b/filters/cow/blk.c @@ -114,8 +114,18 @@ blk_init (void) #ifdef HAVE_MKOSTEMP fd = mkostemp (template, O_CLOEXEC); #else + /* Not atomic, but this is only invoked during .load, so the race + * won't affect any plugin actions trying to fork + */ fd = mkstemp (template); - fcntl (fd, F_SETFD, FD_CLOEXEC); + if (fd >= 0) { + fd = set_cloexec (fd); + if (fd < 0) { + int e = errno; + unlink (template); + errno = e; + } + } #endif if (fd == -1) { nbdkit_error ("mkostemp: %s: %m", tmpdir); -- 2.20.1