clean up

Signed-off-by: Samuli Thomasson <samuli.thomasson@pm.me>

src/libstore/globals.cc

@@ -86,7 +86,7 @@ Settings::Settings()
     }
 
 #if (defined(__linux__) || defined(__FreeBSD__)) && defined(SANDBOX_SHELL)
-    sandboxPaths = SandboxPaths { { "/bin/sh", SandboxPath(SANDBOX_SHELL) } };
+    sandboxPaths = { { "/bin/sh", SANDBOX_SHELL } };
 #endif
 
     /* chroot-like behavior from Apple's sandbox */
@@ -297,25 +297,26 @@ template<> void BaseSetting<SandboxMode>::convertToArg(Args & args, const std::s
 }
 
 NLOHMANN_JSON_SERIALIZE_ENUM(SandboxPath::MountOpt, {
-    {SandboxPath::MountOpt::ro,          "ro"},
+    {SandboxPath::MountOpt::ro, "ro"},
 #ifdef __linux__
-    {SandboxPath::MountOpt::nodev,       "nodev"},
-    {SandboxPath::MountOpt::noexec,      "noexec"},
-    {SandboxPath::MountOpt::nosuid,      "nosuid"},
-    {SandboxPath::MountOpt::noatime,     "noatime"},
-    {SandboxPath::MountOpt::nodiratime,  "nodiratime"},
-    {SandboxPath::MountOpt::relatime,    "relatime"},
+    {SandboxPath::MountOpt::nodev, "nodev"},
+    {SandboxPath::MountOpt::noexec, "noexec"},
+    {SandboxPath::MountOpt::nosuid, "nosuid"},
+    {SandboxPath::MountOpt::noatime, "noatime"},
+    {SandboxPath::MountOpt::nodiratime, "nodiratime"},
+    {SandboxPath::MountOpt::relatime, "relatime"},
     {SandboxPath::MountOpt::strictatime, "strictatime"},
-    {SandboxPath::MountOpt::private_,    "private"},
-    {SandboxPath::MountOpt::slave,       "slave"},
-    {SandboxPath::MountOpt::unbindable,  "unbindable"},
+    {SandboxPath::MountOpt::private_, "private"},
+    {SandboxPath::MountOpt::slave, "slave"},
+    {SandboxPath::MountOpt::unbindable, "unbindable"},
 #endif
 });
 
 NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT(SandboxPath, source, optional, readOnly, options);
 
 /**
- * Parses either old (strings) or new (json object) format sandbox-paths.
+ * Parses either old ("path[=source][?]" strings) or new (json object) format
+ * sandbox-paths. Legacy format supports only a subset of available settings.
  */
 SandboxPaths SandboxPath::parse(const std::string_view & str, const std::string & ctx)
 {
@@ -334,15 +335,14 @@ SandboxPaths SandboxPath::parse(const std::string_view & str, const std::string
         for (auto & [k, v] : nlohmann::json::parse(str, nullptr, false, true).template get<SandboxPaths>())
             add(k, std::move(v));
     } else {
-        /* Parses legacy format sandbox-path e.g. "path[=source][?]".
-         * This format supports only a subset of options available with JSON format. */
         for (std::string_view s : tokenizeString<Strings>(str)) {
             bool optional = s.ends_with('?');
             if (optional) s.remove_suffix(1);
             if (size_t eq = s.find('='); eq != s.npos) {
                 add(std::string(s, 0, eq), { std::string(s.data() + eq + 1, s.size() - eq - 1), optional });
-            } else
+            } else {
                 add(std::string(s), { "", optional });
+            }
         }
     }
     return res;
@@ -350,7 +350,7 @@ SandboxPaths SandboxPath::parse(const std::string_view & str, const std::string
 
 template<> SandboxPaths BaseSetting<SandboxPaths>::parse(const std::string & str) const
 {
-    return SandboxPath().parse(str, this->name);
+    return SandboxPath::parse(str, this->name);
 }
 
 template<> struct BaseSetting<SandboxPaths>::trait

src/libstore/include/nix/store/globals.hh

@@ -25,6 +25,7 @@ struct SandboxPath;
 using SandboxPaths = std::map<Path, SandboxPath, std::less<>>;
 struct SandboxPath
 {
+public:
     typedef enum {
 #ifdef __linux__
         ro = MS_RDONLY,
@@ -39,14 +40,19 @@ struct SandboxPath
         slave = MS_SLAVE,
         unbindable = MS_UNBINDABLE
 #else
-        ro // FIXME: do any options make sense on other that linux?
+        ro  // FIXME: do any options make sense on other that linux?
 #endif
     } MountOpt;
 
 #ifdef __linux__
     /* Options to set when readOnly=true */
-    static constexpr std::array<MountOpt, 5> readOnlyDefaults = {
-        ro, nodev, noexec, nosuid, noatime
+    constexpr static MountOpt readOnlyDefaults[] = { ro, nodev, noexec, nosuid, noatime };
+
+    /* Only one atime option should be enabled at a time. Same for propagation
+     * style.*/
+    constexpr static std::pair<uint64_t, const char*> exclusiveOptionMasks[] = {
+        {MS_NOATIME | MS_NODIRATIME | MS_RELATIME | MS_STRICTATIME, "option-atime"},
+        {MS_SHARED | MS_PRIVATE | MS_SLAVE, "propagation"},
     };
 #endif
 
@@ -65,10 +71,18 @@ struct SandboxPath
 
     std::vector<MountOpt> options;
 
-    SandboxPath(const Path & source = "",
-        bool optional = false, bool readOnly = false, const std::vector<MountOpt> & options = { })
-        : source(source), optional(optional), readOnly(readOnly), options(options) { };
-    SandboxPath(const char * source) : SandboxPath(Path(source)) { };
+    SandboxPath(std::string source = "", bool optional = false,
+        bool readOnly = false, std::vector<MountOpt> options = { }) :
+        source(std::string(std::move(source))), optional(optional),
+        readOnly(readOnly), options(std::move(options)) { }
+
+    /* This is to enable the full implicit conversion from e.g. const char[],
+     * even when binding a reference. Code can specify paths with literals and
+     * nothing extra. (Have angried the C++ deities with this? Seems like
+     * there should be a better way?) */
+    template<typename S, typename = std::enable_if_t<std::is_convertible_v<S, Path>>>
+    SandboxPath(S&& source, bool optional = false, bool readOnly = false) :
+        SandboxPath(Path(std::forward<S>(source)), optional, readOnly) { }
 
     static SandboxPaths parse(const std::string_view & str, const std::string& = "(unknown)");
 };

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

@@ -106,7 +106,7 @@ protected:
      * Stuff we need to pass to initChild().
      */
 
-    typedef SandboxPaths PathsInChroot; // maps target path to source path
+    typedef SandboxPaths PathsInChroot;
 
     typedef StringMap Environment;
     Environment env;
@@ -920,7 +920,7 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
                 if (line == "") {
                     state = stBegin;
                 } else {
-                    for (const auto & [k, v] : SandboxPath().parse(line))
+                    for (const auto & [k, v] : SandboxPath::parse(line, "extra-sandbox-paths (via pre-build-hook)"))
                         pathsInChroot.try_emplace(k, v);
                 }
             }

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

@@ -121,32 +121,30 @@ static void setupSeccomp()
 #  endif
 }
 
-static auto combineMountOpts(auto init, auto iter)
-{
-    return std::transform_reduce(iter.cbegin(), iter.cend(), init,
-        [](unsigned long a, unsigned long b) {
-            if (b & (MS_NOATIME | MS_RELATIME | MS_NODIRATIME | MS_STRICTATIME)) {
-                return (a & ~(MS_NOATIME | MS_NODIRATIME | MS_RELATIME | MS_STRICTATIME)) | b;
-            }
-            return a | b;
-        },
-        [](const SandboxPath::MountOpt & o) { return static_cast<unsigned long>(o); });
+static uint64_t mergeIntoMountOpts(uint64_t oset, uint64_t o) {
+    for (const auto & [mask, _] : SandboxPath::exclusiveOptionMasks)
+        if (o & mask) return (oset & ~mask) | o;
+    return oset | o;
 };
 
-
-static void doBind(const SandboxPath & pval, const Path & target)
+template<typename Iterable>
+static uint64_t combineMountOpts(uint64_t init, const Iterable& opts)
 {
-    auto source = pval.source;
-    auto optional = pval.optional;
-    debug("bind mounting '%1%' to '%2%'", source, target);
+    return std::transform_reduce(std::begin(opts), std::end(opts), init, mergeIntoMountOpts,
+        [](const SandboxPath::MountOpt & o) { return static_cast<uint64_t>(o); });
+};
+
+static void doBind(const SandboxPath & v, const Path & target)
+{
+    debug("bind mounting '%1%' to '%2%'", v.source, target);
 
     auto bindMount = [&]() {
-        if (mount(source.c_str(), target.c_str(), "", MS_BIND | MS_REC, 0) == -1)
-            throw SysError("bind mount from '%1%' to '%2%' failed", source, target);
+        if (mount(v.source.c_str(), target.c_str(), "", MS_BIND | MS_REC, 0) == -1)
+            throw SysError("bind mount from '%1%' to '%2%' failed", v.source, target);
 
         // set extra options when wanted
-        auto flags = pval.readOnly ? combineMountOpts(0, pval.readOnlyDefaults) : 0;
-        flags = combineMountOpts(flags, pval.options);
+        auto flags = v.readOnly ? combineMountOpts(0, SandboxPath::readOnlyDefaults) : 0;
+        flags = combineMountOpts(flags, v.options);
         if (flags != 0) {
             // initial mount wouldn't respect MS_RDONLY, must remount
             debug("remounting '%s' with flags: %d", target, flags);
@@ -155,12 +153,12 @@ static void doBind(const SandboxPath & pval, const Path & target)
         }
     };
 
-    auto maybeSt = maybeLstat(source);
+    auto maybeSt = maybeLstat(v.source);
     if (!maybeSt) {
-        if (optional)
+        if (v.optional)
             return;
         else
-            throw SysError("getting attributes of path '%1%'", source);
+            throw SysError("getting attributes of path '%1%'", v.source);
     }
     auto st = *maybeSt;
 
@@ -170,7 +168,7 @@ static void doBind(const SandboxPath & pval, const Path & target)
     } else if (S_ISLNK(st.st_mode)) {
         // Symlinks can (apparently) not be bind-mounted, so just copy it
         createDirs(dirOf(target));
-        copyFile(std::filesystem::path(source), std::filesystem::path(target), false);
+        copyFile(std::filesystem::path(v.source), std::filesystem::path(target), false);
     } else {
         createDirs(dirOf(target));
         writeFile(target, "");