Merge pull request #6280 from thufschmitt/fix-mv-in-different-filesystems

Fix mv in different filesystems

11 files changed, 196 insertions, 122 deletions

diff --git a/configure.ac b/configure.ac
index f0210ab78..64fa12fc7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -296,15 +296,6 @@ AC_CHECK_FUNCS([setresuid setreuid lchown])
 AC_CHECK_FUNCS([strsignal posix_fallocate sysconf])
 
 
-# This is needed if bzip2 is a static library, and the Nix libraries
-# are dynamic.
-case "${host_os}" in
-  darwin*)
-    LDFLAGS="-all_load $LDFLAGS"
-    ;;
-esac
-
-
 AC_ARG_WITH(sandbox-shell, AS_HELP_STRING([--with-sandbox-shell=PATH],[path of a statically-linked shell to use as /bin/sh in sandboxes]),
   sandbox_shell=$withval)
 AC_SUBST(sandbox_shell)
diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index 3fff2385f..459fdae79 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -705,8 +705,7 @@ static void movePath(const Path & src, const Path & dst)
     if (changePerm)
         chmod_(src, st.st_mode | S_IWUSR);
 
-    if (rename(src.c_str(), dst.c_str()))
-        throw SysError("renaming '%1%' to '%2%'", src, dst);
+    renameFile(src, dst);
 
     if (changePerm)
         chmod_(dst, st.st_mode);
diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 79a241ae0..6843173a7 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -223,8 +223,7 @@ static void movePath(const Path & src, const Path & dst)
     if (changePerm)
         chmod_(src, st.st_mode | S_IWUSR);
 
-    if (rename(src.c_str(), dst.c_str()))
-        throw SysError("renaming '%1%' to '%2%'", src, dst);
+    renameFile(src, dst);
 
     if (changePerm)
         chmod_(dst, st.st_mode);
@@ -311,7 +310,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
             if (buildMode != bmCheck && status.known->isValid()) continue;
             auto p = worker.store.printStorePath(status.known->path);
             if (pathExists(chrootRootDir + p))
-                rename((chrootRootDir + p).c_str(), p.c_str());
+                renameFile((chrootRootDir + p), p);
         }
 
     return diskFull;
@@ -2625,8 +2624,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
             Path prev = path + checkSuffix;
             deletePath(prev);
             Path dst = path + checkSuffix;
-            if (rename(path.c_str(), dst.c_str()))
-                throw SysError("renaming '%s' to '%s'", path, dst);
+            renameFile(path, dst);
         }
     }
 
diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc
index 426d58a53..ba04bb16c 100644
--- a/src/libstore/builtins/unpack-channel.cc
+++ b/src/libstore/builtins/unpack-channel.cc
@@ -22,8 +22,7 @@ void builtinUnpackChannel(const BasicDerivation & drv)
     auto entries = readDirectory(out);
     if (entries.size() != 1)
         throw Error("channel tarball '%s' contains more than one file", src);
-    if (rename((out + "/" + entries[0].name).c_str(), (out + "/" + channelName).c_str()) == -1)
-        throw SysError("renaming channel directory");
+    renameFile((out + "/" + entries[0].name), (out + "/" + channelName));
 }
 
 }
diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index d58ed78b1..4c1a82279 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -39,9 +39,7 @@ static void makeSymlink(const Path & link, const Path & target)
     createSymlink(target, tempLink);
 
     /* Atomically replace the old one. */
-    if (rename(tempLink.c_str(), link.c_str()) == -1)
-        throw SysError("cannot rename '%1%' to '%2%'",
-            tempLink , link);
+    renameFile(tempLink, link);
 }
 
 
diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc
index ba4416f6d..f20b1fa02 100644
--- a/src/libstore/local-binary-cache-store.cc
+++ b/src/libstore/local-binary-cache-store.cc
@@ -57,8 +57,7 @@ protected:
         AutoDelete del(tmp, false);
         StreamToSourceAdapter source(istream);
         writeFile(tmp, source);
-        if (rename(tmp.c_str(), path2.c_str()))
-            throw SysError("renaming '%1%' to '%2%'", tmp, path2);
+        renameFile(tmp, path2);
         del.cancel();
     }
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index eba3b0fa5..a272e4301 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -1430,8 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name
                     writeFile(realPath, dumpSource);
             } else {
                 /* Move the temporary path we restored above. */
-                if (rename(tempPath.c_str(), realPath.c_str()))
-                    throw Error("renaming '%s' to '%s'", tempPath, realPath);
+                moveFile(tempPath, realPath);
             }
 
             /* For computing the nar hash. In recursive SHA-256 mode, this
@@ -1942,8 +1941,7 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log)
 
     writeFile(tmpFile, compress("bzip2", log));
 
-    if (rename(tmpFile.c_str(), logPath.c_str()) != 0)
-        throw SysError("renaming '%1%' to '%2%'", tmpFile, logPath);
+    renameFile(tmpFile, logPath);
 }
 
 std::optional<std::string> LocalStore::getVersion()
diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc
index 8af9b1dde..4d2781180 100644
--- a/src/libstore/optimise-store.cc
+++ b/src/libstore/optimise-store.cc
@@ -229,7 +229,9 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,
     }
 
     /* Atomically replace the old file with the new hard link. */
-    if (rename(tempLink.c_str(), path.c_str()) == -1) {
+    try {
+        renameFile(tempLink, path);
+    } catch (SysError & e) {
         if (unlink(tempLink.c_str()) == -1)
             printError("unable to unlink '%1%'", tempLink);
         if (errno == EMLINK) {
@@ -240,7 +242,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,
             debug("'%s' has reached maximum number of links", linkPath);
             return;
         }
-        throw SysError("cannot rename '%1%' to '%2%'", tempLink, path);
+        throw;
     }
 
     stats.filesLinked++;
diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc
new file mode 100644
index 000000000..403389e60
--- /dev/null
+++ b/src/libutil/filesystem.cc
@@ -0,0 +1,172 @@
+#include <sys/time.h>
+#include <filesystem>
+
+#include "finally.hh"
+#include "util.hh"
+#include "types.hh"
+
+namespace fs = std::filesystem;
+
+namespace nix {
+
+static Path tempName(Path tmpRoot, const Path & prefix, bool includePid,
+    int & counter)
+{
+    tmpRoot = canonPath(tmpRoot.empty() ? getEnv("TMPDIR").value_or("/tmp") : tmpRoot, true);
+    if (includePid)
+        return (format("%1%/%2%-%3%-%4%") % tmpRoot % prefix % getpid() % counter++).str();
+    else
+        return (format("%1%/%2%-%3%") % tmpRoot % prefix % counter++).str();
+}
+
+Path createTempDir(const Path & tmpRoot, const Path & prefix,
+    bool includePid, bool useGlobalCounter, mode_t mode)
+{
+    static int globalCounter = 0;
+    int localCounter = 0;
+    int & counter(useGlobalCounter ? globalCounter : localCounter);
+
+    while (1) {
+        checkInterrupt();
+        Path tmpDir = tempName(tmpRoot, prefix, includePid, counter);
+        if (mkdir(tmpDir.c_str(), mode) == 0) {
+#if __FreeBSD__
+            /* Explicitly set the group of the directory.  This is to
+               work around around problems caused by BSD's group
+               ownership semantics (directories inherit the group of
+               the parent).  For instance, the group of /tmp on
+               FreeBSD is "wheel", so all directories created in /tmp
+               will be owned by "wheel"; but if the user is not in
+               "wheel", then "tar" will fail to unpack archives that
+               have the setgid bit set on directories. */
+            if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0)
+                throw SysError("setting group of directory '%1%'", tmpDir);
+#endif
+            return tmpDir;
+        }
+        if (errno != EEXIST)
+            throw SysError("creating directory '%1%'", tmpDir);
+    }
+}
+
+
+std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix)
+{
+    Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX");
+    // Strictly speaking, this is UB, but who cares...
+    // FIXME: use O_TMPFILE.
+    AutoCloseFD fd(mkstemp((char *) tmpl.c_str()));
+    if (!fd)
+        throw SysError("creating temporary file '%s'", tmpl);
+    closeOnExec(fd.get());
+    return {std::move(fd), tmpl};
+}
+
+void createSymlink(const Path & target, const Path & link,
+    std::optional<time_t> mtime)
+{
+    if (symlink(target.c_str(), link.c_str()))
+        throw SysError("creating symlink from '%1%' to '%2%'", link, target);
+    if (mtime) {
+        struct timeval times[2];
+        times[0].tv_sec = *mtime;
+        times[0].tv_usec = 0;
+        times[1].tv_sec = *mtime;
+        times[1].tv_usec = 0;
+        if (lutimes(link.c_str(), times))
+            throw SysError("setting time of symlink '%s'", link);
+    }
+}
+
+void replaceSymlink(const Path & target, const Path & link,
+    std::optional<time_t> mtime)
+{
+    for (unsigned int n = 0; true; n++) {
+        Path tmp = canonPath(fmt("%s/.%d_%s", dirOf(link), n, baseNameOf(link)));
+
+        try {
+            createSymlink(target, tmp, mtime);
+        } catch (SysError & e) {
+            if (e.errNo == EEXIST) continue;
+            throw;
+        }
+
+        renameFile(tmp, link);
+
+        break;
+    }
+}
+
+void setWriteTime(const fs::path & p, const struct stat & st)
+{
+    struct timeval times[2];
+    times[0] = {
+        .tv_sec = st.st_atime,
+        .tv_usec = 0,
+    };
+    times[1] = {
+        .tv_sec = st.st_mtime,
+        .tv_usec = 0,
+    };
+    if (lutimes(p.c_str(), times) != 0)
+        throw SysError("changing modification time of '%s'", p);
+}
+
+void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete)
+{
+    // TODO: Rewrite the `is_*` to use `symlink_status()`
+    auto statOfFrom = lstat(from.path().c_str());
+    auto fromStatus = from.symlink_status();
+
+    // Mark the directory as writable so that we can delete its children
+    if (andDelete && fs::is_directory(fromStatus)) {
+        fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow);
+    }
+
+
+    if (fs::is_symlink(fromStatus) || fs::is_regular_file(fromStatus)) {
+        fs::copy(from.path(), to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing);
+    } else if (fs::is_directory(fromStatus)) {
+        fs::create_directory(to);
+        for (auto & entry : fs::directory_iterator(from.path())) {
+            copy(entry, to / entry.path().filename(), andDelete);
+        }
+    } else {
+        throw Error("file '%s' has an unsupported type", from.path());
+    }
+
+    setWriteTime(to, statOfFrom);
+    if (andDelete) {
+        if (!fs::is_symlink(fromStatus))
+            fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow);
+        fs::remove(from.path());
+    }
+}
+
+void renameFile(const Path & oldName, const Path & newName)
+{
+    fs::rename(oldName, newName);
+}
+
+void moveFile(const Path & oldName, const Path & newName)
+{
+    try {
+        renameFile(oldName, newName);
+    } catch (fs::filesystem_error & e) {
+        auto oldPath = fs::path(oldName);
+        auto newPath = fs::path(newName);
+        // For the move to be as atomic as possible, copy to a temporary
+        // directory
+        fs::path temp = createTempDir(newPath.parent_path(), "rename-tmp");
+        Finally removeTemp = [&]() { fs::remove(temp); };
+        auto tempCopyTarget = temp / "copy-target";
+        if (e.code().value() == EXDEV) {
+            fs::remove(newPath);
+            warn("Can’t rename %s as %s, copying instead", oldName, newName);
+            copy(fs::directory_entry(oldPath), tempCopyTarget, true);
+            renameFile(tempCopyTarget, newPath);
+        }
+    }
+}
+
+}
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index e11cb9c60..96ac11ea2 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -508,61 +508,6 @@ void deletePath(const Path & path, uint64_t & bytesFreed)
 }
 
 
-static Path tempName(Path tmpRoot, const Path & prefix, bool includePid,
-    int & counter)
-{
-    tmpRoot = canonPath(tmpRoot.empty() ? getEnv("TMPDIR").value_or("/tmp") : tmpRoot, true);
-    if (includePid)
-        return (format("%1%/%2%-%3%-%4%") % tmpRoot % prefix % getpid() % counter++).str();
-    else
-        return (format("%1%/%2%-%3%") % tmpRoot % prefix % counter++).str();
-}
-
-
-Path createTempDir(const Path & tmpRoot, const Path & prefix,
-    bool includePid, bool useGlobalCounter, mode_t mode)
-{
-    static int globalCounter = 0;
-    int localCounter = 0;
-    int & counter(useGlobalCounter ? globalCounter : localCounter);
-
-    while (1) {
-        checkInterrupt();
-        Path tmpDir = tempName(tmpRoot, prefix, includePid, counter);
-        if (mkdir(tmpDir.c_str(), mode) == 0) {
-#if __FreeBSD__
-            /* Explicitly set the group of the directory.  This is to
-               work around around problems caused by BSD's group
-               ownership semantics (directories inherit the group of
-               the parent).  For instance, the group of /tmp on
-               FreeBSD is "wheel", so all directories created in /tmp
-               will be owned by "wheel"; but if the user is not in
-               "wheel", then "tar" will fail to unpack archives that
-               have the setgid bit set on directories. */
-            if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0)
-                throw SysError("setting group of directory '%1%'", tmpDir);
-#endif
-            return tmpDir;
-        }
-        if (errno != EEXIST)
-            throw SysError("creating directory '%1%'", tmpDir);
-    }
-}
-
-
-std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix)
-{
-    Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX");
-    // Strictly speaking, this is UB, but who cares...
-    // FIXME: use O_TMPFILE.
-    AutoCloseFD fd(mkstemp((char *) tmpl.c_str()));
-    if (!fd)
-        throw SysError("creating temporary file '%s'", tmpl);
-    closeOnExec(fd.get());
-    return {std::move(fd), tmpl};
-}
-
-
 std::string getUserName()
 {
     auto pw = getpwuid(geteuid());
@@ -684,44 +629,6 @@ Paths createDirs(const Path & path)
 }
 
 
-void createSymlink(const Path & target, const Path & link,
-    std::optional<time_t> mtime)
-{
-    if (symlink(target.c_str(), link.c_str()))
-        throw SysError("creating symlink from '%1%' to '%2%'", link, target);
-    if (mtime) {
-        struct timeval times[2];
-        times[0].tv_sec = *mtime;
-        times[0].tv_usec = 0;
-        times[1].tv_sec = *mtime;
-        times[1].tv_usec = 0;
-        if (lutimes(link.c_str(), times))
-            throw SysError("setting time of symlink '%s'", link);
-    }
-}
-
-
-void replaceSymlink(const Path & target, const Path & link,
-    std::optional<time_t> mtime)
-{
-    for (unsigned int n = 0; true; n++) {
-        Path tmp = canonPath(fmt("%s/.%d_%s", dirOf(link), n, baseNameOf(link)));
-
-        try {
-            createSymlink(target, tmp, mtime);
-        } catch (SysError & e) {
-            if (e.errNo == EEXIST) continue;
-            throw;
-        }
-
-        if (rename(tmp.c_str(), link.c_str()) != 0)
-            throw SysError("renaming '%1%' to '%2%'", tmp, link);
-
-        break;
-    }
-}
-
-
 void readFull(int fd, char * buf, size_t count)
 {
     while (count) {
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 29227ecc6..cd83f250f 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -168,6 +168,17 @@ void createSymlink(const Path & target, const Path & link,
 void replaceSymlink(const Path & target, const Path & link,
     std::optional<time_t> mtime = {});
 
+void renameFile(const Path & src, const Path & dst);
+
+/**
+ * Similar to 'renameFile', but fallback to a copy+remove if `src` and `dst`
+ * are on a different filesystem.
+ *
+ * Beware that this might not be atomic because of the copy that happens behind
+ * the scenes
+ */
+void moveFile(const Path & src, const Path & dst);
+
 
 /* Wrappers arount read()/write() that read/write exactly the
    requested number of bytes. */