From f10465774fafaa2423cb05e02b38e03ed2abded7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 29 Aug 2021 18:09:13 +0200 Subject: [PATCH 1/2] Force all Pos* to be non-null This fixes a class of crashes and introduces ptr to make the code robust against this failure mode going forward. Thanks regnat for the idea of a ref without overhead! Closes #4895 Closes #4893 Closes #5127 Closes #5113 --- src/libexpr/attr-set.hh | 8 +++---- src/libexpr/eval.cc | 24 ++++++++++----------- src/libexpr/eval.hh | 2 +- src/libexpr/primops.cc | 4 ++-- src/libexpr/value-to-xml.cc | 2 +- src/libutil/ref.hh | 43 +++++++++++++++++++++++++++++++++++++ src/nix-env/user-env.cc | 4 ++-- 7 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/libexpr/attr-set.hh b/src/libexpr/attr-set.hh index 1da8d91df..7d6ffc9f3 100644 --- a/src/libexpr/attr-set.hh +++ b/src/libexpr/attr-set.hh @@ -17,8 +17,8 @@ struct Attr { Symbol name; Value * value; - Pos * pos; - Attr(Symbol name, Value * value, Pos * pos = &noPos) + ptr pos; + Attr(Symbol name, Value * value, ptr pos = ptr(&noPos)) : name(name), value(value), pos(pos) { }; Attr() : pos(&noPos) { }; bool operator < (const Attr & a) const @@ -35,13 +35,13 @@ class Bindings { public: typedef uint32_t size_t; - Pos *pos; + ptr pos; private: size_t size_, capacity_; Attr attrs[0]; - Bindings(size_t capacity) : size_(0), capacity_(capacity) { } + Bindings(size_t capacity) : pos(&noPos), size_(0), capacity_(capacity) { } Bindings(const Bindings & bindings) = delete; public: diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 327f7e974..361c52151 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -770,7 +770,7 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) } Bindings::iterator j = env->values[0]->attrs->find(var.name); if (j != env->values[0]->attrs->end()) { - if (countCalls && j->pos) attrSelects[*j->pos]++; + if (countCalls) attrSelects[*j->pos]++; return j->value; } if (!env->prevWith) @@ -825,9 +825,9 @@ void EvalState::mkThunk_(Value & v, Expr * expr) } -void EvalState::mkPos(Value & v, Pos * pos) +void EvalState::mkPos(Value & v, ptr pos) { - if (pos && pos->file.set()) { + if (pos->file.set()) { mkAttrs(v, 3); mkString(*allocAttr(v, sFile), pos->file); mkInt(*allocAttr(v, sLine), pos->line); @@ -1027,7 +1027,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) } else vAttr = i.second.e->maybeThunk(state, i.second.inherited ? env : env2); env2.values[displ++] = vAttr; - v.attrs->push_back(Attr(i.first, vAttr, &i.second.pos)); + v.attrs->push_back(Attr(i.first, vAttr, ptr(&i.second.pos))); } /* If the rec contains an attribute called `__overrides', then @@ -1059,7 +1059,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) else for (auto & i : attrs) - v.attrs->push_back(Attr(i.first, i.second.e->maybeThunk(state, env), &i.second.pos)); + v.attrs->push_back(Attr(i.first, i.second.e->maybeThunk(state, env), ptr(&i.second.pos))); /* Dynamic attrs apply *after* rec and __overrides. */ for (auto & i : dynamicAttrs) { @@ -1076,11 +1076,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) i.valueExpr->setName(nameSym); /* Keep sorted order so find can catch duplicates */ - v.attrs->push_back(Attr(nameSym, i.valueExpr->maybeThunk(state, *dynamicEnv), &i.pos)); + v.attrs->push_back(Attr(nameSym, i.valueExpr->maybeThunk(state, *dynamicEnv), ptr(&i.pos))); v.attrs->sort(); // FIXME: inefficient } - v.attrs->pos = &pos; + v.attrs->pos = ptr(&pos); } @@ -1138,7 +1138,7 @@ static string showAttrPath(EvalState & state, Env & env, const AttrPath & attrPa void ExprSelect::eval(EvalState & state, Env & env, Value & v) { Value vTmp; - Pos * pos2 = 0; + ptr pos2(&noPos); Value * vAttrs = &vTmp; e->eval(state, env, vTmp); @@ -1164,13 +1164,13 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) } vAttrs = j->value; pos2 = j->pos; - if (state.countCalls && pos2) state.attrSelects[*pos2]++; + if (state.countCalls) state.attrSelects[*pos2]++; } - state.forceValue(*vAttrs, ( pos2 != NULL ? *pos2 : this->pos ) ); + state.forceValue(*vAttrs, (*pos2 != noPos ? *pos2 : this->pos ) ); } catch (Error & e) { - if (pos2 && pos2->file != state.sDerivationNix) + if (*pos2 != noPos && pos2->file != state.sDerivationNix) addErrorTrace(e, *pos2, "while evaluating the attribute '%1%'", showAttrPath(state, env, attrPath)); throw; @@ -1616,7 +1616,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) void ExprPos::eval(EvalState & state, Env & env, Value & v) { - state.mkPos(v, &pos); + state.mkPos(v, ptr(&pos)); } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 6f3474854..f3684db79 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -308,7 +308,7 @@ public: void mkList(Value & v, size_t length); void mkAttrs(Value & v, size_t capacity); void mkThunk_(Value & v, Expr * expr); - void mkPos(Value & v, Pos * pos); + void mkPos(Value & v, ptr pos); void concatLists(Value & v, size_t nrLists, Value * * lists, const Pos & pos); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index bfe41c9fa..2295e69a1 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2109,7 +2109,7 @@ void prim_getAttr(EvalState & state, const Pos & pos, Value * * args, Value & v) pos ); // !!! add to stack trace? - if (state.countCalls && i->pos) state.attrSelects[*i->pos]++; + if (state.countCalls && *i->pos != noPos) state.attrSelects[*i->pos]++; state.forceValue(*i->value, pos); v = *i->value; } @@ -2369,7 +2369,7 @@ static void prim_functionArgs(EvalState & state, const Pos & pos, Value * * args for (auto & i : args[0]->lambda.fun->formals->formals) { // !!! should optimise booleans (allocate only once) Value * value = state.allocValue(); - v.attrs->push_back(Attr(i.name, value, &i.pos)); + v.attrs->push_back(Attr(i.name, value, ptr(&i.pos))); mkBool(*value, i.def); } v.attrs->sort(); diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index 7464455d8..2ddc5f751 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -42,7 +42,7 @@ static void showAttrs(EvalState & state, bool strict, bool location, XMLAttrs xmlAttrs; xmlAttrs["name"] = i; - if (location && a.pos != &noPos) posToXML(xmlAttrs, *a.pos); + if (location && a.pos != ptr(&noPos)) posToXML(xmlAttrs, *a.pos); XMLOpenElement _(doc, "attr", xmlAttrs); printValueAsXML(state, strict, location, diff --git a/src/libutil/ref.hh b/src/libutil/ref.hh index 2549ef496..d6bf53bb8 100644 --- a/src/libutil/ref.hh +++ b/src/libutil/ref.hh @@ -99,4 +99,47 @@ make_ref(Args&&... args) return ref(p); } + +/* A non-nullable pointer. + This is similar to a C++ "& reference", but mutable. + This is similar to ref but backed by a regular pointer instead of a smart pointer. + */ +template +class ptr { +private: + T * p; + +public: + ptr(const ptr & r) + : p(r.p) + { } + + explicit ptr(T * p) + : p(p) + { + if (!p) + throw std::invalid_argument("null pointer cast to ptr"); + } + + T* operator ->() const + { + return &*p; + } + + T& operator *() const + { + return *p; + } + + bool operator == (const ptr & other) const + { + return p == other.p; + } + + bool operator != (const ptr & other) const + { + return p != other.p; + } +}; + } diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index 5ceb2ae67..4b574fe0a 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -131,9 +131,9 @@ bool createUserEnv(EvalState & state, DrvInfos & elems, state.forceValue(topLevel); PathSet context; Attr & aDrvPath(*topLevel.attrs->find(state.sDrvPath)); - auto topLevelDrv = state.store->parseStorePath(state.coerceToPath(aDrvPath.pos ? *(aDrvPath.pos) : noPos, *(aDrvPath.value), context)); + auto topLevelDrv = state.store->parseStorePath(state.coerceToPath(*aDrvPath.pos, *(aDrvPath.value), context)); Attr & aOutPath(*topLevel.attrs->find(state.sOutPath)); - Path topLevelOut = state.coerceToPath(aOutPath.pos ? *(aOutPath.pos) : noPos, *(aOutPath.value), context); + Path topLevelOut = state.coerceToPath(*aOutPath.pos, *(aOutPath.value), context); /* Realise the resulting store expression. */ debug("building user environment"); From 92778a5f80d6f5c17e4ad09f7cbf0839341f2398 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 30 Aug 2021 09:52:26 +0200 Subject: [PATCH 2/2] Tidy --- src/nix-env/user-env.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index 4b574fe0a..1fd4bcbd3 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -131,9 +131,9 @@ bool createUserEnv(EvalState & state, DrvInfos & elems, state.forceValue(topLevel); PathSet context; Attr & aDrvPath(*topLevel.attrs->find(state.sDrvPath)); - auto topLevelDrv = state.store->parseStorePath(state.coerceToPath(*aDrvPath.pos, *(aDrvPath.value), context)); + auto topLevelDrv = state.store->parseStorePath(state.coerceToPath(*aDrvPath.pos, *aDrvPath.value, context)); Attr & aOutPath(*topLevel.attrs->find(state.sOutPath)); - Path topLevelOut = state.coerceToPath(*aOutPath.pos, *(aOutPath.value), context); + Path topLevelOut = state.coerceToPath(*aOutPath.pos, *aOutPath.value, context); /* Realise the resulting store expression. */ debug("building user environment");