Merge pull request NixOS#11188 from lf-/jade/kill-int-overflow

Ban integer overflow in the Nix language

doc/manual/rl-next/ban-integer-overflow.md

@@ -0,0 +1,21 @@
+---
+synopsis: Define integer overflow in the Nix language as an error
+issues: [10968]
+prs: [11188]
+---
+
+Previously, integer overflow in the Nix language invoked C++ level signed overflow, which was undefined behaviour, but *usually* manifested as wrapping around on overflow.
+
+Since prior to the public release of Lix, Lix had C++ signed overflow defined to crash the process and nobody noticed this having accidentally removed overflow from the Nix language for three months until it was caught by fiddling around.
+Given the significant body of actual Nix code that has been evaluated by Lix in that time, it does not appear that nixpkgs or much of importance depends on integer overflow, so it appears safe to turn into an error.
+
+Some other overflows were fixed:
+- `builtins.fromJSON` of values greater than the maximum representable value in a signed 64-bit integer will generate an error.
+- `nixConfig` in flakes will no longer accept negative values for configuration options.
+
+Integer overflow now looks like the following:
+
+```
+$ nix eval --expr '9223372036854775807 + 1'
+error: integer overflow in adding 9223372036854775807 + 1
+```

doc/manual/src/language/operators.md

@@ -67,8 +67,12 @@ After evaluating *attrset* and *attrpath*, the computational complexity is O(log
 
 ## Arithmetic
 
-Numbers are type-compatible:
-Pure integer operations will always return integers, whereas any operation involving at least one floating point number return a floating point number.
+Numbers will retain their type unless mixed with other numeric types:
+Pure integer operations will always return integers, whereas any operation involving at least one floating point number returns a floating point number.
+
+Evaluation of the following numeric operations throws an evaluation error:
+- Division by zero
+- Integer overflow, that is, any operation yielding a result outside of the representable range of [Nix language integers](./syntax.md#number-literal)
 
 See also [Comparison] and [Equality].
 

doc/manual/src/language/syntax.md

@@ -15,6 +15,13 @@ See [String literals](string-literals.md).
   Numbers, which can be *integers* (like `123`) or *floating point*
   (like `123.43` or `.27e13`).
 
+  Integers in the Nix language are 64-bit [two's complement] signed integers, with a range of -9223372036854775808 to 9223372036854775807, inclusive.
+
+  [two's complement]: https://en.wikipedia.org/wiki/Two%27s_complement
+
+  Note that negative numeric literals are actually parsed as unary negation of positive numeric literals.
+  This means that the minimum integer `-9223372036854775808` cannot be written as-is as a literal, since the positive number `9223372036854775808` is one past the maximum range.
+
   See [arithmetic] and [comparison] operators for semantics.
 
   [arithmetic]: ./operators.md#arithmetic

src/libcmd/installable-flake.cc

@@ -104,12 +104,12 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
 
     auto drvPath = attr->forceDerivation();
 
-    std::optional<NixInt> priority;
+    std::optional<NixInt::Inner> priority;
 
     if (attr->maybeGetAttr(state->sOutputSpecified)) {
     } else if (auto aMeta = attr->maybeGetAttr(state->sMeta)) {
         if (auto aPriority = aMeta->maybeGetAttr("priority"))
-            priority = aPriority->getInt();
+            priority = aPriority->getInt().value;
     }
 
     return {{

src/libcmd/installable-value.hh

@@ -40,7 +40,7 @@ struct ExtraPathInfoValue : ExtraPathInfo
         /**
          * An optional priority for use with "build envs". See Package
          */
-        std::optional<NixInt> priority;
+        std::optional<NixInt::Inner> priority;
 
         /**
          * The attribute path associated with this value. The idea is

src/libexpr-c/nix_api_value.cc

@@ -306,7 +306,7 @@ int64_t nix_get_int(nix_c_context * context, const nix_value * value)
     try {
         auto & v = check_value_in(value);
         assert(v.type() == nix::nInt);
-        return v.integer();
+        return v.integer().value;
     }
     NIXC_CATCH_ERRS_RES(0);
 }

src/libexpr/eval-cache.cc

@@ -328,7 +328,7 @@ struct AttrDb
             case AttrType::Bool:
                 return {{rowId, queryAttribute.getInt(2) != 0}};
             case AttrType::Int:
-                return {{rowId, int_t{queryAttribute.getInt(2)}}};
+                return {{rowId, int_t{NixInt{queryAttribute.getInt(2)}}}};
             case AttrType::ListOfStrings:
                 return {{rowId, tokenizeString<std::vector<std::string>>(queryAttribute.getStr(2), "\t")}};
             case AttrType::Missing:
@@ -471,7 +471,7 @@ Value & AttrCursor::forceValue()
         else if (v.type() == nBool)
             cachedValue = {root->db->setBool(getKey(), v.boolean()), v.boolean()};
         else if (v.type() == nInt)
-            cachedValue = {root->db->setInt(getKey(), v.integer()), int_t{v.integer()}};
+            cachedValue = {root->db->setInt(getKey(), v.integer().value), int_t{v.integer()}};
         else if (v.type() == nAttrs)
             ; // FIXME: do something?
         else

src/libexpr/eval.cc

@@ -1979,7 +1979,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     NixStringContext context;
     std::vector<BackedStringView> s;
     size_t sSize = 0;
-    NixInt n = 0;
+    NixInt n{0};
     NixFloat nf = 0;
 
     bool first = !forceString;
@@ -2023,17 +2023,22 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
 
         if (firstType == nInt) {
             if (vTmp.type() == nInt) {
-                n += vTmp.integer();
+                auto newN = n + vTmp.integer();
+                if (auto checked = newN.valueChecked(); checked.has_value()) {
+                    n = NixInt(*checked);
+                } else {
+                    state.error<EvalError>("integer overflow in adding %1% + %2%", n, vTmp.integer()).atPos(i_pos).debugThrow();
+                }
             } else if (vTmp.type() == nFloat) {
                 // Upgrade the type from int to float;
                 firstType = nFloat;
-                nf = n;
+                nf = n.value;
                 nf += vTmp.fpoint();
             } else
                 state.error<EvalError>("cannot add %1% to an integer", showType(vTmp)).atPos(i_pos).withFrame(env, *this).debugThrow();
         } else if (firstType == nFloat) {
             if (vTmp.type() == nInt) {
-                nf += vTmp.integer();
+                nf += vTmp.integer().value;
             } else if (vTmp.type() == nFloat) {
                 nf += vTmp.fpoint();
             } else
@@ -2158,7 +2163,7 @@ NixFloat EvalState::forceFloat(Value & v, const PosIdx pos, std::string_view err
     try {
         forceValue(v, pos);
         if (v.type() == nInt)
-            return v.integer();
+            return v.integer().value;
         else if (v.type() != nFloat)
             error<TypeError>(
                 "expected a float but found %1%: %2%",
@@ -2345,7 +2350,7 @@ BackedStringView EvalState::coerceToString(
            shell scripting convenience, just like `null'. */
         if (v.type() == nBool && v.boolean()) return "1";
         if (v.type() == nBool && !v.boolean()) return "";
-        if (v.type() == nInt) return std::to_string(v.integer());
+        if (v.type() == nInt) return std::to_string(v.integer().value);
         if (v.type() == nFloat) return std::to_string(v.fpoint());
         if (v.type() == nNull) return "";
 
@@ -2728,9 +2733,9 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
 
     // Special case type-compatibility between float and int
     if (v1.type() == nInt && v2.type() == nFloat)
-        return v1.integer() == v2.fpoint();
+        return v1.integer().value == v2.fpoint();
     if (v1.type() == nFloat && v2.type() == nInt)
-        return v1.fpoint() == v2.integer();
+        return v1.fpoint() == v2.integer().value;
 
     // All other types are not compatible with each other.
     if (v1.type() != v2.type()) return false;

src/libexpr/get-drvs.cc

@@ -246,8 +246,8 @@ NixInt PackageInfo::queryMetaInt(const std::string & name, NixInt def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            integer meta fields. */
-        if (auto n = string2Int<NixInt>(v->c_str()))
-            return *n;
+        if (auto n = string2Int<NixInt::Inner>(v->c_str()))
+            return NixInt{*n};
     }
     return def;
 }

src/libexpr/json-to-value.cc

@@ -2,6 +2,7 @@
 #include "value.hh"
 #include "eval.hh"
 
+#include <limits>
 #include <variant>
 #include <nlohmann/json.hpp>
 
@@ -101,8 +102,12 @@ class JSONSax : nlohmann::json_sax<json> {
         return true;
     }
 
-    bool number_unsigned(number_unsigned_t val) override
+    bool number_unsigned(number_unsigned_t val_) override
     {
+        if (val_ > std::numeric_limits<NixInt::Inner>::max()) {
+            throw Error("unsigned json number %1% outside of Nix integer range", val_);
+        }
+        NixInt::Inner val = val_;
         rs->value(state).mkInt(val);
         rs->add();
         return true;

src/libexpr/lexer.l

@@ -138,7 +138,7 @@ or          { return OR_KW; }
 {INT}       { errno = 0;
               std::optional<int64_t> numMay = string2Int<int64_t>(yytext);
               if (numMay.has_value()) {
-                  yylval->n = *numMay;
+                  yylval->n = NixInt{*numMay};
               } else {
                   throw ParseError(ErrorInfo{
                       .msg = HintFmt("invalid integer '%1%'", yytext),

src/libexpr/nixexpr.hh

@@ -107,6 +107,7 @@ struct ExprInt : Expr
 {
     Value v;
     ExprInt(NixInt n) { v.mkInt(n); };
+    ExprInt(NixInt::Inner n) { v.mkInt(n); };
     Value * maybeThunk(EvalState & state, Env & env) override;
     COMMON_METHODS
 };