Force all Pos* to be non-null

This fixes a class of crashes and introduces ptr<T> to make the
code robust against this failure mode going forward.

Thanks regnat for the idea of a ref<T> without overhead!

Closes #4895
Closes #4893
Closes #5127
Closes #5113

src/libexpr/attr-set.hh

@@ -17,8 +17,8 @@ struct Attr
 {
     Symbol name;
     Value * value;
-    Pos * pos;
+    ptr<Pos> pos;
-    Attr(Symbol name, Value * value, Pos * pos = &noPos)
+    Attr(Symbol name, Value * value, ptr<Pos> 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> 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:

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> 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<Pos> 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));
 }
 
 

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> pos);
 
     void concatLists(Value & v, size_t nrLists, Value * * lists, const Pos & pos);
 

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();

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,

src/libutil/ref.hh

@@ -99,4 +99,47 @@ make_ref(Args&&... args)
     return ref<T>(p);
 }
 
+
+/* A non-nullable pointer.
+   This is similar to a C++ "& reference", but mutable.
+   This is similar to ref<T> but backed by a regular pointer instead of a smart pointer.
+ */
+template<typename T>
+class ptr {
+private:
+    T * p;
+
+public:
+    ptr<T>(const ptr<T> & r)
+        : p(r.p)
+    { }
+
+    explicit ptr<T>(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<T> & other) const
+    {
+        return p == other.p;
+    }
+
+    bool operator != (const ptr<T> & other) const
+    {
+        return p != other.p;
+    }
+};
+
 }

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");