From 8edcae1bf0f0a061b83ac3d60e0a99209b7bdf34 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 14:33:12 -0600 Subject: [PATCH 1/6] add an impl of Expr::show for ExprInheritFrom that doesn't crash ExprVar::show() assumes it has a name. dynamic inherits do not necessarily (ever?) have a name. Change-Id: If10893188e307431da17f0c1bd0787adc74f7141 --- src/libexpr/nixexpr.cc | 7 +++++++ src/libexpr/nixexpr.hh | 3 ++- tests/unit/libexpr/expr-print.cc | 34 ++++++++++++++++++++++++++++++++ tests/unit/meson.build | 1 + 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 tests/unit/libexpr/expr-print.cc diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index bc53ca053..08d4b279b 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -50,6 +50,13 @@ void ExprVar::show(const SymbolTable & symbols, std::ostream & str) const str << symbols[name]; } +void ExprInheritFrom::show(SymbolTable const & symbols, std::ostream & str) const +{ + str << "(/* expanded inherit (expr) */ "; + fromExpr->show(symbols, str); + str << ")"; +} + void ExprSelect::show(const SymbolTable & symbols, std::ostream & str) const { str << "("; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 703a32e8f..25ba94595 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -154,7 +154,8 @@ struct ExprInheritFrom : ExprVar this->fromWith = nullptr; } - void bindVars(EvalState & es, const std::shared_ptr & env); + void show(SymbolTable const & symbols, std::ostream & str) const override; + void bindVars(EvalState & es, const std::shared_ptr & env) override; }; struct ExprSelect : Expr diff --git a/tests/unit/libexpr/expr-print.cc b/tests/unit/libexpr/expr-print.cc new file mode 100644 index 000000000..aa25d00df --- /dev/null +++ b/tests/unit/libexpr/expr-print.cc @@ -0,0 +1,34 @@ +#include +#include + +#include + +#include "tests/libexpr.hh" + +#include "nixexpr.hh" +#include "ref.hh" + +namespace nix +{ + +using namespace testing; +struct ExprPrintingTests : LibExprTest +{ + void test(Expr const & expr, std::string_view expected) + { + std::stringstream out; + expr.show(state.symbols, out); + ASSERT_EQ(out.str(), expected); + } +}; + +TEST_F(ExprPrintingTests, ExprInheritFrom) +{ + // ExprInheritFrom has its own show() impl. + // If it uses its parent class's impl it will crash. + auto inheritSource = make_ref(state.symbols.create("stdenv")); + ExprInheritFrom const eInheritFrom(noPos, 0, inheritSource); + test(eInheritFrom, "(/* expanded inherit (expr) */ stdenv)"); +} + +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 2b5471526..e2ca95629 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -185,6 +185,7 @@ libexpr_tests_sources = files( 'libexpr/primops.cc', 'libexpr/search-path.cc', 'libexpr/trivial.cc', + 'libexpr/expr-print.cc', 'libexpr/value/context.cc', 'libexpr/value/print.cc', ) From d6268d7afb5edca3a554960eae8caa7eef9bd057 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:17:41 -0600 Subject: [PATCH 2/6] add an ExprPrinter class, like ValuePrinter To be used Shortly Change-Id: I9def7975aa55f251eb8486391677771f7352d7ce --- src/libexpr/print.cc | 6 ++++++ src/libexpr/print.hh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index e387a09fb..87db004b2 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -574,4 +574,10 @@ fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & va return *this; } +std::ostream & operator<<(std::ostream & output, ExprPrinter const & printer) +{ + printer.expr.show(printer.state.symbols, output); + return output; +} + } diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 42826d94d..3deaa33d4 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -15,6 +15,8 @@ namespace nix { +struct Expr; + class EvalState; struct Value; @@ -50,6 +52,12 @@ void printValue(EvalState & state, std::ostream & str, Value & v, PrintOptions o /** * A partially-applied form of `printValue` which can be formatted using `<<` * without allocating an intermediate string. + * This class should not outlive the eval state or it will UAF. + * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have + * EvalState &, not ref, and constructing a new shared_ptr to data that + * already has a shared_ptr is a much bigger footgun. In the current architecture of + * libexpr, using a ValuePrinter after an EvalState has been destroyed would be + * pretty hard. */ class ValuePrinter { friend std::ostream & operator << (std::ostream & output, const ValuePrinter & printer); @@ -73,4 +81,26 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer); template<> fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value); +/** + * A partially-applied form of Expr::show(), which can be formatted using `<<` + * without allocating an intermediate string. + * This class should not outlive the eval state or it will UAF. + * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have + * EvalState &, not ref, and constructing a new shared_ptr to data that + * already has a shared_ptr is a much bigger footgun. In the current architecture of + * libexpr, using an ExprPrinter after an EvalState has been destroyed would be + * pretty hard. + */ +class ExprPrinter +{ + /** The eval state used to get symbols. */ + EvalState const & state; + /** The expression to print. */ + Expr const & expr; + +public: + ExprPrinter(EvalState const & state, Expr const & expr) : state(state), expr(expr) { } + friend std::ostream & operator << (std::ostream & output, ExprPrinter const & printer); +}; + } From c1bfc99017ab8ec22e9e4cf48ce3b0e24de4338a Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:22:29 -0600 Subject: [PATCH 3/6] trace when the `foo` part of `foo.bar.baz` errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like: let errpkg = throw "invalid foobar"; in errpkg.meta error: … while calling the 'throw' builtin at «string»:2:12: 1| let 2| errpkg = throw "invalid foobar"; | ^ 3| in errpkg.meta error: invalid foobar into errors like: let errpkg = throw "invalid foobar"; in errpkg.meta error: … while evaluating 'errpkg' to select 'meta' on it at «string»:3:4: 2| errpkg = throw "invalid foobar"; 3| in errpkg.meta | ^ … while calling the 'throw' builtin at «string»:2:12: 1| let 2| errpkg = throw "invalid foobar"; | ^ 3| in errpkg.meta error: invalid foobar For the low price of one try/catch, you too can have the incorrect line of code actually show up in the trace! Change-Id: If8d6200ec1567706669d405c34adcd7e2d2cd29d --- src/libexpr/eval.cc | 15 ++++++++++++++- tests/functional/lang/eval-fail-recursion.err.exp | 6 ++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a6a64a43c..b01867a09 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1432,7 +1432,20 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) Value * vCurrent = &vFirst; // Position for the current attrset Value in this select chain. PosIdx posCurrent; - e->eval(state, env, vFirst); + + try { + e->eval(state, env, vFirst); + } catch (Error & e) { + assert(this->e != nullptr); + state.addErrorTrace( + e, + getPos(), + "while evaluating '%s' to select '%s' on it", + ExprPrinter(state, *this->e), + showAttrPath(state.symbols, this->attrPath) + ); + throw; + } try { auto dts = state.debugRepl diff --git a/tests/functional/lang/eval-fail-recursion.err.exp b/tests/functional/lang/eval-fail-recursion.err.exp index 19380dc65..f0057b2d5 100644 --- a/tests/functional/lang/eval-fail-recursion.err.exp +++ b/tests/functional/lang/eval-fail-recursion.err.exp @@ -1,4 +1,10 @@ error: + … while evaluating 'a' to select 'foo' on it + at /pwd/lang/eval-fail-recursion.nix:1:21: + 1| let a = {} // a; in a.foo + | ^ + 2| + … in the right operand of the update (//) operator at /pwd/lang/eval-fail-recursion.nix:1:12: 1| let a = {} // a; in a.foo From 029d191c9d6d282bbedbb53800404b9b397150ef Mon Sep 17 00:00:00 2001 From: Qyriad Date: Sat, 22 Jun 2024 21:59:47 -0600 Subject: [PATCH 4/6] trace which part of `foo.bar.baz` errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like: let somepkg.src = throw "invalid foobar"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta … while calling the 'throw' builtin at «string»:2:17: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta error: invalid foobar into errors like: let somepkg.src = throw "invalid foobar"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta … while evaluating 'somepkg.src' to select 'meta' on it at «string»:3:4: 2| somepkg.src = throw "invalid foobar"; 3| in somepkg.src.meta | ^ … while calling the 'throw' builtin at «string»:2:17: 1| let 2| somepkg.src = throw "invalid foobar"; | ^ 3| in somepkg.src.meta error: invalid foobar And for type errors, from: let somepkg.src = "I'm not an attrset"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = "I'm not an attrset"; | ^ 3| in somepkg.src.meta … while selecting an attribute at «string»:3:4: 2| somepkg.src = "I'm not an attrset"; 3| in somepkg.src.meta | ^ error: expected a set but found a string: "I'm not an attrset" into: let somepkg.src = "I'm not an attrset"; in somepkg.src.meta error: … while evaluating the attribute 'src.meta' at «string»:2:3: 1| let 2| somepkg.src = "I'm not an attrset"; | ^ 3| in somepkg.src.meta … while selecting 'meta' on 'somepkg.src' at «string»:3:4: 2| somepkg.src = "I'm not an attrset"; 3| in somepkg.src.meta | ^ error: expected a set but found a string: "I'm not an attrset" For the low price of an enumerate() and a lambda you too can have the incorrect line of code actually show up in the trace! Change-Id: Ic1491c86e33c167891bdac9adad6224784760bd6 --- src/libexpr/eval.cc | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b01867a09..56581cc19 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1458,12 +1458,46 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) showAttrPath(state, env, attrPath)) : nullptr; - for (auto const & currentAttrName : attrPath) { + for (auto const & [partIdx, currentAttrName] : enumerate(attrPath)) { state.nrLookups++; Symbol const name = getName(currentAttrName, state, env); - state.forceValue(*vCurrent, pos); + // For formatting errors, which should be done only when needed. + auto partsSoFar = [&]() -> std::string { + std::stringstream ss; + // We start with the base thing this ExprSelect is selecting on. + assert(this->e != nullptr); + this->e->show(state.symbols, ss); + + // Then grab each part of the attr path up to this one. + assert(partIdx < attrPath.size()); + std::span const parts( + attrPath.begin(), + attrPath.begin() + partIdx + ); + + // And convert them to strings and join them. + for (auto const & part : parts) { + auto const partName = getName(part, state, env); + ss << "." << state.symbols[partName]; + } + + return ss.str(); + }; + + try { + state.forceValue(*vCurrent, pos); + } catch (Error & e) { + state.addErrorTrace( + e, + getPos(), + "while evaluating '%s' to select '%s' on it", + partsSoFar(), + state.symbols[name] + ); + throw; + } if (vCurrent->type() != nAttrs) { @@ -1479,7 +1513,10 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) "expected a set but found %s: %s", showType(*vCurrent), ValuePrinter(state, *vCurrent, errorPrintOptions) - ).withTrace(pos, "while selecting an attribute").debugThrow(); + ).addTrace( + pos, + HintFmt("while selecting '%s' on '%s'", state.symbols[name], partsSoFar()) + ).debugThrow(); } // Now that we know this is actually an attrset, try to find an attr From d680f9fe57ceac422824df5c49c944a6dc02505c Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 17:26:21 -0600 Subject: [PATCH 5/6] distinguish between throws & errors during throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns errors like this: let throwMsg = a: throw (a + " invalid bar"); in throwMsg "bullshit" error: … from call site at «string»:3:4: 2| throwMsg = a: throw (a + " invalid bar"); 3| in throwMsg "bullshit" | ^ … while calling 'throwMsg' at «string»:2:14: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" … while calling the 'throw' builtin at «string»:2:17: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" error: bullshit invalid bar into errors like this: let throwMsg = a: throw (a + " invalid bar"); in throwMsg "bullshit" error: … from call site at «string»:3:4: 2| throwMsg = a: throw (a + " invalid bar"); 3| in throwMsg "bullshit" | ^ … while calling 'throwMsg' at «string»:2:14: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" … caused by explicit throw at «string»:2:17: 1| let 2| throwMsg = a: throw (a + " invalid bar"); | ^ 3| in throwMsg "bullshit" error: bullshit invalid bar Change-Id: I593688928ece20f97999d1bf03b2b46d9ac338cb --- src/libexpr/eval.cc | 9 +++++++++ tests/functional/eval.sh | 2 +- tests/functional/lang/eval-fail-duplicate-traces.err.exp | 2 +- .../eval-fail-foldlStrict-strict-op-application.err.exp | 2 +- tests/functional/lang/eval-fail-mutual-recursion.err.exp | 2 +- tests/functional/lang/eval-fail-not-throws.err.exp | 2 +- tests/functional/lang/eval-fail-toJSON.err.exp | 2 +- 7 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 56581cc19..d9bdb0d2c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1807,6 +1807,15 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & try { fn->fun(*this, vCur.determinePos(noPos), args, vCur); + } catch (ThrownError & e) { + // Distinguish between an error that simply happened while "throw" + // was being evaluated and an explicit thrown error. + if (fn->name == "throw") { + addErrorTrace(e, pos, "caused by explicit %s", "throw"); + } else { + addErrorTrace(e, pos, "while calling the '%s' builtin", fn->name); + } + throw; } catch (Error & e) { addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; diff --git a/tests/functional/eval.sh b/tests/functional/eval.sh index 9c125b569..ae6fcec63 100644 --- a/tests/functional/eval.sh +++ b/tests/functional/eval.sh @@ -25,7 +25,7 @@ nix eval -E 'assert 1 + 2 == 3; true' # Top-level eval errors should be printed to stderr with a traceback. topLevelThrow="$(expectStderr 1 nix eval --expr 'throw "a sample throw message"')" [[ "$topLevelThrow" =~ "a sample throw message" ]] -[[ "$topLevelThrow" =~ "while calling the 'throw' builtin" ]] +[[ "$topLevelThrow" =~ "caused by explicit throw" ]] # But errors inside something should print an elided version, and exit with 0. outputOfNestedThrow="$(nix eval --expr '{ throws = throw "a sample throw message"; }')" diff --git a/tests/functional/lang/eval-fail-duplicate-traces.err.exp b/tests/functional/lang/eval-fail-duplicate-traces.err.exp index cedaebd3b..d9e2ec945 100644 --- a/tests/functional/lang/eval-fail-duplicate-traces.err.exp +++ b/tests/functional/lang/eval-fail-duplicate-traces.err.exp @@ -41,7 +41,7 @@ error: | ^ 5| if n > 0 - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-duplicate-traces.nix:7:10: 6| then throwAfter (n - 1) 7| else throw "Uh oh!"; diff --git a/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp b/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp index 4903bc82d..6955fad13 100644 --- a/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp +++ b/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp @@ -27,7 +27,7 @@ error: | ^ 6| - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:5:9: 4| null 5| [ (_: throw "Not the final value, but is still forced!") (_: 23) ] diff --git a/tests/functional/lang/eval-fail-mutual-recursion.err.exp b/tests/functional/lang/eval-fail-mutual-recursion.err.exp index c034afcd5..c03d2e840 100644 --- a/tests/functional/lang/eval-fail-mutual-recursion.err.exp +++ b/tests/functional/lang/eval-fail-mutual-recursion.err.exp @@ -54,7 +54,7 @@ error: (21 duplicate frames omitted) - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-mutual-recursion.nix:34:10: 33| then throwAfterB true 10 34| else throw "Uh oh!"; diff --git a/tests/functional/lang/eval-fail-not-throws.err.exp b/tests/functional/lang/eval-fail-not-throws.err.exp index fc81f7277..5882a260a 100644 --- a/tests/functional/lang/eval-fail-not-throws.err.exp +++ b/tests/functional/lang/eval-fail-not-throws.err.exp @@ -5,7 +5,7 @@ error: | ^ 2| - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-not-throws.nix:1:4: 1| ! (throw "uh oh!") | ^ diff --git a/tests/functional/lang/eval-fail-toJSON.err.exp b/tests/functional/lang/eval-fail-toJSON.err.exp index ad267711b..18c334923 100644 --- a/tests/functional/lang/eval-fail-toJSON.err.exp +++ b/tests/functional/lang/eval-fail-toJSON.err.exp @@ -40,7 +40,7 @@ error: | ^ 8| } - … while calling the 'throw' builtin + … caused by explicit throw at /pwd/lang/eval-fail-toJSON.nix:7:13: 6| { 7| c.d = throw "hah no"; From 07ddac2c5123cac426cce13ad552560b4c0b5f53 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 19:08:53 -0600 Subject: [PATCH 6/6] mildly cleanup libexpr/eval.hh Change-Id: I40d01a8f8b7fb101279c6f88ebdf1f0969d9d7f0 --- src/libexpr/eval.hh | 51 ++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 8e390e46d..2d12a6532 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -9,14 +9,13 @@ #include "symbol-table.hh" #include "config.hh" #include "experimental-features.hh" -#include "input-accessor.hh" #include "search-path.hh" #include "repl-exit-status.hh" +#include #include #include #include -#include #include namespace nix { @@ -38,11 +37,26 @@ namespace eval_cache { class EvalCache; } +/** Alias for std::map which uses boehmgc's allocator conditional on us actually + * using boehmgc in this build. + */ +#if HAVE_BOEHMGC + template + using GcMap = std::map< + KeyT, + ValueT, + std::less, + traceable_allocator> + >; +#else + using GcMap = std::map +#endif + /** * Function that implements a primop. */ -typedef void (* PrimOpFun) (EvalState & state, const PosIdx pos, Value * * args, Value & v); +using PrimOpImpl = void(EvalState & state, PosIdx pos, Value ** args, Value & v); /** * Info about a primitive operation, and its implementation @@ -76,7 +90,7 @@ struct PrimOp /** * Implementation of the primop. */ - std::function::type> fun; + std::function fun; /** * Optional experimental for this to be gated on. @@ -115,11 +129,7 @@ struct Constant bool impureOnly = false; }; -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator > > ValMap; -#else - typedef std::map ValMap; -#endif +using ValMap = GcMap; struct Env { @@ -214,7 +224,8 @@ public: /** * Debugger */ - ReplExitStatus (* debugRepl)(ref es, const ValMap & extraEnv); + using ReplCallbackImpl = ReplExitStatus(ref es, ValMap const & extraEnv); + ReplCallbackImpl * debugRepl; bool debugStop; bool inDebugger = false; int trylevel; @@ -252,21 +263,13 @@ private: /** * A cache from path names to parse trees. */ -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator>> FileParseCache; -#else - typedef std::map FileParseCache; -#endif + using FileParseCache = GcMap; FileParseCache fileParseCache; /** * A cache from path names to values. */ -#if HAVE_BOEHMGC - typedef std::map, traceable_allocator>> FileEvalCache; -#else - typedef std::map FileEvalCache; -#endif + using FileEvalCache = GcMap; FileEvalCache fileEvalCache; SearchPath searchPath; @@ -737,15 +740,15 @@ private: bool countCalls; - typedef std::map PrimOpCalls; + using PrimOpCalls = std::map; PrimOpCalls primOpCalls; - typedef std::map FunctionCalls; + using FunctionCalls = std::map; FunctionCalls functionCalls; void incrFunctionCall(ExprLambda * fun); - typedef std::map AttrSelects; + using AttrSelects = std::map; AttrSelects attrSelects; friend struct ExprOpUpdate; @@ -787,7 +790,7 @@ std::string showType(const Value & v); */ SourcePath resolveExprPath(SourcePath path); -static const std::string corepkgsPrefix{"/__corepkgs__/"}; +static constexpr std::string_view corepkgsPrefix{"/__corepkgs__/"}; }