From 1eef1927b6505cdc848b037b3af53c46a01e3871 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 18 Jun 2024 16:17:57 -0700 Subject: [PATCH 1/4] libexpr: fix accessing uninitialized values and fix pure-eval docs We got UBSan working on Lix, so we of course immediately found a bug and some definitely nonsense behaviour. Accessing `pureEval` or `restrictEval` from a default setting value is nonsense, since they would never be actually set by the time that value is set so they are not going to do anything. The configuration is not applied in an initializer (and even if it were, it's not going to be in the right order). After looking into *that*, we hunted down what actually was applying these, since clearly this code did not do anything. The EvalState constructor should have a "search path added and removed here :)" sign on it, because that's where it is done. We added an explicit initialization of the optional in there because it was otherwise unclear why pureEval also has the search path to allowed paths setup code run. We then realized that the `pureEval` documentation was *also* bogus, and we rewrote it. In so doing, we realized that we forgot to file a bug to make `builtins.storePath` work in pure eval mode, so we filed one of those: https://git.lix.systems/lix-project/lix/issues/402 Yaks have been thoroughly shorn. UBSan report: ../src/libexpr/eval-settings.cc:66:10: runtime error: member call on address 0x752fa9a13060 which does not point to an object of type 'nix::BaseSetting' 0x752fa9a13060: note: object has invalid vptr 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ invalid vptr 0 0x752fa95106a6 in nix::EvalSettings::getDefaultNixPath[abi:cxx11]() /home/jade/lix/lix2/build/src/libexpr/eval-settings.cc:66:10 1 0x752fa950e420 in nix::EvalSettings::EvalSettings() /home/jade/lix/lix2/build/src/libexpr/eval-settings.hh:36:15 2 0x752fa9469f1f in __cxx_global_var_init.50 /home/jade/lix/lix2/build/src/libexpr/eval-settings.cc:98:14 3 0x752fa9469f1f in _GLOBAL__sub_I_eval_settings.cc /home/jade/lix/lix2/build/src/libexpr/eval-settings.cc 4 0x752fabbd308d in call_init (/nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2+0x508d) (BuildId: a5b8228edc9f16078ac3c894af964eeb990ecb4c) 5 0x752fabbd317b in _dl_init (/nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2+0x517b) (BuildId: a5b8228edc9f16078ac3c894af964eeb990ecb4c) 6 0x752fabbe9c2f in _dl_start_user (/nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2+0x1bc2f) (BuildId: a5b8228edc9f16078ac3c894af964eeb990ecb4c) Change-Id: I5d8ffb7bfbe24b6584020ac74eed93d9f2e6d111 --- src/libexpr/eval-settings.cc | 8 +++----- src/libexpr/eval-settings.hh | 14 ++++++++++++-- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 12 +++++++----- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/libexpr/eval-settings.cc b/src/libexpr/eval-settings.cc index 105fd3e9d..0bdf1b9a5 100644 --- a/src/libexpr/eval-settings.cc +++ b/src/libexpr/eval-settings.cc @@ -63,11 +63,9 @@ Strings EvalSettings::getDefaultNixPath() } }; - if (!evalSettings.restrictEval && !evalSettings.pureEval) { - add(getNixDefExpr() + "/channels"); - add(rootChannelsDir() + "/nixpkgs", "nixpkgs"); - add(rootChannelsDir()); - } + add(getNixDefExpr() + "/channels"); + add(rootChannelsDir() + "/nixpkgs", "nixpkgs"); + add(rootChannelsDir()); return res; } diff --git a/src/libexpr/eval-settings.hh b/src/libexpr/eval-settings.hh index cd73d195f..4673c509b 100644 --- a/src/libexpr/eval-settings.hh +++ b/src/libexpr/eval-settings.hh @@ -75,8 +75,17 @@ struct EvalSettings : Config R"( Pure evaluation mode ensures that the result of Nix expressions is fully determined by explicitly declared inputs, and not influenced by external state: - - Restrict file system and network access to files specified by cryptographic hash - - Disable [`builtins.currentSystem`](@docroot@/language/builtin-constants.md#builtins-currentSystem) and [`builtins.currentTime`](@docroot@/language/builtin-constants.md#builtins-currentTime) + - File system and network access is restricted to accesses to immutable data only: + - Path literals relative to the home directory like `~/lix` are rejected at parse time. + - Access to absolute paths that did not result from Nix language evaluation is rejected when such paths are given as parameters to builtins like, for example, [`builtins.readFile`](@docroot@/language/builtins.md#builtins-readFile). + + Access is nonetheless allowed to (absolute) paths in the Nix store that are returned by builtins like [`builtins.filterSource`](@docroot@/language/builtins.md#builtins-filterSource), [`builtins.fetchTarball`](@docroot@/language/builtins.md#builtins-fetchTarball) and similar. + - Impure fetches such as not specifying a commit ID for `builtins.fetchGit` or not specifying a hash for `builtins.fetchTarball` are rejected. + - In flakes, access to relative paths outside of the root of the flake's source tree (often, a git repository) is rejected. + - The evaluator ignores `NIX_PATH`, `-I` and the `nix-path` setting. Thus, [`builtins.nixPath`](@docroot@/language/builtin-constants.md#builtins-nixPath) is an empty list. + - The builtins [`builtins.currentSystem`](@docroot@/language/builtin-constants.md#builtins-currentSystem) and [`builtins.currentTime`](@docroot@/language/builtin-constants.md#builtins-currentTime) are absent from `builtins`. + - [`builtins.getEnv`](@docroot@/language/builtin-constants.md#builtins-currentSystem) always returns empty string for any variable. + - [`builtins.storePath`](@docroot@/language/builtins.md#builtins-storePath) throws an error (Lix may change this, tracking issue: ) )" }; @@ -98,6 +107,7 @@ struct EvalSettings : Config allowed to access `https://github.com/NixOS/patchelf.git`. )"}; + Setting traceFunctionCalls{this, false, "trace-function-calls", R"( If set to `true`, the Nix evaluator will trace every function call. diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 25d98b23b..4b3593f09 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -418,7 +418,7 @@ EvalState::EvalState( } if (evalSettings.restrictEval || evalSettings.pureEval) { - allowedPaths = PathSet(); + allowedPaths = std::optional(PathSet()); for (auto & i : searchPath.elements) { auto r = resolveSearchPathPath(i.path); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 4b7e61dfe..20851da70 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -923,14 +923,15 @@ static RegisterPrimOp primop_getEnv({ .args = {"s"}, .doc = R"( `getEnv` returns the value of the environment variable *s*, or an - empty string if the variable doesn’t exist. This function should be + empty string if the variable doesn't exist. This function should be used with care, as it can introduce all sorts of nasty environment dependencies in your Nix expression. - `getEnv` is used in Nix Packages to locate the file - `~/.nixpkgs/config.nix`, which contains user-local settings for Nix - Packages. (That is, it does a `getEnv "HOME"` to locate the user’s - home directory.) + `getEnv` is used in nixpkgs for evil impurities such as locating the file + `~/.config/nixpkgs/config.nix` which contains user-local settings for nixpkgs. + (That is, it does a `getEnv "HOME"` to locate the user's home directory.) + + When in [pure evaluation mode](@docroot@/command-ref/conf-file.md#conf-pure-eval), this function always returns an empty string. )", .fun = prim_getEnv, }); @@ -1506,6 +1507,7 @@ static RegisterPrimOp primop_storePath({ in a new path (e.g. `/nix/store/ld01dnzc…-source-source`). Not available in [pure evaluation mode](@docroot@/command-ref/conf-file.md#conf-pure-eval). + Lix may change this, tracking issue: See also [`builtins.fetchClosure`](#builtins-fetchClosure). )", From e0a3a5f2267b28b242c5e2fbaca261b0bbd607c0 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 18 Jun 2024 16:31:58 -0700 Subject: [PATCH 2/4] build: make UBSan work :) This is really just a question of turning off the production sanitizer configuration so we get nice diagnostics. Not much else to say. Change-Id: I76bd6d225320056ed95bd89955f00beff2db0d2f --- meson.build | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 672db4ddf..e6151e0a2 100644 --- a/meson.build +++ b/meson.build @@ -437,7 +437,9 @@ add_project_arguments( language : 'cpp', ) -if cxx.get_id() in ['gcc', 'clang'] +# We turn off the production UBSan if the slower dev UBSan is requested, to +# give better diagnostics. +if cxx.get_id() in ['gcc', 'clang'] and 'undefined' not in get_option('b_sanitize') # 2024-03-24: jade benchmarked the default sanitize reporting in clang and got # a regression of about 10% on hackage-packages.nix with clang. So we are trapping instead. # @@ -452,6 +454,11 @@ if cxx.get_id() in ['gcc', 'clang'] add_project_arguments(sanitize_args, language: 'cpp') add_project_link_arguments(sanitize_args, language: 'cpp') endif +# Clang's default of -no-shared-libsan on Linux causes link errors; on macOS it defaults to shared. +# GCC defaults to shared libsan so is fine. +if cxx.get_id() == 'clang' and get_option('b_sanitize') != '' + add_project_link_arguments('-shared-libsan', language : 'cpp') +endif add_project_link_arguments('-pthread', language : 'cpp') if cxx.get_linker_id() in ['ld.bfd', 'ld.gold'] From f2fff1faa4c6a308ad30da691e18ceccf6626e0d Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 18 Jun 2024 19:11:44 -0700 Subject: [PATCH 3/4] libmain: fix UB in verbosity assignment This was generating an out-of-range verbosity value. We should just process it as an int and then convert to verbosity with a clamping function, which trivially avoids any domain type violations. Change-Id: I0ed20da8e1496a1225ff3008b76827d99265d404 --- src/libmain/common-args.cc | 5 +++-- src/libutil/error.hh | 2 ++ src/libutil/logging.cc | 7 +++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/libmain/common-args.cc b/src/libmain/common-args.cc index 20b5befe4..8fcb10325 100644 --- a/src/libmain/common-args.cc +++ b/src/libmain/common-args.cc @@ -1,5 +1,6 @@ #include "common-args.hh" #include "args/root.hh" +#include "error.hh" #include "globals.hh" #include "loggers.hh" #include "logging.hh" @@ -14,14 +15,14 @@ MixCommonArgs::MixCommonArgs(const std::string & programName) .shortName = 'v', .description = "Increase the logging verbosity level.", .category = loggingCategory, - .handler = {[]() { verbosity = (Verbosity) (verbosity + 1); }}, + .handler = {[]() { verbosity = verbosityFromIntClamped(int(verbosity) + 1); }}, }); addFlag({ .longName = "quiet", .description = "Decrease the logging verbosity level.", .category = loggingCategory, - .handler = {[]() { verbosity = verbosity > lvlError ? (Verbosity) (verbosity - 1) : lvlError; }}, + .handler = {[]() { verbosity = verbosityFromIntClamped(int(verbosity) - 1); }}, }); addFlag({ diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 0884f9f32..06dfba0df 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -45,6 +45,8 @@ typedef enum { lvlVomit } Verbosity; +Verbosity verbosityFromIntClamped(int val); + /** * The lines of code surrounding an error. */ diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index b01bb4dd4..fecbc89ac 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -5,6 +5,7 @@ #include "position.hh" #include "terminal.hh" +#include #include #include #include @@ -110,6 +111,12 @@ public: Verbosity verbosity = lvlInfo; +Verbosity verbosityFromIntClamped(int val) +{ + int clamped = std::clamp(val, int(lvlError), int(lvlVomit)); + return static_cast(clamped); +} + void writeToStderr(std::string_view s) { try { From c897fba787c0a0c829f384717ffdc152365a07b2 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 18 Jun 2024 19:13:40 -0700 Subject: [PATCH 4/4] store: fix null reference from DerivationGoal::waiteeDone This happened during a PathSubstitutionGoal of a .drv file: substitution of '/tmp/jade/nix-test/ca/eval-store/store/1lj7lsq5y0f25mfbnq6d3zd0bw5ay33n-dependencies-input-2.drv' What happened here is that since PathSubstitutionGoal is not a DerivationGoal, in production builds, the UB was not caught, since it would early-exit from failing a dynamic_cast to DerivationGoal * on the very next line, but before the null reference was ever used. This was nonetheless UB. The fix should be to just rearrange the two lines; I don't think there is a further bug there, since *substituting a .drv* **necessarily** means you cannot have the representation of the derivation as would be necessary for drv to not be null there. Test failure: ++(eval-store.sh:12) _RR_TRACE_DIR=/home/jade/.local/share/rr rr record -- nix build -f dependencies.nix --eval-store /tmp/jade/nix-test/ca/eval-store/eval-store -o /tmp/jade/nix-test/ca/eval-store/result don't know how to build these paths: /tmp/jade/nix-test/ca/eval-store/store/6y51mf0p57ggipgab6hdjabbvplzsicq-dependencies-top.drv copying 1 paths... copying path '/tmp/jade/nix-test/ca/eval-store/store/8027afyvqb87y1sf5xhdkqsflqn1ziy8-dependencies.builder0.sh' to 'local'... copying 1 paths... copying path '/tmp/jade/nix-test/ca/eval-store/store/7r5pqyncvfgrryf9gzy1z56z3xigi61x-builder-dependencies-input-0.sh' to 'local'... copying 1 paths... copying path '/tmp/jade/nix-test/ca/eval-store/store/nhmgm87zlqy3ks96dxrn7l37b72azi99-builder-dependencies-input-1.sh' to 'local'... copying 1 paths... copying path '/tmp/jade/nix-test/ca/eval-store/store/nq4qa2j6y8ajqazlfq6h46ck637my1n6-builder-dependencies-input-2.sh' to 'local'... copying 1 paths... copying path '/tmp/jade/nix-test/ca/eval-store/store/6vh0vna9l5afck01y7iaks3hm9ikwqyj-builder-fod-input.sh' to 'local'... building '/tmp/jade/nix-test/ca/eval-store/store/gy91pqymf2nc5v7ld1bad94xpwxdi25s-dependencies-input-0.drv'... building '/tmp/jade/nix-test/ca/eval-store/store/w7wlkjx97ivmnrymkac5av3nyp94hzvq-dependencies-input-1.drv'... ../src/libstore/build/derivation-goal.cc:1556:22: runtime error: reference binding to null pointer of type 'Derivation' 0 0x734ba59a6886 in nix::DerivationGoal::waiteeDone(std::shared_ptr, nix::Goal::ExitCode) /home/jade/lix/lix2/build/src/libstore/build/derivation-goal.cc:1556:12 1 0x734ba59c0962 in nix::Goal::amDone(nix::Goal::ExitCode, std::optional) /home/jade/lix/lix2/build/src/libstore/build/goal.cc:95:25 2 0x734ba5a1c44a in nix::PathSubstitutionGoal::done(nix::Goal::ExitCode, nix::BuildResult::Status, std::optional, std::allocator>>) /home/jade/lix/lix2/build/src/libstore/build/substitution-goal.cc:38:5 3 0x734ba5a1b454 in nix::PathSubstitutionGoal::init() /home/jade/lix/lix2/build/src/libstore/build/substitution-goal.cc:56:9 4 0x734ba5a2a6c6 in nix::Worker::run(std::set, nix::CompareGoalPtrs, std::allocator>> const&) /home/jade/lix/lix2/build/src/libstore/build/worker.cc:320:23 5 0x734ba59b93d8 in nix::Store::buildPathsWithResults(std::vector> const&, nix::BuildMode, std::shared_ptr) /home/jade/lix/lix2/build/src/libstore/build/entry-points.cc:60:12 6 0x734ba663c107 in nix::Installable::build2(nix::ref, nix::ref, nix::Realise, std::vector, std::allocator>> const&, nix::BuildMode) /home/jade/lix/lix2/build/src/libcmd/installables.cc:637:36 Change-Id: Id0e651e480bebf6356733b01bc639e9bb59c7bd0 --- src/libstore/build/derivation-goal.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 97ba994ad..46399b0a8 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1553,11 +1553,12 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result) Goal::waiteeDone(waitee, result); if (!useDerivation) return; - auto & fullDrv = *dynamic_cast(drv.get()); auto * dg = dynamic_cast(&*waitee); if (!dg) return; + auto & fullDrv = *dynamic_cast(drv.get()); + auto * nodeP = fullDrv.inputDrvs.findSlot(DerivedPath::Opaque { .path = dg->drvPath }); if (!nodeP) return; auto & outputs = nodeP->value;