From 76e88871b21c47c0216e160a5fb926f763ba64fe Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 19 Sep 2012 15:43:23 -0400 Subject: [PATCH 1/3] Templatise tokenizeString() --- src/libexpr/attr-path.cc | 2 +- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 2 +- src/libstore/build.cc | 19 ++++++++----------- src/libstore/gc.cc | 2 +- src/libstore/globals.cc | 20 ++++++-------------- src/libstore/local-store.cc | 6 +++--- src/libutil/util.cc | 7 +++++-- src/libutil/util.hh | 2 +- src/nix-setuid-helper/nix-setuid-helper.cc | 7 +++---- 10 files changed, 30 insertions(+), 39 deletions(-) diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index c9dbe8ef2..45794a62d 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -10,7 +10,7 @@ namespace nix { void findAlongAttrPath(EvalState & state, const string & attrPath, Bindings & autoArgs, Expr * e, Value & v) { - Strings tokens = tokenizeString(attrPath, "."); + Strings tokens = tokenizeString(attrPath, "."); Error attrError = Error(format("attribute selection path `%1%' does not match expression") % attrPath); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index c10177223..9e5be908f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -179,7 +179,7 @@ EvalState::EvalState() /* Initialise the Nix expression search path. */ searchPathInsertionPoint = searchPath.end(); - Strings paths = tokenizeString(getEnv("NIX_PATH", ""), ":"); + Strings paths = tokenizeString(getEnv("NIX_PATH", ""), ":"); foreach (Strings::iterator, i, paths) addToSearchPath(*i); addToSearchPath("nix=" + settings.nixDataDir + "/nix/corepkgs"); searchPathInsertionPoint = searchPath.begin(); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index d3809e698..bbe89c5f5 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -363,7 +363,7 @@ static void prim_derivationStrict(EvalState & state, Value * * args, Value & v) else throw EvalError(format("invalid value `%1%' for `outputHashMode' attribute") % s); } else if (key == "outputs") { - Strings tmp = tokenizeString(s); + Strings tmp = tokenizeString(s); outputs.clear(); foreach (Strings::iterator, j, tmp) { if (outputs.find(*j) != outputs.end()) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 04ba02d14..0b7790a5b 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1435,7 +1435,7 @@ HookReply DerivationGoal::tryBuildHook() /* Tell the hook about system features (beyond the system type) required from the build machine. (The hook could parse the drv file itself, but this is easier.) */ - Strings features = tokenizeString(drv.env["requiredSystemFeatures"]); + Strings features = tokenizeString(drv.env["requiredSystemFeatures"]); foreach (Strings::iterator, i, features) checkStoreName(*i); /* !!! abuse */ /* Send the request to the hook. */ @@ -1594,7 +1594,7 @@ void DerivationGoal::startBuilder() fixed-output derivations is by definition pure (since we already know the cryptographic hash of the output). */ if (fixedOutput) { - Strings varNames = tokenizeString(drv.env["impureEnvVars"]); + Strings varNames = tokenizeString(drv.env["impureEnvVars"]); foreach (Strings::iterator, i, varNames) env[*i] = getEnv(*i); } @@ -1606,7 +1606,7 @@ void DerivationGoal::startBuilder() by `nix-store --register-validity'. However, the deriver fields are left empty. */ string s = drv.env["exportReferencesGraph"]; - Strings ss = tokenizeString(s); + Strings ss = tokenizeString(s); if (ss.size() % 2 != 0) throw BuildError(format("odd number of tokens in `exportReferencesGraph': `%1%'") % s); for (Strings::iterator i = ss.begin(); i != ss.end(); ) { @@ -1911,14 +1911,11 @@ void DerivationGoal::initChild() outside of the namespace. Making a subtree private is local to the namespace, though, so setting MS_PRIVATE does not affect the outside world. */ - Strings mounts = tokenizeString(readFile("/proc/self/mountinfo", true), "\n"); + Strings mounts = tokenizeString(readFile("/proc/self/mountinfo", true), "\n"); foreach (Strings::iterator, i, mounts) { - Strings fields = tokenizeString(*i, " "); - assert(fields.size() >= 5); - Strings::iterator j = fields.begin(); - std::advance(j, 4); - if (mount(0, j->c_str(), 0, MS_PRIVATE, 0) == -1) - throw SysError(format("unable to make filesystem `%1%' private") % *j); + vector fields = tokenizeString >(*i, " "); + if (mount(0, fields.at(4).c_str(), 0, MS_PRIVATE, 0) == -1) + throw SysError(format("unable to make filesystem `%1%' private") % fields.at(4)); } /* Bind-mount all the directories from the "host" @@ -2053,7 +2050,7 @@ void DerivationGoal::initChild() PathSet parseReferenceSpecifiers(const Derivation & drv, string attr) { PathSet result; - Paths paths = tokenizeString(attr); + Paths paths = tokenizeString(attr); foreach (Strings::iterator, i, paths) { if (isStorePath(*i)) result.insert(*i); diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 4a025a8fe..ca16b6491 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -373,7 +373,7 @@ static void addAdditionalRoots(StoreAPI & store, PathSet & roots) string result = runProgram(rootFinder); - Strings paths = tokenizeString(result, "\n"); + Strings paths = tokenizeString(result, "\n"); foreach (Strings::iterator, i, paths) { if (isInStore(*i)) { diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 7e2624bbf..9b22d5450 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -65,15 +65,7 @@ void Settings::processEnvironment() substituters.push_back(nixLibexecDir + "/nix/substituters/download-using-manifests.pl"); substituters.push_back(nixLibexecDir + "/nix/substituters/download-from-binary-cache.pl"); } else - substituters = tokenizeString(subs, ":"); -} - - -string & at(Strings & ss, unsigned int n) -{ - Strings::iterator i = ss.begin(); - advance(i, n); - return *i; + substituters = tokenizeString(subs, ":"); } @@ -95,15 +87,15 @@ void Settings::loadConfFile() if (hash != string::npos) line = string(line, 0, hash); - Strings tokens = tokenizeString(line); + vector tokens = tokenizeString >(line); if (tokens.empty()) continue; - if (tokens.size() < 2 || at(tokens, 1) != "=") + if (tokens.size() < 2 || tokens[1] != "=") throw Error(format("illegal configuration line `%1%' in `%2%'") % line % settingsFile); - string name = at(tokens, 0); + string name = tokens[0]; - Strings::iterator i = tokens.begin(); + vector::iterator i = tokens.begin(); advance(i, 2); settings[name] = concatStringsSep(" ", Strings(i, tokens.end())); // FIXME: slow }; @@ -170,7 +162,7 @@ void Settings::get(PathSet & res, const string & name) SettingsMap::iterator i = settings.find(name); if (i == settings.end()) return; res.clear(); - Strings ss = tokenizeString(i->second); + Strings ss = tokenizeString(i->second); res.insert(ss.begin(), ss.end()); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 793ec89d9..085e662ed 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1435,7 +1435,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source) /* Lock the output path. But don't lock if we're being called from a build hook (whose parent process already acquired a lock on this path). */ - Strings locksHeld = tokenizeString(getEnv("NIX_HELD_LOCKS")); + Strings locksHeld = tokenizeString(getEnv("NIX_HELD_LOCKS")); if (find(locksHeld.begin(), locksHeld.end(), dstPath) == locksHeld.end()) outputLock.lockPaths(singleton(dstPath)); @@ -1645,7 +1645,7 @@ ValidPathInfo LocalStore::queryPathInfoOld(const Path & path) string info = readFile(infoFile); /* Parse it. */ - Strings lines = tokenizeString(info, "\n"); + Strings lines = tokenizeString(info, "\n"); foreach (Strings::iterator, i, lines) { string::size_type p = i->find(':'); @@ -1654,7 +1654,7 @@ ValidPathInfo LocalStore::queryPathInfoOld(const Path & path) string name(*i, 0, p); string value(*i, p + 2); if (name == "References") { - Strings refs = tokenizeString(value, " "); + Strings refs = tokenizeString(value, " "); res.references = PathSet(refs.begin(), refs.end()); } else if (name == "Deriver") { res.deriver = value; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 56bf5875d..7f95d3981 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -984,9 +984,9 @@ void _interrupted() ////////////////////////////////////////////////////////////////////// -Strings tokenizeString(const string & s, const string & separators) +template C tokenizeString(const string & s, const string & separators) { - Strings result; + C result; string::size_type pos = s.find_first_not_of(separators, 0); while (pos != string::npos) { string::size_type end = s.find_first_of(separators, pos + 1); @@ -998,6 +998,9 @@ Strings tokenizeString(const string & s, const string & separators) return result; } +template Strings tokenizeString(const string & s, const string & separators); +template vector tokenizeString(const string & s, const string & separators); + string concatStringsSep(const string & sep, const Strings & ss) { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 0616288cd..408d99a96 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -286,7 +286,7 @@ MakeError(Interrupted, BaseError) /* String tokenizer. */ -Strings tokenizeString(const string & s, const string & separators = " \t\n\r"); +template C tokenizeString(const string & s, const string & separators = " \t\n\r"); /* Concatenate the given strings with a separator between the diff --git a/src/nix-setuid-helper/nix-setuid-helper.cc b/src/nix-setuid-helper/nix-setuid-helper.cc index 26c457fd9..697964088 100644 --- a/src/nix-setuid-helper/nix-setuid-helper.cc +++ b/src/nix-setuid-helper/nix-setuid-helper.cc @@ -193,16 +193,15 @@ static void run(int argc, char * * argv) if (st.st_mode & (S_IWGRP | S_IWOTH)) throw Error(format("`%1%' should not be group or world-writable") % configFile); - Strings tokens = tokenizeString(readFile(fdConfig)); + vector tokens = tokenizeString >(readFile(fdConfig)); fdConfig.close(); if (tokens.size() != 2) throw Error(format("parse error in `%1%'") % configFile); - Strings::iterator i = tokens.begin(); - string nixUser = *i++; - string buildUsersGroup = *i++; + string nixUser = tokens[0]; + string buildUsersGroup = tokens[1]; /* Check that the caller (real uid) is the one allowed to call From b9124a5c336fd231adaa548cf5be311731847848 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 19 Sep 2012 15:45:29 -0400 Subject: [PATCH 2/3] Support having /nix/store as a read-only bind mount It turns out that the immutable bit doesn't work all that well. A better way is to make the entire Nix store a read-only bind mount, i.e. by doing $ mount --bind /nix/store /nix/store $ mount -o remount,ro,bind /nix/store (This would typically done in an early boot script, before anything from /nix/store is used.) Since Nix needs to be able to write to the Nix store, it now detects if /nix/store is a read-only bind mount and then makes it writable in a private mount namespace. --- src/libstore/local-store.cc | 39 +++++++++++++++++++++++++++++++++++++ src/libstore/local-store.hh | 2 ++ 2 files changed, 41 insertions(+) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 085e662ed..bb755b821 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -20,6 +20,11 @@ #include #include +#if HAVE_UNSHARE +#include +#include +#endif + #include @@ -292,6 +297,8 @@ LocalStore::LocalStore(bool reserveSpace) } else openDB(false); + + makeStoreWritable(); } @@ -411,6 +418,38 @@ void LocalStore::openDB(bool create) } +/* To improve purity, users may want to make the Nix store a read-only + bind mount. So make the Nix store writable for this process. */ +void LocalStore::makeStoreWritable() +{ +#if HAVE_UNSHARE + if (getuid() != 0) return; + + if (!pathExists("/proc/self/mountinfo")) return; + + /* Check if /nix/store is a read-only bind mount. */ + bool found = false; + Strings mounts = tokenizeString(readFile("/proc/self/mountinfo", true), "\n"); + foreach (Strings::iterator, i, mounts) { + vector fields = tokenizeString >(*i, " "); + if (fields.at(3) == "/" || fields.at(4) != settings.nixStore) continue; + Strings options = tokenizeString(fields.at(5), ","); + if (std::find(options.begin(), options.end(), "ro") == options.end()) continue; + found = true; + break; + } + + if (!found) return; + + if (unshare(CLONE_NEWNS) == -1) + throw SysError("setting up a private mount namespace"); + + if (mount(0, settings.nixStore.c_str(), 0, MS_REMOUNT | MS_BIND, 0) == -1) + throw SysError(format("remounting %1% writable") % settings.nixStore); +#endif +} + + const time_t mtimeStore = 1; /* 1 second into the epoch */ diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index d2b13d6a9..8899873a7 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -228,6 +228,8 @@ private: void openDB(bool create); + void makeStoreWritable(); + unsigned long long queryValidPathId(const Path & path); unsigned long long addValidPath(const ValidPathInfo & info, bool checkOutputs = true); From b9c2b4d5b4cd5d52a950e6dd90eb2e2e79891fa0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 19 Sep 2012 16:17:54 -0400 Subject: [PATCH 3/3] Remove setting of the immutable bit Using the immutable bit is problematic, especially in conjunction with store optimisation. For instance, if the garbage collector deletes a file, it has to clear its immutable bit, but if the file has additional hard links, we can't set the bit afterwards because we don't know the remaining paths. So now that we support having the entire Nix store as a read-only mount, we may as well drop the immutable bit. Unfortunately, we have to keep the code to clear the immutable bit for backwards compatibility. --- src/libstore/build.cc | 13 +---- src/libstore/local-store.cc | 2 - src/libstore/optimise-store.cc | 86 ++++++++++++---------------------- src/libutil/immutable.cc | 22 +-------- src/libutil/immutable.hh | 6 --- 5 files changed, 34 insertions(+), 95 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 0b7790a5b..4051adacc 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1355,11 +1355,6 @@ void DerivationGoal::buildDone() /* Delete the chroot (if we were using one). */ autoDelChroot.reset(); /* this runs the destructor */ - /* Deleting the chroot will have caused the immutable bits on - hard-linked inputs to be cleared. So set them again. */ - foreach (PathSet::iterator, i, regularInputPaths) - makeImmutable(*i); - /* Delete redirected outputs (when doing hash rewriting). */ foreach (PathSet::iterator, i, redirectedOutputs) deletePath(*i); @@ -1766,11 +1761,8 @@ void DerivationGoal::startBuilder() /* Hard-linking fails if we exceed the maximum link count on a file (e.g. 32000 of ext3), which is quite possible after a `nix-store - --optimise'. It can also fail if another - process called makeImmutable() on *i after we - did makeMutable(). In those cases, make a copy - instead. */ - if (errno != EMLINK && errno != EPERM) + --optimise'. */ + if (errno != EMLINK) throw SysError(format("linking `%1%' to `%2%'") % p % *i); StringSink sink; dumpPath(*i, sink); @@ -1778,7 +1770,6 @@ void DerivationGoal::startBuilder() restorePath(p, source); } - makeImmutable(*i); regularInputPaths.insert(*i); } } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index bb755b821..3fd772f26 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -517,8 +517,6 @@ void canonicalisePathMetaData(const Path & path, bool recurse) foreach (Strings::iterator, i, names) canonicalisePathMetaData(path + "/" + *i, true); } - - makeImmutable(path); } diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 9d0242bbc..43b3c9b4f 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -33,8 +33,7 @@ struct MakeReadOnly ~MakeReadOnly() { try { - /* This will make the path read-only (and restore the - immutable bit on platforms that support it). */ + /* This will make the path read-only. */ if (path != "") canonicalisePathMetaData(path, false); } catch (...) { ignoreException(); @@ -43,14 +42,6 @@ struct MakeReadOnly }; -struct MakeImmutable -{ - Path path; - MakeImmutable(const Path & path) : path(path) { } - ~MakeImmutable() { makeImmutable(path); } -}; - - void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) { checkInterrupt(); @@ -101,7 +92,6 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) if (!pathExists(linkPath)) { /* Nope, create a hard link in the links directory. */ makeMutable(path); - MakeImmutable mk1(path); if (link(path.c_str(), linkPath.c_str()) == 0) return; if (errno != EEXIST) throw SysError(format("cannot link `%1%' to `%2%'") % linkPath % path); @@ -134,57 +124,41 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) MakeReadOnly makeReadOnly(mustToggle ? dirOf(path) : ""); /* If ‘linkPath’ is immutable, we can't create hard links to it, - so make it mutable first (and make it immutable again when - we're done). We also have to make ‘path’ mutable, otherwise - rename() will fail to delete it. */ + so make it mutable first. We also have to make ‘path’ mutable, + otherwise rename() will fail to delete it. */ makeMutable(path); - MakeImmutable mk2(path); + makeMutable(linkPath); - /* Another process might be doing the same thing (creating a new - link to ‘linkPath’) and make ‘linkPath’ immutable before we're - done. In that case, just retry. */ - unsigned int retries = 1024; - while (--retries > 0) { - makeMutable(linkPath); - MakeImmutable mk1(linkPath); + Path tempLink = (format("%1%/.tmp-link-%2%-%3%") + % settings.nixStore % getpid() % rand()).str(); - Path tempLink = (format("%1%/.tmp-link-%2%-%3%") - % settings.nixStore % getpid() % rand()).str(); - - if (link(linkPath.c_str(), tempLink.c_str()) == -1) { - if (errno == EMLINK) { - /* Too many links to the same file (>= 32000 on most - file systems). This is likely to happen with empty - files. Just shrug and ignore. */ - if (st.st_size) - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; - } - if (errno == EPERM) continue; - throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); + if (link(linkPath.c_str(), tempLink.c_str()) == -1) { + if (errno == EMLINK) { + /* Too many links to the same file (>= 32000 on most file + systems). This is likely to happen with empty files. + Just shrug and ignore. */ + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; } - - /* Atomically replace the old file with the new hard link. */ - if (rename(tempLink.c_str(), path.c_str()) == -1) { - if (unlink(tempLink.c_str()) == -1) - printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); - if (errno == EMLINK) { - /* Some filesystems generate too many links on the - rename, rather than on the original link. - (Probably it temporarily increases the st_nlink - field before decreasing it again.) */ - if (st.st_size) - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; - } - if (errno == EPERM) continue; - throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); - } - - break; + throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); } - if (retries == 0) throw Error(format("cannot link `%1%' to `%2%'") % path % linkPath); + /* Atomically replace the old file with the new hard link. */ + if (rename(tempLink.c_str(), path.c_str()) == -1) { + if (unlink(tempLink.c_str()) == -1) + printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); + if (errno == EMLINK) { + /* Some filesystems generate too many links on the rename, + rather than on the original link. (Probably it + temporarily increases the st_nlink field before + decreasing it again.) */ + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); + } stats.filesLinked++; stats.bytesFreed += st.st_size; diff --git a/src/libutil/immutable.cc b/src/libutil/immutable.cc index f72f85625..766af4939 100644 --- a/src/libutil/immutable.cc +++ b/src/libutil/immutable.cc @@ -16,7 +16,7 @@ namespace nix { -void changeMutable(const Path & path, bool mut) +void makeMutable(const Path & path) { #if defined(FS_IOC_SETFLAGS) && defined(FS_IOC_GETFLAGS) && defined(FS_IMMUTABLE_FL) @@ -38,30 +38,12 @@ void changeMutable(const Path & path, bool mut) /* Silently ignore errors getting/setting the immutable flag so that we work correctly on filesystems that don't support it. */ if (ioctl(fd, FS_IOC_GETFLAGS, &flags)) return; - old = flags; - - if (mut) flags &= ~FS_IMMUTABLE_FL; - else flags |= FS_IMMUTABLE_FL; - + flags &= ~FS_IMMUTABLE_FL; if (old == flags) return; - if (ioctl(fd, FS_IOC_SETFLAGS, &flags)) return; - #endif } -void makeImmutable(const Path & path) -{ - changeMutable(path, false); -} - - -void makeMutable(const Path & path) -{ - changeMutable(path, true); -} - - } diff --git a/src/libutil/immutable.hh b/src/libutil/immutable.hh index 8af419004..8e98b76a4 100644 --- a/src/libutil/immutable.hh +++ b/src/libutil/immutable.hh @@ -4,12 +4,6 @@ namespace nix { -/* Make the given path immutable, i.e., prevent it from being modified - in any way, even by root. This is a no-op on platforms that do not - support this, or if the calling user is not privileged. On Linux, - this is implemented by doing the equivalent of ‘chattr +i path’. */ -void makeImmutable(const Path & path); - /* Make the given path mutable. */ void makeMutable(const Path & path);