lix-project/lix
20
57
Fork 31
Code Issues 348 Code Review (Gerrit) Projects 3 Releases Wiki Activity

Explicitly model all settings and fail on unrecognized ones

Browse source 
Previously, the Settings class allowed other code to query for string
properties, which led to a proliferation of code all over the place making
up new options without any sort of central registry of valid options. This
commit pulls all those options back into the central Settings class and
removes the public get() methods, to discourage future abuses like that.

Furthermore, because we know the full set of options ahead of time, we
now fail loudly if someone enters an unrecognized option, thus preventing
subtle typos. With some template fun, we could probably also dump the full
set of options (with documentation, defaults, etc.) to the command line,
but I'm not doing that yet here.
This commit is contained in:
Dan Peebles 2017-02-21 22:50:18 -05:00
parent 8b1b5f9a12
commit e7cb2847ab
10 changed files with 218 additions and 87 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/libexpr/eval.cc
View file

@ -299,7 +299,7 @@ EvalState::EvalState(const Strings & _searchPath, ref<Store> store)
{
countCalls = getEnv("NIX_COUNT_CALLS", "0") != "0";
restricted = settings.get("restrict-eval", false);
restricted = settings.restrictEval;
assert(gcInitialised);

43
src/libstore/build.cc
View file

@ -34,13 +34,6 @@
#include <pwd.h>
#include <grp.h>
/* chroot-like behavior from Apple's sandbox */
#if __APPLE__
#define DEFAULT_ALLOWED_IMPURE_PREFIXES "/System/Library /usr/lib /dev /bin/sh"
#else
#define DEFAULT_ALLOWED_IMPURE_PREFIXES ""
#endif
/* Includes required for chroot support. */
#if __linux__
#include <sys/socket.h>
@ -1279,7 +1272,7 @@ void DerivationGoal::inputsRealised()
/* Don't repeat fixed-output derivations since they're already
verified by their output hash.*/
nrRounds = fixedOutput ? 1 : settings.get("build-repeat", 0) + 1;
nrRounds = fixedOutput ? 1 : settings.buildRepeat + 1;
/* Okay, try to build. Note that here we don't wait for a build
slot to become available, since we don't need one if there is a
@ -1685,9 +1678,7 @@ void DerivationGoal::startBuilder()
/* Are we doing a chroot build? */
{
string x = settings.get("build-use-sandbox",
/* deprecated alias */
settings.get("build-use-chroot", string("false")));
string x = settings.useSandbox;
if (x != "true" && x != "false" && x != "relaxed")
throw Error("option ‘build-use-sandbox’ must be set to one of ‘true’, ‘false’ or ‘relaxed’");
if (x == "true") {
@ -1744,21 +1735,10 @@ void DerivationGoal::startBuilder()
if (useChroot) {
string defaultChrootDirs;
#if __linux__
if (worker.store.isInStore(BASH_PATH))
defaultChrootDirs = "/bin/sh=" BASH_PATH;
#endif
/* Allow a user-configurable set of directories from the
host file system. */
PathSet dirs = tokenizeString<StringSet>(
settings.get("build-sandbox-paths",
/* deprecated alias with lower priority */
settings.get("build-chroot-dirs", defaultChrootDirs)));
PathSet dirs2 = tokenizeString<StringSet>(
settings.get("build-extra-chroot-dirs",
settings.get("build-extra-sandbox-paths", string(""))));
PathSet dirs = settings.sandboxPaths;
PathSet dirs2 = settings.extraSandboxPaths;
dirs.insert(dirs2.begin(), dirs2.end());
dirsInChroot.clear();
@ -1790,8 +1770,7 @@ void DerivationGoal::startBuilder()
for (auto & i : closure)
dirsInChroot[i] = i;
string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES));
PathSet allowedPaths = tokenizeString<StringSet>(allowed);
PathSet allowedPaths = settings.allowedImpureHostPrefixes;
/* This works like the above, except on a per-derivation level */
Strings impurePaths = tokenizeString<Strings>(get(drv->env, "__impureHostDeps"));
@ -1811,7 +1790,7 @@ void DerivationGoal::startBuilder()
}
}
if (!found)
throw Error(format("derivation ‘%1%’ requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed);
throw Error(format("derivation ‘%1%’ requested impure path ‘%2%’, but it was not in allowed-impure-host-deps") % drvPath % i);
dirsInChroot[i] = i;
}
@ -2433,7 +2412,7 @@ void DerivationGoal::runChild()
/* Mount a new tmpfs on /dev/shm to ensure that whatever
the builder puts in /dev/shm is cleaned up automatically. */
if (pathExists("/dev/shm") && mount("none", (chrootRootDir + "/dev/shm").c_str(), "tmpfs", 0,
fmt("size=%s", settings.get("sandbox-dev-shm-size", std::string("50%"))).c_str()) == -1)
fmt("size=%s", settings.sandboxShmSize).c_str()) == -1)
throw SysError("mounting /dev/shm");
#if 0
@ -2596,7 +2575,7 @@ void DerivationGoal::runChild()
sandboxProfile += "(version 1)\n";
/* Violations will go to the syslog if you set this. Unfortunately the destination does not appear to be configurable */
if (settings.get("darwin-log-sandbox-violations", false)) {
if (settings.darwinLogSandboxViolations) {
sandboxProfile += "(deny default)\n";
} else {
sandboxProfile += "(deny default (with no-log))\n";
@ -2743,7 +2722,7 @@ void DerivationGoal::registerOutputs()
InodesSeen inodesSeen;
Path checkSuffix = ".check";
bool runDiffHook = settings.get("run-diff-hook", false);
bool runDiffHook = settings.runDiffHook;
bool keepPreviousRound = settings.keepFailed || runDiffHook;
/* Check whether the output paths were created, and grep each
@ -2981,7 +2960,7 @@ void DerivationGoal::registerOutputs()
? fmt("output ‘%1%’ of ‘%2%’ differs from ‘%3%’ from previous round", i->path, drvPath, prev)
: fmt("output ‘%1%’ of ‘%2%’ differs from previous round", i->path, drvPath);
auto diffHook = settings.get("diff-hook", std::string(""));
auto diffHook = settings.diffHook;
if (prevExists && diffHook != "" && runDiffHook) {
try {
auto diff = runProgram(diffHook, true, {prev, i->path});
@ -2992,7 +2971,7 @@ void DerivationGoal::registerOutputs()
}
}
if (settings.get("enforce-determinism", true))
if (settings.enforceDeterminism)
throw NotDeterministic(msg);
printError(msg);

4
src/libstore/crypto.cc
View file

@ -105,12 +105,12 @@ PublicKeys getDefaultPublicKeys()
// FIXME: filter duplicates
for (auto s : settings.get("binary-cache-public-keys", Strings())) {
for (auto s : settings.binaryCachePublicKeys) {
PublicKey key(s);
publicKeys.emplace(key.name, key);
}
for (auto secretKeyFile : settings.get("secret-key-files", Strings())) {
for (auto secretKeyFile : settings.secretKeyFiles) {
try {
SecretKey secretKey(readFile(secretKeyFile));
publicKeys.emplace(secretKey.name, secretKey.toPublicKey());

6
src/libstore/download.cc
View file

@ -331,9 +331,9 @@ struct CurlDownloader : public Downloader
curl_multi_setopt(curlm, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
#endif
curl_multi_setopt(curlm, CURLMOPT_MAX_TOTAL_CONNECTIONS,
settings.get("binary-caches-parallel-connections", 25));
settings.binaryCachesParallelConnections);
enableHttp2 = settings.get("enable-http2", true);
enableHttp2 = settings.enableHttp2;
wakeupPipe.create();
fcntl(wakeupPipe.readSide.get(), F_SETFL, O_NONBLOCK);
@ -573,7 +573,7 @@ Path Downloader::downloadCached(ref<Store> store, const string & url_, bool unpa
string expectedETag;
int ttl = settings.get("tarball-ttl", 60 * 60);
int ttl = settings.tarballTtl;
bool skip = false;
if (pathExists(fileLink) && pathExists(dataFile)) {

148
src/libstore/globals.cc
View file

@ -17,12 +17,23 @@ namespace nix {
must be deleted and recreated on startup.) */
#define DEFAULT_SOCKET_PATH "/daemon-socket/socket"
/* chroot-like behavior from Apple's sandbox */
#if __APPLE__
#define DEFAULT_ALLOWED_IMPURE_PREFIXES "/System/Library /usr/lib /dev /bin/sh"
#else
#define DEFAULT_ALLOWED_IMPURE_PREFIXES ""
#endif
Settings settings;
Settings::Settings()
{
deprecatedOptions = StringSet({
"build-use-chroot", "build-chroot-dirs", "build-extra-chroot-dirs",
"this-option-never-existed-but-who-will-know"
});
nixPrefix = NIX_PREFIX;
nixStore = canonPath(getEnv("NIX_STORE_DIR", getEnv("NIX_STORE", NIX_STORE_DIR)));
nixDataDir = canonPath(getEnv("NIX_DATA_DIR", NIX_DATA_DIR));
@ -73,6 +84,32 @@ Settings::Settings()
showTrace = false;
enableImportNative = false;
netrcFile = fmt("%s/%s", nixConfDir, "netrc");
useSandbox = "false"; // TODO: make into an enum
#if __linux__
sandboxPaths = tokenizeString<StringSet>("/bin/sh=" BASH_PATH);
#endif
restrictEval = false;
buildRepeat = 0;
allowedImpureHostPrefixes = tokenizeString<StringSet>(DEFAULT_ALLOWED_IMPURE_PREFIXES);
sandboxShmSize = "50%";
darwinLogSandboxViolations = false;
runDiffHook = false;
diffHook = "";
enforceDeterminism = true;
binaryCachePublicKeys = Strings();
secretKeyFiles = Strings();
binaryCachesParallelConnections = 25;
enableHttp2 = true;
tarballTtl = 60 * 60;
signedBinaryCaches = "";
substituters = Strings();
binaryCaches = Strings();
extraBinaryCaches = Strings();
trustedUsers = Strings({"root"});
allowedUsers = Strings({"*"});
printMissing = true;
}
@ -115,39 +152,6 @@ void Settings::set(const string & name, const string & value)
overrides[name] = value;
}
string Settings::get(const string & name, const string & def)
{
auto i = settings.find(name);
if (i == settings.end()) return def;
return i->second;
}
Strings Settings::get(const string & name, const Strings & def)
{
auto i = settings.find(name);
if (i == settings.end()) return def;
return tokenizeString<Strings>(i->second);
}
bool Settings::get(const string & name, bool def)
{
bool res = def;
_get(res, name);
return res;
}
int Settings::get(const string & name, int def)
{
int res = def;
_get(res, name);
return res;
}
void Settings::update()
{
_get(tryFallback, "build-fallback");
@ -181,13 +185,71 @@ void Settings::update()
_get(keepGoing, "keep-going");
_get(keepFailed, "keep-failed");
_get(netrcFile, "netrc-file");
_get(useSandbox, "build-use-sandbox", "build-use-chroot");
_get(sandboxPaths, "build-sandbox-paths", "build-chroot-dirs");
_get(extraSandboxPaths, "build-extra-sandbox-paths", "build-extra-chroot-dirs");
_get(restrictEval, "restrict-eval");
_get(buildRepeat, "build-repeat");
_get(allowedImpureHostPrefixes, "allowed-impure-host-deps");
_get(sandboxShmSize, "sandbox-dev-shm-size");
_get(darwinLogSandboxViolations, "darwin-log-sandbox-violations");
_get(runDiffHook, "run-diff-hook");
_get(diffHook, "diff-hook");
_get(enforceDeterminism, "enforce-determinism");
_get(binaryCachePublicKeys, "binary-cache-public-keys");
_get(secretKeyFiles, "secret-key-files");
_get(binaryCachesParallelConnections, "binary-caches-parallel-connections");
_get(enableHttp2, "enable-http2");
_get(tarballTtl, "tarball-ttl");
_get(signedBinaryCaches, "signed-binary-caches");
_get(substituters, "substituters");
_get(binaryCaches, "binary-caches");
_get(extraBinaryCaches, "extra-binary-caches");
_get(trustedUsers, "trusted-users");
_get(allowedUsers, "allowed-users");
_get(printMissing, "print-missing");
/* Clear out any deprecated options that might be left, so users know we recognize the option
but aren't processing it anymore */
for (auto &i : deprecatedOptions) {
if (settings.find(i) != settings.end()) {
printError(format("warning: deprecated option '%1%' is no longer supported and will be ignored") % i);
settings.erase(i);
}
}
if (settings.size() != 0) {
string bad;
for (auto &i : settings)
bad += "'" + i.first + "', ";
bad.pop_back();
bad.pop_back();
throw Error(format("unrecognized options: %s") % bad);
}
}
void Settings::checkDeprecated(const string & name)
{
if (deprecatedOptions.find(name) != deprecatedOptions.end())
printError(format("warning: deprecated option '%1%' will soon be unsupported") % name);
}
void Settings::_get(string & res, const string & name)
{
SettingsMap::iterator i = settings.find(name);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
res = i->second;
}
void Settings::_get(string & res, const string & name1, const string & name2)
{
SettingsMap::iterator i = settings.find(name1);
if (i == settings.end()) i = settings.find(name2);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
res = i->second;
}
@ -196,6 +258,8 @@ void Settings::_get(bool & res, const string & name)
{
SettingsMap::iterator i = settings.find(name);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
if (i->second == "true") res = true;
else if (i->second == "false") res = false;
else throw Error(format("configuration option ‘%1%’ should be either ‘true’ or ‘false’, not ‘%2%’")
@ -207,6 +271,20 @@ void Settings::_get(StringSet & res, const string & name)
{
SettingsMap::iterator i = settings.find(name);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
res.clear();
Strings ss = tokenizeString<Strings>(i->second);
res.insert(ss.begin(), ss.end());
}
void Settings::_get(StringSet & res, const string & name1, const string & name2)
{
SettingsMap::iterator i = settings.find(name1);
if (i == settings.end()) i = settings.find(name2);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
res.clear();
Strings ss = tokenizeString<Strings>(i->second);
res.insert(ss.begin(), ss.end());
@ -216,6 +294,8 @@ void Settings::_get(Strings & res, const string & name)
{
SettingsMap::iterator i = settings.find(name);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
res = tokenizeString<Strings>(i->second);
}
@ -224,6 +304,8 @@ template<class N> void Settings::_get(N & res, const string & name)
{
SettingsMap::iterator i = settings.find(name);
if (i == settings.end()) return;
checkDeprecated(i->first);
settings.erase(i);
if (!string2Int(i->second, res))
throw Error(format("configuration setting ‘%1%’ should have an integer value") % name);
}

86
src/libstore/globals.hh
View file

@ -20,14 +20,6 @@ struct Settings {
void set(const string & name, const string & value);
string get(const string & name, const string & def);
Strings get(const string & name, const Strings & def);
bool get(const string & name, bool def);
int get(const string & name, int def);
void update();
string pack();
@ -36,6 +28,10 @@ struct Settings {
SettingsMap getOverrides();
/* TODO: the comments below should be strings and exposed via a nice command-line UI or similar.
We should probably replace it with some sort of magic template or macro to minimize the amount
of duplication and pain here. */
/* The directory where we store sources and derived files. */
Path nixStore;
@ -187,6 +183,75 @@ struct Settings {
/* Whether the importNative primop should be enabled */
bool enableImportNative;
/* Whether to enable sandboxed builds (string until we get an enum for true/false/relaxed) */
string useSandbox;
/* The basic set of paths to expose in a sandbox */
PathSet sandboxPaths;
/* Any extra sandbox paths to expose */
PathSet extraSandboxPaths;
/* Whether to allow certain questionable operations (like fetching) during evaluation */
bool restrictEval;
/* The number of times to repeat a build to check for determinism */
int buildRepeat;
/* Which prefixes to allow derivations to ask for access to (primarily for Darwin) */
PathSet allowedImpureHostPrefixes;
/* The size of /dev/shm in the build sandbox (for Linux) */
string sandboxShmSize;
/* Whether to log Darwin sandbox access violations to the system log */
bool darwinLogSandboxViolations;
/* ??? */
bool runDiffHook;
/* ??? */
string diffHook;
/* Whether to fail if repeated builds produce different output */
bool enforceDeterminism;
/* The known public keys for a binary cache */
Strings binaryCachePublicKeys;
/* Secret keys to use for build output signing */
Strings secretKeyFiles;
/* Number of parallel connections to hit a binary cache with when finding out if it contains hashes */
int binaryCachesParallelConnections;
/* Whether to enable HTTP2 */
bool enableHttp2;
/* How soon to expire tarballs like builtins.fetchTarball and (ugh, bad name) builtins.fetchurl */
int tarballTtl;
/* ??? */
string signedBinaryCaches;
/* ??? */
Strings substituters;
/* ??? */
Strings binaryCaches;
/* ??? */
Strings extraBinaryCaches;
/* Who we trust to ask the daemon to do unsafe things */
Strings trustedUsers;
/* ?Who we trust to use the daemon in safe ways */
Strings allowedUsers;
/* ??? */
bool printMissing;
/* The hook to run just before a build to set derivation-specific
build settings */
Path preBuildHook;
@ -196,11 +261,16 @@ struct Settings {
Path netrcFile;
private:
StringSet deprecatedOptions;
SettingsMap settings, overrides;
void checkDeprecated(const string & name);
void _get(string & res, const string & name);
void _get(string & res, const string & name1, const string & name2);
void _get(bool & res, const string & name);
void _get(StringSet & res, const string & name);
void _get(StringSet & res, const string & name1, const string & name2);
void _get(Strings & res, const string & name);
template<class N> void _get(N & res, const string & name);
};

4
src/libstore/local-store.cc
View file

@ -44,7 +44,7 @@ LocalStore::LocalStore(const Params & params)
, reservedPath(dbDir + "/reserved")
, schemaPath(dbDir + "/schema")
, trashDir(realStoreDir + "/trash")
, requireSigs(trim(settings.get("signed-binary-caches", std::string(""))) != "") // FIXME: rename option
, requireSigs(trim(settings.signedBinaryCaches) != "") // FIXME: rename option
, publicKeys(getDefaultPublicKeys())
{
auto state(_state.lock());
@ -1330,7 +1330,7 @@ void LocalStore::signPathInfo(ValidPathInfo & info)
{
// FIXME: keep secret keys in memory.
auto secretKeyFiles = settings.get("secret-key-files", Strings());
auto secretKeyFiles = settings.secretKeyFiles;
for (auto & secretKeyFile : secretKeyFiles) {
SecretKey secretKey(readFile(secretKeyFile));

6
src/libstore/store-api.cc
View file

@ -766,13 +766,13 @@ std::list<ref<Store>> getDefaultSubstituters()
state->stores.push_back(openStore(uri));
};
for (auto uri : settings.get("substituters", Strings()))
for (auto uri : settings.substituters)
addStore(uri);
for (auto uri : settings.get("binary-caches", Strings()))
for (auto uri : settings.binaryCaches)
addStore(uri);
for (auto uri : settings.get("extra-binary-caches", Strings()))
for (auto uri : settings.extraBinaryCaches)
addStore(uri);
state->done = true;

4
src/nix-daemon/nix-daemon.cc
View file

@ -865,8 +865,8 @@ static void daemonLoop(char * * argv)
struct group * gr = peer.gidKnown ? getgrgid(peer.gid) : 0;
string group = gr ? gr->gr_name : std::to_string(peer.gid);
Strings trustedUsers = settings.get("trusted-users", Strings({"root"}));
Strings allowedUsers = settings.get("allowed-users", Strings({"*"}));
Strings trustedUsers = settings.trustedUsers;
Strings allowedUsers = settings.allowedUsers;
if (matchUser(user, group, trustedUsers))
trusted = true;

2
src/nix-store/nix-store.cc
View file

@ -146,7 +146,7 @@ static void opRealise(Strings opFlags, Strings opArgs)
unknown = PathSet();
}
if (settings.get("print-missing", true))
if (settings.printMissing)
printMissing(ref<Store>(store), willBuild, willSubstitute, unknown, downloadSize, narSize);
if (dryRun) return;