From dd75711895057328b00f92353730e7f873eafd52 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 12 Jul 2024 14:56:30 +0200 Subject: [PATCH 1/6] Use std::strong_ordering for version comparison The actual motive here is the avoidance of integer overflow if we were to make these use checked NixInts and retain the subtraction. However, the actual *intent* of this code is a three-way comparison, which can be done with operator<=>, so we should just do *that* instead. Change-Id: I7f9a7da1f3176424b528af6d1b4f1591e4ab26bf --- src/libexpr/primops.cc | 3 ++- src/libstore/names.cc | 8 ++++---- src/libstore/names.hh | 2 +- src/nix-env/nix-env.cc | 22 +++++++++++----------- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0b3b19b57..a23583f34 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4446,7 +4446,8 @@ static void prim_compareVersions(EvalState & state, const PosIdx pos, Value * * { auto version1 = state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.compareVersions"); auto version2 = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.compareVersions"); - v.mkInt(compareVersions(version1, version2)); + auto result = compareVersions(version1, version2); + v.mkInt(result < 0 ? -1 : result > 0 ? 1 : 0); } static RegisterPrimOp primop_compareVersions({ diff --git a/src/libstore/names.cc b/src/libstore/names.cc index 277aabf0f..c0e1b1022 100644 --- a/src/libstore/names.cc +++ b/src/libstore/names.cc @@ -94,7 +94,7 @@ static bool componentsLT(const std::string_view c1, const std::string_view c2) } -int compareVersions(const std::string_view v1, const std::string_view v2) +std::strong_ordering compareVersions(const std::string_view v1, const std::string_view v2) { auto p1 = v1.begin(); auto p2 = v2.begin(); @@ -102,11 +102,11 @@ int compareVersions(const std::string_view v1, const std::string_view v2) while (p1 != v1.end() || p2 != v2.end()) { auto c1 = nextComponent(p1, v1.end()); auto c2 = nextComponent(p2, v2.end()); - if (componentsLT(c1, c2)) return -1; - else if (componentsLT(c2, c1)) return 1; + if (componentsLT(c1, c2)) return std::strong_ordering::less; + else if (componentsLT(c2, c1)) return std::strong_ordering::greater; } - return 0; + return std::strong_ordering::equal; } diff --git a/src/libstore/names.hh b/src/libstore/names.hh index d82b99bb4..a6909d545 100644 --- a/src/libstore/names.hh +++ b/src/libstore/names.hh @@ -30,7 +30,7 @@ typedef std::list DrvNames; std::string_view nextComponent(std::string_view::const_iterator & p, const std::string_view::const_iterator end); -int compareVersions(const std::string_view v1, const std::string_view v2); +std::strong_ordering compareVersions(const std::string_view v1, const std::string_view v2); DrvNames drvNamesFromArgs(const Strings & opArgs); } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 40c200542..ba2baccee 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -204,15 +204,15 @@ static void loadDerivations(EvalState & state, const SourcePath & nixExprPath, } -static long getPriority(EvalState & state, PackageInfo & drv) +static NixInt getPriority(EvalState & state, PackageInfo & drv) { - return drv.queryMetaInt("priority", 0); + return drv.queryMetaInt("priority", NixInt(0)); } -static long comparePriorities(EvalState & state, PackageInfo & drv1, PackageInfo & drv2) +static std::strong_ordering comparePriorities(EvalState & state, PackageInfo & drv1, PackageInfo & drv2) { - return getPriority(state, drv2) - getPriority(state, drv1); + return getPriority(state, drv2) <=> getPriority(state, drv1); } @@ -280,7 +280,7 @@ std::vector pickNewestOnly(EvalState & state, std::vector matches) auto & oneDrv = match.packageInfo; const auto drvName = DrvName { oneDrv.queryName() }; - long comparison = 1; + std::strong_ordering comparison = std::strong_ordering::greater; const auto itOther = newest.find(drvName.name); @@ -288,9 +288,9 @@ std::vector pickNewestOnly(EvalState & state, std::vector matches) auto & newestDrv = itOther->second.packageInfo; comparison = - oneDrv.querySystem() == newestDrv.querySystem() ? 0 : - oneDrv.querySystem() == settings.thisSystem ? 1 : - newestDrv.querySystem() == settings.thisSystem ? -1 : 0; + oneDrv.querySystem() == newestDrv.querySystem() ? std::strong_ordering::equal : + oneDrv.querySystem() == settings.thisSystem ? std::strong_ordering::greater : + newestDrv.querySystem() == settings.thisSystem ? std::strong_ordering::less : std::strong_ordering::equal; if (comparison == 0) comparison = comparePriorities(state, oneDrv, newestDrv); if (comparison == 0) @@ -625,13 +625,13 @@ static void upgradeDerivations(Globals & globals, continue; DrvName newName(j->queryName()); if (newName.name == drvName.name) { - int d = compareVersions(drvName.version, newName.version); + std::strong_ordering d = compareVersions(drvName.version, newName.version); if ((upgradeType == utLt && d < 0) || (upgradeType == utLeq && d <= 0) || (upgradeType == utEq && d == 0) || upgradeType == utAlways) { - long d2 = -1; + std::strong_ordering d2 = std::strong_ordering::less; if (bestElem != availElems.end()) { d2 = comparePriorities(*globals.state, *bestElem, *j); if (d2 == 0) d2 = compareVersions(bestVersion, newName.version); @@ -902,7 +902,7 @@ static VersionDiff compareVersionAgainstSet( for (auto & i : elems) { DrvName name2(i.queryName()); if (name.name == name2.name) { - int d = compareVersions(name.version, name2.version); + std::strong_ordering d = compareVersions(name.version, name2.version); if (d < 0) { diff = cvGreater; version = name2.version; From e28cb67d41643bf2073e46149e6f5d10d1fd0ef5 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 5 Jul 2024 15:52:55 +0200 Subject: [PATCH 2/6] libutil: add checked arithmetic tools This is in preparation for adding checked arithmetic to the evaluator. Change-Id: I6e115ce8f5411feda1706624977a4dcd5efd4d13 --- src/libutil/checked-arithmetic.hh | 182 ++++++++++++++++++ src/libutil/meson.build | 1 + .../tests/gtest-with-params.hh | 54 ++++++ tests/unit/libutil/checked-arithmetic.cc | 158 +++++++++++++++ 4 files changed, 395 insertions(+) create mode 100644 src/libutil/checked-arithmetic.hh create mode 100644 tests/unit/libutil-support/tests/gtest-with-params.hh create mode 100644 tests/unit/libutil/checked-arithmetic.cc diff --git a/src/libutil/checked-arithmetic.hh b/src/libutil/checked-arithmetic.hh new file mode 100644 index 000000000..55d6ad205 --- /dev/null +++ b/src/libutil/checked-arithmetic.hh @@ -0,0 +1,182 @@ +#pragma once +/** + * @file Checked arithmetic with classes that make it hard to accidentally make something an unchecked operation. + */ + +#include +#include // IWYU pragma: keep +#include +#include +#include +#include +#include + +namespace nix::checked { + +class DivideByZero : std::exception +{}; + +/** + * Numeric value enforcing checked arithmetic. Performing mathematical operations on such values will return a Result + * type which needs to be checked. + */ +template +struct Checked +{ + using Inner = T; + + // TODO: this must be a "trivial default constructor", which means it + // cannot set the value to NOT DO UB on uninit. + T value; + + Checked() = default; + explicit Checked(T const value) + : value{value} + { + } + Checked(Checked const & other) = default; + Checked(Checked && other) = default; + Checked & operator=(Checked const & other) = default; + + std::strong_ordering operator<=>(Checked const & other) const = default; + std::strong_ordering operator<=>(T const & other) const + { + return value <=> other; + } + + explicit operator T() const + { + return value; + } + + enum class OverflowKind { + NoOverflow, + Overflow, + DivByZero, + }; + + class Result + { + T value; + OverflowKind overflowed_; + + public: + Result(T value, bool overflowed) + : value{value} + , overflowed_{overflowed ? OverflowKind::Overflow : OverflowKind::NoOverflow} + { + } + Result(T value, OverflowKind overflowed) + : value{value} + , overflowed_{overflowed} + { + } + + bool operator==(Result other) const + { + return value == other.value && overflowed_ == other.overflowed_; + } + + std::optional valueChecked() const + { + if (overflowed_ != OverflowKind::NoOverflow) { + return std::nullopt; + } else { + return value; + } + } + + /** + * Returns the result as if the arithmetic were performed as wrapping arithmetic. + * + * \throws DivideByZero if the operation was a divide by zero. + */ + T valueWrapping() const + { + if (overflowed_ == OverflowKind::DivByZero) { + throw DivideByZero{}; + } + return value; + } + + bool overflowed() const + { + return overflowed_ == OverflowKind::Overflow; + } + + bool divideByZero() const + { + return overflowed_ == OverflowKind::DivByZero; + } + }; + + Result operator+(Checked const other) const + { + return (*this) + other.value; + } + Result operator+(T const other) const + { + T result; + bool overflowed = __builtin_add_overflow(value, other, &result); + return Result{result, overflowed}; + } + + Result operator-(Checked const other) const + { + return (*this) - other.value; + } + Result operator-(T const other) const + { + T result; + bool overflowed = __builtin_sub_overflow(value, other, &result); + return Result{result, overflowed}; + } + + Result operator*(Checked const other) const + { + return (*this) * other.value; + } + Result operator*(T const other) const + { + T result; + bool overflowed = __builtin_mul_overflow(value, other, &result); + return Result{result, overflowed}; + } + + Result operator/(Checked const other) const + { + return (*this) / other.value; + } + /** + * Performs a checked division. + * + * If the right hand side is zero, the result is marked as a DivByZero and + * valueWrapping will throw. + */ + Result operator/(T const other) const + { + constexpr T const minV = std::numeric_limits::min(); + + // It's only possible to overflow with signed division since doing so + // requires crossing the two's complement limits by MIN / -1 (since + // two's complement has one more in range in the negative direction + // than in the positive one). + if (std::is_signed() && (value == minV && other == -1)) { + return Result{minV, true}; + } else if (other == 0) { + return Result{0, OverflowKind::DivByZero}; + } else { + T result = value / other; + return Result{result, false}; + } + } +}; + +template +std::ostream & operator<<(std::ostream & ios, Checked v) +{ + ios << v.value; + return ios; +} + +} diff --git a/src/libutil/meson.build b/src/libutil/meson.build index 8552c4c9d..e0299177f 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -171,6 +171,7 @@ headers = [config_h] + files( 'args/root.hh', 'callback.hh', 'canon-path.hh', + 'checked-arithmetic.hh', 'chunked-vector.hh', 'closure.hh', 'comparator.hh', diff --git a/tests/unit/libutil-support/tests/gtest-with-params.hh b/tests/unit/libutil-support/tests/gtest-with-params.hh new file mode 100644 index 000000000..323a083fe --- /dev/null +++ b/tests/unit/libutil-support/tests/gtest-with-params.hh @@ -0,0 +1,54 @@ +#pragma once +// SPDX-FileCopyrightText: 2014 Emil Eriksson +// +// SPDX-License-Identifier: BSD-2-Clause +// +// The lion's share of this code is copy pasted directly out of RapidCheck +// headers, so the copyright is set accordingly. +/** + * @file Implements the ability to run a RapidCheck test under gtest with changed + * test parameters such as the number of tests to run. This is useful for + * running very large numbers of the extremely cheap property tests. + */ + +#include +#include +#include + +namespace rc::detail { + +using MakeTestParams = TestParams (*)(); + +template +void checkGTestWith(Testable && testable, MakeTestParams makeTestParams) +{ + const auto testInfo = ::testing::UnitTest::GetInstance()->current_test_info(); + detail::TestMetadata metadata; + metadata.id = std::string(testInfo->test_case_name()) + "/" + std::string(testInfo->name()); + metadata.description = std::string(testInfo->name()); + + const auto result = checkTestable(std::forward(testable), metadata, makeTestParams()); + + if (result.template is()) { + const auto success = result.template get(); + if (!success.distribution.empty()) { + printResultMessage(result, std::cout); + std::cout << std::endl; + } + } else { + std::ostringstream ss; + printResultMessage(result, ss); + FAIL() << ss.str() << std::endl; + } +} +} + +#define RC_GTEST_PROP_WITH_PARAMS(TestCase, Name, MakeParams, ArgList) \ + void rapidCheck_propImpl_##TestCase##_##Name ArgList; \ + \ + TEST(TestCase, Name) \ + { \ + ::rc::detail::checkGTestWith(&rapidCheck_propImpl_##TestCase##_##Name, MakeParams); \ + } \ + \ + void rapidCheck_propImpl_##TestCase##_##Name ArgList diff --git a/tests/unit/libutil/checked-arithmetic.cc b/tests/unit/libutil/checked-arithmetic.cc new file mode 100644 index 000000000..75018660d --- /dev/null +++ b/tests/unit/libutil/checked-arithmetic.cc @@ -0,0 +1,158 @@ +#include +#include +#include +#include +#include +#include + +#include + +#include "tests/gtest-with-params.hh" + +namespace rc { +using namespace nix; + +template +struct Arbitrary> +{ + static Gen> arbitrary() + { + return gen::arbitrary(); + } +}; + +} + +namespace nix::checked { + +// Pointer to member function! Mildly gross. +template +using Oper = Checked::Result (Checked::*)(T const other) const; + +template +using ReferenceOper = T (*)(T a, T b); + +/** + * Checks that performing an operation that overflows into an inaccurate result + * has the desired behaviour. + * + * TBig is a type large enough to represent all results of TSmall operations. + */ +template +void checkType(TSmall a_, TSmall b, Oper oper, ReferenceOper reference) +{ + // Sufficient to fit all values + TBig referenceResult = reference(a_, b); + constexpr const TSmall minV = std::numeric_limits::min(); + constexpr const TSmall maxV = std::numeric_limits::max(); + + Checked a{a_}; + auto result = (a.*(oper))(b); + + // Just truncate it to get the in-range result + RC_ASSERT(result.valueWrapping() == static_cast(referenceResult)); + + if (referenceResult > maxV || referenceResult < minV) { + RC_ASSERT(result.overflowed()); + RC_ASSERT(!result.valueChecked().has_value()); + } else { + RC_ASSERT(!result.overflowed()); + RC_ASSERT(result.valueChecked().has_value()); + RC_ASSERT(*result.valueChecked() == referenceResult); + } +} + +/** + * Checks that performing an operation that overflows into an inaccurate result + * has the desired behaviour. + * + * TBig is a type large enough to represent all results of TSmall operations. + */ +template +void checkDivision(TSmall a_, TSmall b) +{ + // Sufficient to fit all values + constexpr const TSmall minV = std::numeric_limits::min(); + + Checked a{a_}; + auto result = a / b; + + if (std::is_signed() && a_ == minV && b == -1) { + // This is the only possible overflow condition + RC_ASSERT(result.valueWrapping() == minV); + RC_ASSERT(result.overflowed()); + } else if (b == 0) { + RC_ASSERT(result.divideByZero()); + RC_ASSERT_THROWS_AS(result.valueWrapping(), nix::checked::DivideByZero); + RC_ASSERT(result.valueChecked() == std::nullopt); + } else { + TBig referenceResult = a_ / b; + auto result_ = result.valueChecked(); + RC_ASSERT(result_.has_value()); + RC_ASSERT(*result_ == referenceResult); + RC_ASSERT(result.valueWrapping() == referenceResult); + } +} + +/** Creates parameters that perform a more adequate number of checks to validate + * extremely cheap tests such as arithmetic tests */ +static rc::detail::TestParams makeParams() +{ + auto const & conf = rc::detail::configuration(); + auto newParams = conf.testParams; + newParams.maxSuccess = 10000; + return newParams; +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, add_unsigned, makeParams, (uint16_t a, uint16_t b)) +{ + checkType(a, b, &Checked::operator+, [](int32_t a, int32_t b) { return a + b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, add_signed, makeParams, (int16_t a, int16_t b)) +{ + checkType(a, b, &Checked::operator+, [](int32_t a, int32_t b) { return a + b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, sub_unsigned, makeParams, (uint16_t a, uint16_t b)) +{ + checkType(a, b, &Checked::operator-, [](int32_t a, int32_t b) { return a - b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, sub_signed, makeParams, (int16_t a, int16_t b)) +{ + checkType(a, b, &Checked::operator-, [](int32_t a, int32_t b) { return a - b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, mul_unsigned, makeParams, (uint16_t a, uint16_t b)) +{ + checkType(a, b, &Checked::operator*, [](int64_t a, int64_t b) { return a * b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, mul_signed, makeParams, (int16_t a, int16_t b)) +{ + checkType(a, b, &Checked::operator*, [](int64_t a, int64_t b) { return a * b; }); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, div_unsigned, makeParams, (uint16_t a, uint16_t b)) +{ + checkDivision(a, b); +} + +RC_GTEST_PROP_WITH_PARAMS(Checked, div_signed, makeParams, (int16_t a, int16_t b)) +{ + checkDivision(a, b); +} + +// Make absolutely sure that we check the special cases if the proptest +// generator does not come up with them. This one is especially important +// because it has very specific pairs required for the edge cases unlike the +// others. +TEST(Checked, div_signed_special_cases) +{ + checkDivision(std::numeric_limits::min(), -1); + checkDivision(std::numeric_limits::min(), 0); + checkDivision(0, 0); +} + +} From 7b6622d73317f0cd093cde96b5898b8af07abea7 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 12 Jul 2024 16:22:34 +0200 Subject: [PATCH 3/6] language: cleanly ban integer overflows This also bans various sneaking of negative numbers from the language into unsuspecting builtins as was exposed while auditing the consequences of changing the Nix language integer type to a newtype. It's unlikely that this change comprehensively ensures correctness when passing integers out of the Nix language and we should probably add a checked-narrowing function or something similar, but that's out of scope for the immediate change. During the development of this I found a few fun facts about the language: - You could overflow integers by converting from unsigned JSON values. - You could overflow unsigned integers by converting negative numbers into them when going into Nix config, into fetchTree, and into flake inputs. The flake inputs and Nix config cannot actually be tested properly since they both ban thunks, however, we put in checks anyway because it's possible these could somehow be used to do such shenanigans some other way. Note that Lix has banned Nix language integer overflows since the very first public beta, but threw a SIGILL about them because we run with -fsanitize=signed-overflow -fsanitize-undefined-trap-on-error in production builds. Since the Nix language uses signed integers, overflow was simply undefined behaviour, and since we defined that to trap, it did. Trapping on it was a bad UX, but we didn't even entirely notice that we had done this at all until it was reported as a bug a couple of months later (which is, to be fair, that flag working as intended), and it's got enough production time that, aside from code that is IMHO buggy (and which is, in any case, not in nixpkgs) such as https://git.lix.systems/lix-project/lix/issues/445, we don't think anyone doing anything reasonable actually depends on wrapping overflow. Even for weird use cases such as doing funny bit crimes, it doesn't make sense IMO to have wrapping behaviour, since two's complement arithmetic overflow behaviour is so *aggressively* not what you want for *any* kind of mathematics/algorithms. The Nix language exists for package management, a domain where bit crimes are already only dubiously in scope to begin with, and it makes a lot more sense for that domain for the integers to never lose precision, either by throwing errors if they would, or by being arbitrary-precision. Fixes: https://github.com/NixOS/nix/issues/10968 Original-CL: https://gerrit.lix.systems/c/lix/+/1596 Change-Id: I51f253840c4af2ea5422b8a420aa5fafbf8fae75 --- src/libcmd/installable-flake.cc | 4 +- src/libcmd/installable-value.hh | 2 +- src/libexpr-c/nix_api_value.cc | 2 +- src/libexpr/eval-cache.cc | 4 +- src/libexpr/eval.cc | 21 ++-- src/libexpr/get-drvs.cc | 4 +- src/libexpr/json-to-value.cc | 7 +- src/libexpr/lexer.l | 2 +- src/libexpr/nixexpr.hh | 1 + src/libexpr/primops.cc | 102 ++++++++++++------ src/libexpr/primops/fetchTree.cc | 12 ++- src/libexpr/value-to-json.cc | 2 +- src/libexpr/value.hh | 10 +- src/libflake/flake/flake.cc | 22 +++- src/libstore/globals.hh | 10 +- .../lang/eval-fail-fetchTree-negative.err.exp | 8 ++ .../lang/eval-fail-fetchTree-negative.nix | 5 + ...ake-ref-to-string-negative-integer.err.exp | 14 +++ ...l-flake-ref-to-string-negative-integer.nix | 7 ++ .../eval-fail-fromJSON-overflowing.err.exp | 8 ++ .../lang/eval-fail-fromJSON-overflowing.nix | 1 + .../lang/eval-fail-overflowing-add.err.exp | 6 ++ .../lang/eval-fail-overflowing-add.nix | 4 + .../lang/eval-fail-overflowing-div.err.exp | 23 ++++ .../lang/eval-fail-overflowing-div.nix | 7 ++ .../lang/eval-fail-overflowing-mul.err.exp | 16 +++ .../lang/eval-fail-overflowing-mul.nix | 3 + .../lang/eval-fail-overflowing-sub.err.exp | 9 ++ .../lang/eval-fail-overflowing-sub.nix | 4 + tests/unit/libexpr-support/tests/libexpr.hh | 2 +- 30 files changed, 260 insertions(+), 62 deletions(-) create mode 100644 tests/functional/lang/eval-fail-fetchTree-negative.err.exp create mode 100644 tests/functional/lang/eval-fail-fetchTree-negative.nix create mode 100644 tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.err.exp create mode 100644 tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.nix create mode 100644 tests/functional/lang/eval-fail-fromJSON-overflowing.err.exp create mode 100644 tests/functional/lang/eval-fail-fromJSON-overflowing.nix create mode 100644 tests/functional/lang/eval-fail-overflowing-add.err.exp create mode 100644 tests/functional/lang/eval-fail-overflowing-add.nix create mode 100644 tests/functional/lang/eval-fail-overflowing-div.err.exp create mode 100644 tests/functional/lang/eval-fail-overflowing-div.nix create mode 100644 tests/functional/lang/eval-fail-overflowing-mul.err.exp create mode 100644 tests/functional/lang/eval-fail-overflowing-mul.nix create mode 100644 tests/functional/lang/eval-fail-overflowing-sub.err.exp create mode 100644 tests/functional/lang/eval-fail-overflowing-sub.nix diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 8796ad5ba..6c9ee6748 100644 --- a/src/libcmd/installable-flake.cc +++ b/src/libcmd/installable-flake.cc @@ -104,12 +104,12 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() auto drvPath = attr->forceDerivation(); - std::optional priority; + std::optional 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 {{ diff --git a/src/libcmd/installable-value.hh b/src/libcmd/installable-value.hh index 798cb5e1a..60207cd23 100644 --- a/src/libcmd/installable-value.hh +++ b/src/libcmd/installable-value.hh @@ -40,7 +40,7 @@ struct ExtraPathInfoValue : ExtraPathInfo /** * An optional priority for use with "build envs". See Package */ - std::optional priority; + std::optional priority; /** * The attribute path associated with this value. The idea is diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 845e87935..fa2a9cbe2 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/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); } diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 26352187e..95b484606 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -326,7 +326,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>(queryAttribute.getStr(2), "\t")}}; case AttrType::Missing: @@ -469,7 +469,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 diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 746ccab2a..76c54936f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1979,7 +1979,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) NixStringContext context; std::vector 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("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("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( "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; diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 7041a3932..20963ec91 100644 --- a/src/libexpr/get-drvs.cc +++ b/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(v->c_str())) - return *n; + if (auto n = string2Int(v->c_str())) + return NixInt{*n}; } return def; } diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index 21074bdd8..9ac56541a 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -2,6 +2,7 @@ #include "value.hh" #include "eval.hh" +#include #include #include @@ -101,8 +102,12 @@ public: return true; } - bool number_unsigned(number_unsigned_t val) override + bool number_unsigned(number_unsigned_t val_) override { + if (val_ > std::numeric_limits::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; diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index eb1825b7c..a7e44cb72 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -138,7 +138,7 @@ or { return OR_KW; } {INT} { errno = 0; std::optional numMay = string2Int(yytext); if (numMay.has_value()) { - yylval->n = *numMay; + yylval->n = NixInt{*numMay}; } else { throw ParseError(ErrorInfo{ .msg = HintFmt("invalid integer '%1%'", yytext), diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 3279e3d48..7868834f1 100644 --- a/src/libexpr/nixexpr.hh +++ b/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 }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index a23583f34..acc0d6c1c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -587,9 +587,9 @@ struct CompareValues { try { if (v1->type() == nFloat && v2->type() == nInt) - return v1->fpoint() < v2->integer(); + return v1->fpoint() < v2->integer().value; if (v1->type() == nInt && v2->type() == nFloat) - return v1->integer() < v2->fpoint(); + return v1->integer().value < v2->fpoint(); if (v1->type() != v2->type()) state.error("cannot compare %s with %s", showType(*v1), showType(*v2)).debugThrow(); // Allow selecting a subset of enum values @@ -2677,13 +2677,13 @@ static struct LazyPosAcessors { PrimOp primop_lineOfPos{ .arity = 1, .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) { - v.mkInt(state.positions[PosIdx(args[0]->integer())].line); + v.mkInt(state.positions[PosIdx(args[0]->integer().value)].line); } }; PrimOp primop_columnOfPos{ .arity = 1, .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) { - v.mkInt(state.positions[PosIdx(args[0]->integer())].column); + v.mkInt(state.positions[PosIdx(args[0]->integer().value)].column); } }; @@ -3159,7 +3159,8 @@ static void elemAt(EvalState & state, const PosIdx pos, Value & list, int n, Val /* Return the n-1'th element of a list. */ static void prim_elemAt(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - elemAt(state, pos, *args[0], state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.elemAt"), v); + NixInt::Inner elem = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.elemAt").value; + elemAt(state, pos, *args[0], elem, v); } static RegisterPrimOp primop_elemAt({ @@ -3453,10 +3454,12 @@ static RegisterPrimOp primop_all({ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - auto len = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.genList"); + auto len_ = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.genList").value; - if (len < 0) - state.error("cannot create list of size %1%", len).atPos(pos).debugThrow(); + if (len_ < 0) + state.error("cannot create list of size %1%", len_).atPos(pos).debugThrow(); + + size_t len = size_t(len_); // More strict than striclty (!) necessary, but acceptable // as evaluating map without accessing any values makes little sense. @@ -3713,9 +3716,17 @@ static void prim_add(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->type() == nFloat || args[1]->type() == nFloat) v.mkFloat(state.forceFloat(*args[0], pos, "while evaluating the first argument of the addition") + state.forceFloat(*args[1], pos, "while evaluating the second argument of the addition")); - else - v.mkInt( state.forceInt(*args[0], pos, "while evaluating the first argument of the addition") - + state.forceInt(*args[1], pos, "while evaluating the second argument of the addition")); + else { + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument of the addition"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument of the addition"); + + auto result_ = i1 + i2; + if (auto result = result_.valueChecked(); result.has_value()) { + v.mkInt(*result); + } else { + state.error("integer overflow in adding %1% + %2%", i1, i2).atPos(pos).debugThrow(); + } + } } static RegisterPrimOp primop_add({ @@ -3734,9 +3745,18 @@ static void prim_sub(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->type() == nFloat || args[1]->type() == nFloat) v.mkFloat(state.forceFloat(*args[0], pos, "while evaluating the first argument of the subtraction") - state.forceFloat(*args[1], pos, "while evaluating the second argument of the subtraction")); - else - v.mkInt( state.forceInt(*args[0], pos, "while evaluating the first argument of the subtraction") - - state.forceInt(*args[1], pos, "while evaluating the second argument of the subtraction")); + else { + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument of the subtraction"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument of the subtraction"); + + auto result_ = i1 - i2; + + if (auto result = result_.valueChecked(); result.has_value()) { + v.mkInt(*result); + } else { + state.error("integer overflow in subtracting %1% - %2%", i1, i2).atPos(pos).debugThrow(); + } + } } static RegisterPrimOp primop_sub({ @@ -3755,9 +3775,18 @@ static void prim_mul(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->type() == nFloat || args[1]->type() == nFloat) v.mkFloat(state.forceFloat(*args[0], pos, "while evaluating the first of the multiplication") * state.forceFloat(*args[1], pos, "while evaluating the second argument of the multiplication")); - else - v.mkInt( state.forceInt(*args[0], pos, "while evaluating the first argument of the multiplication") - * state.forceInt(*args[1], pos, "while evaluating the second argument of the multiplication")); + else { + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument of the multiplication"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument of the multiplication"); + + auto result_ = i1 * i2; + + if (auto result = result_.valueChecked(); result.has_value()) { + v.mkInt(*result); + } else { + state.error("integer overflow in multiplying %1% * %2%", i1, i2).atPos(pos).debugThrow(); + } + } } static RegisterPrimOp primop_mul({ @@ -3784,10 +3813,12 @@ static void prim_div(EvalState & state, const PosIdx pos, Value * * args, Value NixInt i1 = state.forceInt(*args[0], pos, "while evaluating the first operand of the division"); NixInt i2 = state.forceInt(*args[1], pos, "while evaluating the second operand of the division"); /* Avoid division overflow as it might raise SIGFPE. */ - if (i1 == std::numeric_limits::min() && i2 == -1) - state.error("overflow in integer division").atPos(pos).debugThrow(); - - v.mkInt(i1 / i2); + auto result_ = i1 / i2; + if (auto result = result_.valueChecked(); result.has_value()) { + v.mkInt(*result); + } else { + state.error("integer overflow in dividing %1% / %2%", i1, i2).atPos(pos).debugThrow(); + } } } @@ -3802,8 +3833,9 @@ static RegisterPrimOp primop_div({ static void prim_bitAnd(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - v.mkInt(state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitAnd") - & state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitAnd")); + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitAnd"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitAnd"); + v.mkInt(i1.value & i2.value); } static RegisterPrimOp primop_bitAnd({ @@ -3817,8 +3849,10 @@ static RegisterPrimOp primop_bitAnd({ static void prim_bitOr(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - v.mkInt(state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitOr") - | state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitOr")); + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitOr"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitOr"); + + v.mkInt(i1.value | i2.value); } static RegisterPrimOp primop_bitOr({ @@ -3832,8 +3866,10 @@ static RegisterPrimOp primop_bitOr({ static void prim_bitXor(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - v.mkInt(state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitXor") - ^ state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitXor")); + auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitXor"); + auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitXor"); + + v.mkInt(i1.value ^ i2.value); } static RegisterPrimOp primop_bitXor({ @@ -3913,13 +3949,19 @@ static RegisterPrimOp primop_toString({ non-negative. */ static void prim_substring(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - int start = state.forceInt(*args[0], pos, "while evaluating the first argument (the start offset) passed to builtins.substring"); + NixInt::Inner start = state.forceInt(*args[0], pos, "while evaluating the first argument (the start offset) passed to builtins.substring").value; if (start < 0) state.error("negative start position in 'substring'").atPos(pos).debugThrow(); - int len = state.forceInt(*args[1], pos, "while evaluating the second argument (the substring length) passed to builtins.substring"); + NixInt::Inner len = state.forceInt(*args[1], pos, "while evaluating the second argument (the substring length) passed to builtins.substring").value; + + // Negative length may be idiomatically passed to builtins.substring to get + // the tail of the string. + if (len < 0) { + len = std::numeric_limits::max(); + } // Special-case on empty substring to avoid O(n) strlen // This allows for the use of empty substrings to efficently capture string context @@ -3962,7 +4004,7 @@ static void prim_stringLength(EvalState & state, const PosIdx pos, Value * * arg { NixStringContext context; auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.stringLength"); - v.mkInt(s->size()); + v.mkInt(NixInt::Inner(s->size())); } static RegisterPrimOp primop_stringLength({ diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index f79b6b7b8..78328701d 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -122,9 +122,15 @@ static void fetchTree( } else if (attr.value->type() == nBool) attrs.emplace(state.symbols[attr.name], Explicit{attr.value->boolean()}); - else if (attr.value->type() == nInt) - attrs.emplace(state.symbols[attr.name], uint64_t(attr.value->integer())); - else if (state.symbols[attr.name] == "publicKeys") { + else if (attr.value->type() == nInt) { + auto intValue = attr.value->integer().value; + + if (intValue < 0) { + state.error("negative value given for fetchTree attr %1%: %2%", state.symbols[attr.name], intValue).atPos(pos).debugThrow(); + } + + attrs.emplace(state.symbols[attr.name], uint64_t(intValue)); + } else if (state.symbols[attr.name] == "publicKeys") { experimentalFeatureSettings.require(Xp::VerifiedFetches); attrs.emplace(state.symbols[attr.name], printValueAsJSON(state, true, *attr.value, pos, context).dump()); } diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index f8cc05616..8044fe347 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -22,7 +22,7 @@ json printValueAsJSON(EvalState & state, bool strict, switch (v.type()) { case nInt: - out = v.integer(); + out = v.integer().value; break; case nBool: diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 257da1d2d..1510abfea 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -8,6 +8,7 @@ #include "value/context.hh" #include "source-path.hh" #include "print-options.hh" +#include "checked-arithmetic.hh" #if HAVE_BOEHMGC #include @@ -73,8 +74,8 @@ class EvalState; class XMLWriter; class Printer; -typedef int64_t NixInt; -typedef double NixFloat; +using NixInt = checked::Checked; +using NixFloat = double; /** * External values must descend from ExternalValueBase, so that @@ -304,6 +305,11 @@ public: return internalType != tUninitialized; } + inline void mkInt(NixInt::Inner n) + { + mkInt(NixInt{n}); + } + inline void mkInt(NixInt n) { finishValue(tInt, { .integer = n }); diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index 627dfe830..fd1183514 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -140,9 +140,16 @@ static FlakeInput parseFlakeInput(EvalState & state, case nBool: attrs.emplace(state.symbols[attr.name], Explicit { attr.value->boolean() }); break; - case nInt: - attrs.emplace(state.symbols[attr.name], (long unsigned int) attr.value->integer()); + case nInt: { + auto intValue = attr.value->integer().value; + + if (intValue < 0) { + state.error("negative value given for flake input attribute %1%: %2%", state.symbols[attr.name], intValue).debugThrow(); + } + + attrs.emplace(state.symbols[attr.name], uint64_t(intValue)); break; + } default: if (attr.name == state.symbols.create("publicKeys")) { experimentalFeatureSettings.require(Xp::VerifiedFetches); @@ -272,7 +279,7 @@ static Flake readFlake( else if (setting.value->type() == nInt) flake.config.settings.emplace( state.symbols[setting.name], - state.forceInt(*setting.value, setting.pos, "")); + state.forceInt(*setting.value, setting.pos, "").value); else if (setting.value->type() == nBool) flake.config.settings.emplace( state.symbols[setting.name], @@ -904,8 +911,13 @@ static void prim_flakeRefToString( for (const auto & attr : *args[0]->attrs()) { auto t = attr.value->type(); if (t == nInt) { - attrs.emplace(state.symbols[attr.name], - (uint64_t) attr.value->integer()); + auto intValue = attr.value->integer().value; + + if (intValue < 0) { + state.error("negative value given for flake ref attr %1%: %2%", state.symbols[attr.name], intValue).atPos(pos).debugThrow(); + } + + attrs.emplace(state.symbols[attr.name], uint64_t(intValue)); } else if (t == nBool) { attrs.emplace(state.symbols[attr.name], Explicit { attr.value->boolean() }); diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 8760c9d14..1ca9f02f5 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -1131,7 +1131,10 @@ public: )"}; Setting maxFree{ - this, std::numeric_limits::max(), "max-free", + // n.b. this is deliberately int64 max rather than uint64 max because + // this goes through the Nix language JSON parser and thus needs to be + // representable in Nix language integers. + this, std::numeric_limits::max(), "max-free", R"( When a garbage collection is triggered by the `min-free` option, it stops as soon as `max-free` bytes are available. The default is @@ -1221,7 +1224,10 @@ public: Setting warnLargePathThreshold{ this, - std::numeric_limits::max(), + // n.b. this is deliberately int64 max rather than uint64 max because + // this goes through the Nix language JSON parser and thus needs to be + // representable in Nix language integers. + std::numeric_limits::max(), "warn-large-path-threshold", R"( Warn when copying a path larger than this number of bytes to the Nix store diff --git a/tests/functional/lang/eval-fail-fetchTree-negative.err.exp b/tests/functional/lang/eval-fail-fetchTree-negative.err.exp new file mode 100644 index 000000000..d9ba1f0b2 --- /dev/null +++ b/tests/functional/lang/eval-fail-fetchTree-negative.err.exp @@ -0,0 +1,8 @@ +error: + … while calling the 'fetchTree' builtin + at /pwd/lang/eval-fail-fetchTree-negative.nix:1:1: + 1| builtins.fetchTree { + | ^ + 2| type = "file"; + + error: negative value given for fetchTree attr owner: -1 diff --git a/tests/functional/lang/eval-fail-fetchTree-negative.nix b/tests/functional/lang/eval-fail-fetchTree-negative.nix new file mode 100644 index 000000000..90bcab5d8 --- /dev/null +++ b/tests/functional/lang/eval-fail-fetchTree-negative.nix @@ -0,0 +1,5 @@ +builtins.fetchTree { + type = "file"; + url = "file://eval-fail-fetchTree-negative.nix"; + owner = -1; +} diff --git a/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.err.exp b/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.err.exp new file mode 100644 index 000000000..25c8d7eaa --- /dev/null +++ b/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.err.exp @@ -0,0 +1,14 @@ +error: + … while calling the 'seq' builtin + at /pwd/lang/eval-fail-flake-ref-to-string-negative-integer.nix:1:16: + 1| let n = -1; in builtins.seq n (builtins.flakeRefToString { + | ^ + 2| type = "github"; + + … while calling the 'flakeRefToString' builtin + at /pwd/lang/eval-fail-flake-ref-to-string-negative-integer.nix:1:32: + 1| let n = -1; in builtins.seq n (builtins.flakeRefToString { + | ^ + 2| type = "github"; + + error: negative value given for flake ref attr repo: -1 diff --git a/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.nix b/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.nix new file mode 100644 index 000000000..e0208eb25 --- /dev/null +++ b/tests/functional/lang/eval-fail-flake-ref-to-string-negative-integer.nix @@ -0,0 +1,7 @@ +let n = -1; in builtins.seq n (builtins.flakeRefToString { + type = "github"; + owner = "NixOS"; + repo = n; + ref = "23.05"; + dir = "lib"; +}) diff --git a/tests/functional/lang/eval-fail-fromJSON-overflowing.err.exp b/tests/functional/lang/eval-fail-fromJSON-overflowing.err.exp new file mode 100644 index 000000000..a39082b45 --- /dev/null +++ b/tests/functional/lang/eval-fail-fromJSON-overflowing.err.exp @@ -0,0 +1,8 @@ +error: + … while calling the 'fromJSON' builtin + at /pwd/lang/eval-fail-fromJSON-overflowing.nix:1:1: + 1| builtins.fromJSON ''{"attr": 18446744073709551615}'' + | ^ + 2| + + error: unsigned json number 18446744073709551615 outside of Nix integer range diff --git a/tests/functional/lang/eval-fail-fromJSON-overflowing.nix b/tests/functional/lang/eval-fail-fromJSON-overflowing.nix new file mode 100644 index 000000000..6dfbce3f6 --- /dev/null +++ b/tests/functional/lang/eval-fail-fromJSON-overflowing.nix @@ -0,0 +1 @@ +builtins.fromJSON ''{"attr": 18446744073709551615}'' diff --git a/tests/functional/lang/eval-fail-overflowing-add.err.exp b/tests/functional/lang/eval-fail-overflowing-add.err.exp new file mode 100644 index 000000000..6458cf1c9 --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-add.err.exp @@ -0,0 +1,6 @@ +error: integer overflow in adding 9223372036854775807 + 1 + at /pwd/lang/eval-fail-overflowing-add.nix:4:8: + 3| b = 1; + 4| in a + b + | ^ + 5| diff --git a/tests/functional/lang/eval-fail-overflowing-add.nix b/tests/functional/lang/eval-fail-overflowing-add.nix new file mode 100644 index 000000000..24258fc20 --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-add.nix @@ -0,0 +1,4 @@ +let + a = 9223372036854775807; + b = 1; +in a + b diff --git a/tests/functional/lang/eval-fail-overflowing-div.err.exp b/tests/functional/lang/eval-fail-overflowing-div.err.exp new file mode 100644 index 000000000..8ce07d4d6 --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-div.err.exp @@ -0,0 +1,23 @@ +error: + … while calling the 'seq' builtin + at /pwd/lang/eval-fail-overflowing-div.nix:7:4: + 6| b = -1; + 7| in builtins.seq intMin (builtins.seq b (intMin / b)) + | ^ + 8| + + … while calling the 'seq' builtin + at /pwd/lang/eval-fail-overflowing-div.nix:7:25: + 6| b = -1; + 7| in builtins.seq intMin (builtins.seq b (intMin / b)) + | ^ + 8| + + … while calling the 'div' builtin + at /pwd/lang/eval-fail-overflowing-div.nix:7:48: + 6| b = -1; + 7| in builtins.seq intMin (builtins.seq b (intMin / b)) + | ^ + 8| + + error: integer overflow in dividing -9223372036854775808 / -1 diff --git a/tests/functional/lang/eval-fail-overflowing-div.nix b/tests/functional/lang/eval-fail-overflowing-div.nix new file mode 100644 index 000000000..44fbe9d7e --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-div.nix @@ -0,0 +1,7 @@ +let + # lol, this has to be written as an expression like this because negative + # numbers use unary negation rather than parsing directly, and 2**63 is out + # of range + intMin = -9223372036854775807 - 1; + b = -1; +in builtins.seq intMin (builtins.seq b (intMin / b)) diff --git a/tests/functional/lang/eval-fail-overflowing-mul.err.exp b/tests/functional/lang/eval-fail-overflowing-mul.err.exp new file mode 100644 index 000000000..f42b39d4d --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-mul.err.exp @@ -0,0 +1,16 @@ +error: + … while calling the 'mul' builtin + at /pwd/lang/eval-fail-overflowing-mul.nix:3:10: + 2| a = 4294967297; + 3| in a * a * a + | ^ + 4| + + … while calling the 'mul' builtin + at /pwd/lang/eval-fail-overflowing-mul.nix:3:6: + 2| a = 4294967297; + 3| in a * a * a + | ^ + 4| + + error: integer overflow in multiplying 4294967297 * 4294967297 diff --git a/tests/functional/lang/eval-fail-overflowing-mul.nix b/tests/functional/lang/eval-fail-overflowing-mul.nix new file mode 100644 index 000000000..6081d9c7b --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-mul.nix @@ -0,0 +1,3 @@ +let + a = 4294967297; +in a * a * a diff --git a/tests/functional/lang/eval-fail-overflowing-sub.err.exp b/tests/functional/lang/eval-fail-overflowing-sub.err.exp new file mode 100644 index 000000000..66a3a03f8 --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-sub.err.exp @@ -0,0 +1,9 @@ +error: + … while calling the 'sub' builtin + at /pwd/lang/eval-fail-overflowing-sub.nix:4:6: + 3| b = 2; + 4| in a - b + | ^ + 5| + + error: integer overflow in subtracting -9223372036854775807 - 2 diff --git a/tests/functional/lang/eval-fail-overflowing-sub.nix b/tests/functional/lang/eval-fail-overflowing-sub.nix new file mode 100644 index 000000000..229b8c6d2 --- /dev/null +++ b/tests/functional/lang/eval-fail-overflowing-sub.nix @@ -0,0 +1,4 @@ +let + a = -9223372036854775807; + b = 2; +in a - b diff --git a/tests/unit/libexpr-support/tests/libexpr.hh b/tests/unit/libexpr-support/tests/libexpr.hh index 54a4a0e9e..045607e87 100644 --- a/tests/unit/libexpr-support/tests/libexpr.hh +++ b/tests/unit/libexpr-support/tests/libexpr.hh @@ -85,7 +85,7 @@ namespace nix { if (arg.type() != nInt) { return false; } - return arg.integer() == v; + return arg.integer().value == v; } MATCHER_P(IsFloatEq, v, fmt("The float is equal to \"%1%\"", v)) { From bf050d9e966534eeb92188da1a93ac459bed0f97 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 12 Jul 2024 18:20:14 +0200 Subject: [PATCH 4/6] docs: update to define integer overflow Change-Id: Ie8a1b31035f2d27a220e5df2e9e178ec3b39ee68 --- doc/manual/src/language/operators.md | 8 ++++++-- doc/manual/src/language/syntax.md | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/doc/manual/src/language/operators.md b/doc/manual/src/language/operators.md index e1c020781..2c2dcc276 100644 --- a/doc/manual/src/language/operators.md +++ b/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]. diff --git a/doc/manual/src/language/syntax.md b/doc/manual/src/language/syntax.md index 6108bacd6..f05935502 100644 --- a/doc/manual/src/language/syntax.md +++ b/doc/manual/src/language/syntax.md @@ -183,6 +183,13 @@ This section covers syntax and semantics of the Nix language. 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 From 5878b1475f1c50b42672ed85e7259e7d328920e9 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Thu, 25 Jul 2024 23:06:53 +0200 Subject: [PATCH 5/6] doc: release notes for banning integer overflow Change-Id: Ib75ab5b8b4d879035d7ee7678f9cd0c491a39c0a --- doc/manual/rl-next/ban-integer-overflow.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 doc/manual/rl-next/ban-integer-overflow.md diff --git a/doc/manual/rl-next/ban-integer-overflow.md b/doc/manual/rl-next/ban-integer-overflow.md new file mode 100644 index 000000000..49ecfe4c6 --- /dev/null +++ b/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 *probably* 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 is 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 +``` From 3cc2e2a0ac7df557f5cd958a0736ce4eedf5ae07 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 11 Aug 2024 03:31:44 +0200 Subject: [PATCH 6/6] Edit docs --- doc/manual/rl-next/ban-integer-overflow.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/manual/rl-next/ban-integer-overflow.md b/doc/manual/rl-next/ban-integer-overflow.md index 49ecfe4c6..0e553af76 100644 --- a/doc/manual/rl-next/ban-integer-overflow.md +++ b/doc/manual/rl-next/ban-integer-overflow.md @@ -4,10 +4,10 @@ issues: [10968] prs: [11188] --- -Previously, integer overflow in the Nix language invoked C++ level signed overflow, which was undefined behaviour, but *probably* manifested as wrapping around on overflow. +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 is safe to turn into an error. +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. @@ -16,6 +16,6 @@ Some other overflows were fixed: Integer overflow now looks like the following: ``` -» nix eval --expr '9223372036854775807 + 1' +$ nix eval --expr '9223372036854775807 + 1' error: integer overflow in adding 9223372036854775807 + 1 ```