Share the exception-using chmod wrapper with more code

src/libstore/builtins/fetchurl.cc

@@ -4,6 +4,7 @@
 #include "nix/store/globals.hh"
 #include "nix/util/archive.hh"
 #include "nix/util/compression.hh"
+#include "nix/util/file-system.hh"
 
 namespace nix {
 
@@ -65,8 +66,7 @@ static void builtinFetchurl(const BuiltinBuilderContext & ctx)
 
         auto executable = ctx.drv.env.find("executable");
         if (executable != ctx.drv.env.end() && executable->second == "1") {
-            if (chmod(storePath.c_str(), 0755) == -1)
-                throw SysError("making '%1%' executable", storePath);
+            chmod(storePath, 0755);
         }
     };
 

src/libstore/local-store.cc

@@ -174,8 +174,7 @@ LocalStore::LocalStore(ref<const Config> config)
             if (st.st_uid != 0 || st.st_gid != gr->gr_gid || (st.st_mode & ~S_IFMT) != perm) {
                 if (chown(config->realStoreDir.get().c_str(), 0, gr->gr_gid) == -1)
                     throw SysError("changing ownership of path '%1%'", config->realStoreDir);
-                if (chmod(config->realStoreDir.get().c_str(), perm) == -1)
-                    throw SysError("changing permissions on path '%1%'", config->realStoreDir);
+                chmod(config->realStoreDir.get(), perm);
             }
         }
     }

src/libstore/optimise-store.cc

@@ -3,6 +3,7 @@
 #include "nix/util/signals.hh"
 #include "nix/store/posix-fs-canonicalise.hh"
 #include "nix/util/posix-source-accessor.hh"
+#include "nix/util/file-system.hh"
 
 #include <cstdlib>
 #include <cstring>
@@ -20,8 +21,7 @@ namespace nix {
 static void makeWritable(const Path & path)
 {
     auto st = lstat(path);
-    if (chmod(path.c_str(), st.st_mode | S_IWUSR) == -1)
-        throw SysError("changing writability of '%1%'", path);
+    chmod(path, st.st_mode | S_IWUSR);
 }
 
 struct MakeReadOnly

src/libstore/posix-fs-canonicalise.cc

@@ -24,8 +24,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct
         bool isDir = S_ISDIR(st.st_mode);
         if ((mode != 0444 || isDir) && mode != 0555) {
             mode = (st.st_mode & S_IFMT) | 0444 | (st.st_mode & S_IXUSR || isDir ? 0111 : 0);
-            if (chmod(path.c_str(), mode) == -1)
-                throw SysError("changing mode of '%1%' to %2$o", path, mode);
+            chmod(path, mode);
         }
     }
 

src/libstore/unix/build/chroot-derivation-builder.cc

@@ -85,7 +85,7 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
            instead.) */
         std::filesystem::path chrootTmpDir = chrootRootDir / "tmp";
         createDirs(chrootTmpDir);
-        chmod_(chrootTmpDir, 01777);
+        chmod(chrootTmpDir, 01777);
 
         /* Create a /etc/passwd with entries for the build user and the
            nobody account.  The latter is kind of a hack to support
@@ -119,7 +119,7 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
            build user. */
         std::filesystem::path chrootStoreDir = chrootRootDir / std::filesystem::path(store.storeDir).relative_path();
         createDirs(chrootStoreDir);
-        chmod_(chrootStoreDir, 01775);
+        chmod(chrootStoreDir, 01775);
 
         if (buildUser && chown(chrootStoreDir.c_str(), 0, buildUser->getGID()) == -1)
             throw SysError("cannot change ownership of %1%", PathFmt(chrootStoreDir));

src/libstore/unix/build/derivation-builder.cc

@@ -570,12 +570,6 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
     return builtOutputs;
 }
 
-static void chmod_(const std::filesystem::path & path, mode_t mode)
-{
-    if (chmod(path.c_str(), mode) == -1)
-        throw SysError("setting permissions on %s", PathFmt(path));
-}
-
 /* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if
    it's a directory and we're not root (to be able to update the
    directory's parent link ".."). */
@@ -586,12 +580,12 @@ static void movePath(const std::filesystem::path & src, const std::filesystem::p
     bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR));
 
     if (changePerm)
-        chmod_(src, st.st_mode | S_IWUSR);
+        chmod(src, st.st_mode | S_IWUSR);
 
     std::filesystem::rename(src, dst);
 
     if (changePerm)
-        chmod_(dst, st.st_mode);
+        chmod(dst, st.st_mode);
 }
 
 static void replaceValidPath(const std::filesystem::path & storePath, const std::filesystem::path & tmpPath)
@@ -824,8 +818,7 @@ std::optional<Descriptor> DerivationBuilderImpl::startBuild()
     std::string slaveName = getPtsName(builderOut.get());
 
     if (buildUser) {
-        if (chmod(slaveName.c_str(), 0600))
-            throw SysError("changing mode of pseudoterminal slave");
+        chmod(slaveName, 0600);
 
         if (chown(slaveName.c_str(), buildUser->getUID(), 0))
             throw SysError("changing owner of pseudoterminal slave");
@@ -1926,14 +1919,14 @@ void DerivationBuilderImpl::cleanupBuild(bool force)
          * This hardens against an attack which smuggles a file descriptor
          * to make use of the temporary directory.
          */
-        chmod(topTmpDir.c_str(), 0000);
+        chmod(topTmpDir, 0000);
 
         /* Don't keep temporary directories for builtins because they
            might have privileged stuff (like a copy of netrc). */
         if (settings.keepFailed && !force && !drv.isBuiltin()) {
             printError("note: keeping build directory %s", PathFmt(tmpDir));
-            chmod(topTmpDir.c_str(), 0755);
-            chmod(tmpDir.c_str(), 0755);
+            chmod(topTmpDir, 0755);
+            chmod(tmpDir, 0755);
         } else
             deletePath(topTmpDir);
         topTmpDir = "";

src/libstore/unix/build/linux-derivation-builder.cc

@@ -586,7 +586,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
                 auto dst = chrootRootDir / i.first.relative_path();
                 createDirs(dst.parent_path());
                 writeFile(dst, std::string_view((const char *) sh, sizeof(sh)));
-                chmod_(dst, 0555);
+                chmod(dst, 0555);
             } else
 #  endif
             {
@@ -629,7 +629,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
 
                 /* Make sure /dev/pts/ptmx is world-writable.  With some
                    Linux versions, it is created with permissions 0.  */
-                chmod_(chrootRootDir / "dev" / "pts" / "ptmx", 0666);
+                chmod(chrootRootDir / "dev" / "pts" / "ptmx", 0666);
             } else {
                 if (errno != EINVAL)
                     throw SysError("mounting /dev/pts");
@@ -640,7 +640,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
 
         /* Make /etc unwritable */
         if (!drvOptions.useUidRange(drv))
-            chmod_(chrootRootDir / "etc", 0555);
+            chmod(chrootRootDir / "etc", 0555);
 
         /* Unshare this mount namespace. This is necessary because
            pivot_root() below changes the root of the mount

src/libutil-tests/unix/file-descriptor.cc

@@ -97,8 +97,8 @@ TEST(fchmodatTryNoFollow, works)
         sink.createSymlink(CanonPath("dirlink"), "dir");
     }
 
-    ASSERT_EQ(chmod((tmpDir / "file").c_str(), 0644), 0);
-    ASSERT_EQ(chmod((tmpDir / "dir").c_str(), 0755), 0);
+    ASSERT_NO_THROW(chmod(tmpDir / "file", 0644));
+    ASSERT_NO_THROW(chmod(tmpDir / "dir", 0755));
 
     AutoCloseFD dirFd = openDirectory(tmpDir);
     ASSERT_TRUE(dirFd);
@@ -152,7 +152,7 @@ TEST(fchmodatTryNoFollow, fallbackWithoutProc)
         sink.createSymlink(CanonPath("link"), "file");
     }
 
-    ASSERT_EQ(chmod((tmpDir / "file").c_str(), 0644), 0);
+    ASSERT_NO_THROW(chmod(tmpDir / "file", 0644));
 
     Pid pid = startProcess(
         [&] {

src/libutil/file-system.cc

@@ -739,6 +739,19 @@ std::filesystem::path makeParentCanonical(const std::filesystem::path & rawPath)
     }
 }
 
+void chmod(const std::filesystem::path & path, mode_t mode)
+{
+    if (
+#ifdef _WIN32
+        ::_wchmod
+#else
+        ::chmod
+#endif
+        (path.c_str(), mode)
+        == -1)
+        throw SysError("setting permissions on %s", PathFmt(path));
+}
+
 bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask)
 {
     auto pathString = path.string();
@@ -747,9 +760,7 @@ bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask)
     if (((prevMode ^ mode) & mask) == 0)
         return false;
 
-    if (chmod(pathString.c_str(), mode) != 0)
-        throw SysError("could not set permissions on '%s' to %o", pathString, mode);
-
+    chmod(path, mode);
     return true;
 }
 

src/libutil/include/nix/util/file-system.hh

@@ -418,6 +418,17 @@ extern PathFilter defaultPathFilter;
  */
 bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask = S_IRWXU | S_IRWXG | S_IRWXO);
 
+/**
+ * Set permissions on a path, throwing an exception on error.
+ *
+ * @param path Path to the file to change the permissions for.
+ * @param mode New file mode.
+ *
+ * @todo stop using this and start using `fchmodatTryNoFollow` (or a different
+ * wrapper) to avoid TOCTOU issues.
+ */
+void chmod(const std::filesystem::path & path, mode_t mode);
+
 /**
  * @brief A directory iterator that can be used to iterate over the
  * contents of a directory. It is similar to std::filesystem::directory_iterator

src/libutil/unix-domain-socket.cc

@@ -38,8 +38,7 @@ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode)
 
     bind(fdSocket.get(), path);
 
-    if (chmod(path.c_str(), mode) == -1)
-        throw SysError("changing permissions on '%1%'", path);
+    chmod(path, mode);
 
     if (listen(toSocket(fdSocket.get()), 100) == -1)
         throw SysError("cannot listen on socket '%1%'", path);