From: Alex Rousskov Date: Thu, 24 Feb 2022 02:17:14 +0000 (+0000) Subject: Fix Sum() by replacing it with a safer NaturalSum() (#869) X-Git-Tag: SQUID_6_0_1~228 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b308d7e2ad02ae6622f380d94d2303446f5831a9;p=thirdparty%2Fsquid.git Fix Sum() by replacing it with a safer NaturalSum() (#869) While testing future cache entry expiration computation code, we found a Sum() bug: Sum(0l, -1lu) returns -1 instead of overflowing (long cannot hold the maximum unsigned long value). This discovery triggered an investigation that discovered several Sum() flaws, even inside the supposed-to-be-trivial implementation branch for unsigned operands: 0. AllUnsigned-based optimization path selection was based on the detection of unsigned raw S and T types, but the actual s+t sum used integral-promoted types that could be signed! 1. "sizeof(T) >= sizeof(U)" assertion missed that same-size types have very different maximums when exactly one of the types is signed. 2. "auto sum = a + b" assignment missed that "auto" may be bigger than the return type "T" and, hence, the sum may not overflow when it does not fit T. The overflow would go undetected and the result will be truncated (to fit T) in the return statement. This automatic sum type enlargement may be due to signed->unsigned integral conversion related to the previous bullet, but it can also happen because of integral _promotions_ (e.g., those that convert "char" into "int"). 3. Sum() silently truncated its arguments to fit T. Before commit 1fba9ab, that silent truncation only applied to the first argument (if T did not match its actual type). After that commit, all other arguments could be silently truncated (except the last one). 4. It is trivial for the caller to do everything right when calling Sum() but then assign the result to a variable that cannot hold Sum() value, essentially undoing all the overflow detection work. Fortunately, none of these bugs affected two existing Sum() callers. Some of these problems were very difficult to fix! Eventually, a new, simplified concept emerged that was easier to implement and that was a better match for Squid current and foreseeable needs: NaturalSum(). NaturalSum() is designed for cases where we want an exact sum of arguments but do not consider negative arguments as valid numbers. This both simplifies the implementation and protects typical callers from adding "-1" (e.g., a special "no delay" option setting) to "3600" (e.g., configured TTL) and getting a meaningless result of 3599. NaturalSum() requires the caller to specify the summation type that will be used to accumulate the sum value as Squid iterates over arguments. Sum() wanted to automatically use the largest type that can accommodate (partial) sums, but that complicated task was not implemented and becomes unnecessary when dealing with only natural numbers -- there is no need to temporary inflate the partial sum (beyond what the resulting type can hold) in case some negative operand will decrease it later. Also added SetToNaturalSumOrMax() to allow the caller to reset a variable without guessing its type, avoiding silent sum truncation at assignment time. Also added unit test cases. --- diff --git a/src/Makefile.am b/src/Makefile.am index 3507c8489d..27bbff6ff5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -869,6 +869,18 @@ tests_testBoilerplate_LDFLAGS = $(LIBADD_DL) # dependency-based list so that simpler code is tested before more complex code # which uses it. +## Tests of SquidMath.h +check_PROGRAMS += tests/testMath +tests_testMath_SOURCES = \ + tests/testMath.cc +nodist_tests_testMath_SOURCES = \ + SquidMath.h +tests_testMath_LDADD = \ + $(LIBCPPUNIT_LIBS) \ + $(COMPAT_LIB) \ + $(XTRA_LIBS) +tests_testMath_LDFLAGS = $(LIBADD_DL) + ## Tests of mem/* check_PROGRAMS += tests/testMem diff --git a/src/SquidMath.h b/src/SquidMath.h index acc2ec713a..e909cc905d 100644 --- a/src/SquidMath.h +++ b/src/SquidMath.h @@ -10,6 +10,7 @@ #define _SQUID_SRC_SQUIDMATH_H #include "base/forward.h" +#include "base/Optional.h" #include #include @@ -32,12 +33,12 @@ double doubleAverage(const double, const double, int, const int); // built-ins like __builtin_add_overflow() instead of manual overflow checks. /// std::enable_if_t replacement until C++14 -/// simplifies Sum() declarations below +/// simplifies declarations further below template using EnableIfType = typename std::enable_if::type; /// detects a pair of unsigned types -/// reduces code duplication in Sum() declarations below +/// reduces code duplication in declarations further below template using AllUnsigned = typename std::conditional< std::is_unsigned::value && std::is_unsigned::value, @@ -45,47 +46,144 @@ using AllUnsigned = typename std::conditional< std::false_type >::type; +// TODO: Replace with std::cmp_less() after migrating to C++20. +/// whether integer a is less than integer b, with correct overflow handling +template +constexpr bool +Less(const A a, const B b) { + // The casts below make standard C++ integer conversions explicit. They + // quell compiler warnings about signed/unsigned comparison. The first two + // lines exclude different-sign a and b, making the casts/comparison safe. + using AB = typename std::common_type::type; + return + (a >= 0 && b < 0) ? false : + (a < 0 && b >= 0) ? true : + /* (a >= 0) == (b >= 0) */ static_cast(a) < static_cast(b); +} + +/// ensure that T is supported by NaturalSum() and friends +template +constexpr bool +AssertNaturalType() +{ + static_assert(std::numeric_limits::is_bounded, "std::numeric_limits::max() is meaningful"); + static_assert(std::numeric_limits::is_exact, "no silent loss of precision"); + static_assert(!std::is_enum::value, "no silent creation of non-enumerated values"); + return true; // for static_assert convenience in C++11 constexpr callers +} + +// TODO: Investigate whether this optimization can be expanded to [signed] types +// A and B when std::numeric_limits::is_modulo is true. +/// This IncreaseSumInternal() overload is optimized for speed. /// \returns a non-overflowing sum of the two unsigned arguments (or nothing) -template ::value, int> = 0> -Optional -Sum(const T a, const U b) { - // Instead of computing the largest type dynamically, we simply go by T and - // reject cases like Sum(0, ULLONG_MAX) that would overflow on return. - // TODO: Consider using std::common_type in the return type instead. - static_assert(sizeof(T) >= sizeof(U), "Sum() return type can fit its (unsigned) result"); - - // this optimized implementation relies on unsigned overflows - static_assert(std::is_unsigned::value, "the first Sum(a,b) argument is unsigned"); - static_assert(std::is_unsigned::value, "the second Sum(a,b) argument is unsigned"); - const auto sum = a + b; - // when a+b overflows, the result becomes smaller than any operand - return (sum < a) ? Optional() : Optional(sum); +/// \prec both argument types are unsigned +template ::value, int> = 0> +Optional +IncreaseSumInternal(const A a, const B b) { + // paranoid: AllUnsigned precondition established that already + static_assert(std::is_unsigned::value, "AllUnsigned dispatch worked for A"); + static_assert(std::is_unsigned::value, "AllUnsigned dispatch worked for B"); + + // TODO: Just call AssertNaturalType() after upgrading to C++14. + static_assert(AssertNaturalType(), "S is a supported type"); + static_assert(AssertNaturalType(), "A is a supported type"); + static_assert(AssertNaturalType(), "B is a supported type"); + + // we should only be called by IncreaseSum(); it forces integer promotion + static_assert(std::is_same::value, "a will not be promoted"); + static_assert(std::is_same::value, "b will not be promoted"); + // and without integer promotions, a sum of unsigned integers is unsigned + static_assert(std::is_unsigned::value, "a+b is unsigned"); + + // with integer promotions ruled out, a or b can only undergo integer + // conversion to the higher rank type (A or B, we do not know which) + using AB = typename std::common_type::type; + static_assert(std::is_same::value || std::is_same::value, "no unexpected conversions"); + static_assert(std::is_same::value, "lossless assignment"); + const AB sum = a + b; + + static_assert(std::numeric_limits::is_modulo, "we can detect overflows"); + // 1. modulo math: overflowed sum is smaller than any of its operands + // 2. the sum may overflow S (i.e. the return base type) + // We do not need Less() here because we compare promoted unsigned types. + return (sum >= a && sum <= std::numeric_limits::max()) ? + Optional(sum) : Optional(); } -/// \returns a non-overflowing sum of the two signed arguments (or nothing) -template ::value, int> = 0> -Optional constexpr -Sum(const T a, const U b) { - // Instead of computing the largest type dynamically, we simply go by T and - // reject cases like Sum(0, LLONG_MAX) that would overflow on return. - static_assert(sizeof(T) >= sizeof(U), "Sum() return type can fit its (signed) result"); - - // tests below avoid undefined behavior of signed under/overflows - return b >= 0 ? - ((a > std::numeric_limits::max() - b) ? Optional() : Optional(a + b)): - ((a < std::numeric_limits::min() - b) ? Optional() : Optional(a + b)); +/// This IncreaseSumInternal() overload supports a larger variety of types. +/// \returns a non-overflowing sum of the two arguments (or nothing) +/// \returns nothing if at least one of the arguments is negative +/// \prec at least one of the argument types is signed +template ::value, int> = 0> +Optional constexpr +IncreaseSumInternal(const A a, const B b) { + static_assert(AssertNaturalType(), "S is a supported type"); + static_assert(AssertNaturalType(), "A is a supported type"); + static_assert(AssertNaturalType(), "B is a supported type"); + + // we should only be called by IncreaseSum() that does integer promotion + static_assert(std::is_same::value, "a will not be promoted"); + static_assert(std::is_same::value, "b will not be promoted"); + + return + // We could support a non-under/overflowing sum of negative numbers, but + // our callers use negative values specially (e.g., for do-not-use or + // do-not-limit settings) and are not supposed to do math with them. + (a < 0 || b < 0) ? Optional() : + // To avoid undefined behavior of signed overflow, we must not compute + // the raw a+b sum if it may overflow. When A is not B, a or b undergoes + // (safe for non-negatives) integer conversion in these expressions, so + // we do not know the resulting a+b type AB and its maximum. We must + // also detect subsequent casting-to-S overflows. + // Overflow condition: (a + b > maxAB) or (a + b > maxS). + // A is an integer promotion of S, so maxS <= maxA <= maxAB. + // Since maxS <= maxAB, it is sufficient to just check: a + b > maxS, + // which is the same as the overflow-safe condition here: maxS - a < b. + // Finally, (maxS - a) cannot overflow because a is not negative and + // cannot underflow because a is a promotion of s: 0 <= a <= maxS. + Less(std::numeric_limits::max() - a, b) ? Optional() : + Optional(a + b); +} + +/// argument pack expansion termination for IncreaseSum() +template +Optional +IncreaseSum(const S s, const T t) +{ + // Force (always safe) integer promotions now, to give EnableIfType<> + // promoted types instead of entering IncreaseSumInternal(s,t) + // but getting a _signed_ promoted value of s or t in s + t. + return IncreaseSumInternal(+s, +t); } /// \returns a non-overflowing sum of the arguments (or nothing) -template -Optional -Sum(const T first, Args... args) { - if (const auto others = Sum(args...)) { - return Sum(first, others.value()); +template +Optional +IncreaseSum(const S sum, const T t, const Args... args) { + if (const auto head = IncreaseSum(sum, t)) { + return IncreaseSum(head.value(), args...); } else { - return Optional(); + return Optional(); } } +/// \returns an exact, non-overflowing sum of the arguments (or nothing) +template +Optional +NaturalSum(const Args... args) { + return IncreaseSum(0, args...); +} + +/// Safely resets the given variable to NaturalSum() of the given arguments. +/// If the sum overflows, resets to variable's maximum possible value. +/// \returns the new variable value (like an assignment operator would) +template +S +SetToNaturalSumOrMax(S &var, const Args... args) +{ + var = NaturalSum(args...).value_or(std::numeric_limits::max()); + return var; +} + #endif /* _SQUID_SRC_SQUIDMATH_H */ diff --git a/src/base/ClpMap.h b/src/base/ClpMap.h index 6a78375350..cb3d2229ee 100644 --- a/src/base/ClpMap.h +++ b/src/base/ClpMap.h @@ -193,7 +193,7 @@ ClpMap::MemoryCountedFor(const Key &k, const Value &v) const auto keySz = k.length(); // approximate calculation (e.g., containers store wrappers not value_types) - return Sum( + return NaturalSum( keySz, // storage sizeof(typename Entries::value_type), @@ -277,8 +277,9 @@ template ClpMap::Entry::Entry(const Key &aKey, const Value &v, const Ttl ttl) : key(aKey), value(v), - expires(Sum(squid_curtime, ttl).value_or(std::numeric_limits::max())) + expires(0) // reset below { + SetToNaturalSumOrMax(expires, squid_curtime, ttl); } #endif /* SQUID__SRC_BASE_CLPMAP_H */ diff --git a/src/tests/testMath.cc b/src/tests/testMath.cc new file mode 100644 index 0000000000..e5e5a76c4d --- /dev/null +++ b/src/tests/testMath.cc @@ -0,0 +1,268 @@ +/* + * Copyright (C) 1996-2021 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "base/Optional.h" +#include "compat/cppunit.h" +#include "SquidMath.h" +#include "unitTestMain.h" + +class TestMath: public CPPUNIT_NS::TestFixture +{ + CPPUNIT_TEST_SUITE( TestMath ); + CPPUNIT_TEST( testNaturalSum ); + CPPUNIT_TEST_SUITE_END(); + +protected: + void testNaturalSum(); +}; + +CPPUNIT_TEST_SUITE_REGISTRATION( TestMath ); + +/// bit-width-specific integers, for developer convenience and code readability +/// @{ +static const auto min64s = std::numeric_limits::min(); +static const auto min8s = std::numeric_limits::min(); +static const auto zero8s = int8_t(0); +static const auto zero8u = uint8_t(0); +static const auto zero64s = int64_t(0); +static const auto zero64u = uint64_t(0); +static const auto one8s = int8_t(1); +static const auto one8u = uint8_t(1); +static const auto one64s = int64_t(1); +static const auto one64u = uint64_t(1); +static const auto max8s = std::numeric_limits::max(); +static const auto max8u = std::numeric_limits::max(); +static const auto max64s = std::numeric_limits::max(); +static const auto max64u = std::numeric_limits::max(); +/// @} + +/// helper functions to convert NaturalSum(a,b,...) calls to strings +/// @{ + +template +static std::string +TypeToString() +{ + const std::string prefix = std::is_signed::value ? "" : "u"; + return prefix + "int" + std::to_string(sizeof(A)*8); +} + +template +static std::string +OperandToString(const A a) +{ + return TypeToString() + '(' + std::to_string(+a) + ')'; +} + +template +static std::string +SumToString(const A a, const B b) +{ + return TypeToString() + ": " + OperandToString(a) + " + " + OperandToString(b); +} + +template +static std::string +SumToString(const A a, const B b, const C c) +{ + return TypeToString() + ": " + OperandToString(a) + " + " + OperandToString(b) + " + " + OperandToString(c); +} + +/// @} + +/// ends argument recursion for RawSum() with parameters +template +static S +RawSum() +{ + return S(0); +} + +/// helper function to add up an arbitrary number of arbitrary-type integers +/// while converting every number to type S and ignoring any under/overflows +template +static S +RawSum(A a, Args... args) +{ + return S(a) + RawSum(args...); +} + +/// Tests NaturalSum() calls that are supposed to succeed. +/// Implemented as a class to pass it as a template template parameter. +template +class SuccessSumTester +{ +public: + template + static S Test(Args... args) + { + // to show every non-overflowing sum to be tested: + //std::cout << SumToString(args...) << " = " << +sum << "\n"; + + const auto ns = NaturalSum(args...); + CPPUNIT_ASSERT_MESSAGE(SumToString(args...) + " does not overflow", + ns.has_value()); + + const auto sum = ns.value(); + const auto expected = RawSum(args...); + CPPUNIT_ASSERT_MESSAGE( + SumToString(args...) + " = " + OperandToString(expected) + " rather than " + OperandToString(sum), + sum == expected); + + return sum; + } +}; + +/// Tests NaturalSum() calls that are supposed to overflow. +/// Implemented as a class to pass it as a template template parameter. +template +class OverflowSumTester +{ +public: + template + static void Test(Args... args) + { + // to show every overflowing sum to be tested: + //std::cout << SumToString(args...) << " = overflow\n"; + + CPPUNIT_ASSERT_MESSAGE(SumToString(args...) + " must overflow", + !NaturalSum(args...).has_value()); + } +}; + +/// checks that the summation outcome is unaffected by (not) adding zeros +template class Tester, typename A, typename B> +static void +TestWithZeros(const A a, const B b) +{ + Tester::Test(a, b); + + Tester::Test(zero8u, a, b); + Tester::Test(zero8s, a, b); + Tester::Test(zero64u, a, b); + Tester::Test(zero64s, a, b); + Tester::Test(a, zero8u, b); + Tester::Test(a, zero8s, b); + Tester::Test(a, zero64u, b); + Tester::Test(a, zero64s, b); + Tester::Test(a, b, zero8u); + Tester::Test(a, b, zero8s); + Tester::Test(a, b, zero64u); + Tester::Test(a, b, zero64s); +} + +/// checks that the summation outcome is unaffected by the order of operands +template class Tester, typename A, typename B> +static void +TestOrder(const A a, const B b) +{ + TestWithZeros(a, b); + TestWithZeros(b, a); +} + +/// checks that a+b and similar sums overflow for summation types A and B +template +static void +TestOverflowForEitherSummationType(const A a, const B b) +{ + TestOrder(a, b); + TestOrder(a, b); +} + +/// checks that a+b and similar sums succeed for summation type A but overflow +/// for summation type B +template +static void +TestSuccessForFirstSummationType(const A a, const B b) +{ + TestOrder(a, b); + TestOrder(a, b); +} + +/// \returns successful a+b value using summation type S (which defaults to A) +template +static S +GoodSum(const A a, Args... args) +{ + return SuccessSumTester::Test(a, args...); +} + +void +TestMath::testNaturalSum() +{ + /* + * To simplify spelling out of these repetitive test cases, we let function + * parameters determine the summation type. Regular code should not do that, + * and our public summation APIs do not provide this dangerous shortcut. + */ + + // negative parameters are banned in any position + TestOverflowForEitherSummationType(min64s, zero8s); + TestOverflowForEitherSummationType(min64s, zero8u); + TestOverflowForEitherSummationType(min64s, max64s); + TestOverflowForEitherSummationType(min64s, max64u); + TestOverflowForEitherSummationType(min8s, zero8s); + TestOverflowForEitherSummationType(min8s, zero8s); + TestOverflowForEitherSummationType(min8s, zero8u); + TestOverflowForEitherSummationType(min8s, max64s); + TestOverflowForEitherSummationType(min8s, max64u); + TestOverflowForEitherSummationType(-1, -1); + TestOverflowForEitherSummationType(-1, zero8s); + TestOverflowForEitherSummationType(-1, zero8u); + TestOverflowForEitherSummationType(-1, max64s); + TestOverflowForEitherSummationType(-1, max64u); + + // these overflow regardless of which parameter determines the summation type + TestOverflowForEitherSummationType(max8u, one8u); + TestOverflowForEitherSummationType(max8u, one8s); + TestOverflowForEitherSummationType(max8u, max8s); + TestOverflowForEitherSummationType(max8s, one8s); + TestOverflowForEitherSummationType(max64u, one8u); + TestOverflowForEitherSummationType(max64u, one8s); + TestOverflowForEitherSummationType(max64u, one64u); + TestOverflowForEitherSummationType(max64u, one64s); + TestOverflowForEitherSummationType(max64u, max64s); + TestOverflowForEitherSummationType(max64s, one8u); + TestOverflowForEitherSummationType(max64s, one8s); + TestOverflowForEitherSummationType(max64s, one64s); + + // these overflow only if the second parameter determines the summation type + TestSuccessForFirstSummationType(one8u, max8s); + TestSuccessForFirstSummationType(one64u, max8u); + TestSuccessForFirstSummationType(one64u, max64s); + TestSuccessForFirstSummationType(one64s, max8u); + TestSuccessForFirstSummationType(one64s, max8s); + TestSuccessForFirstSummationType(max64u, zero8u); + TestSuccessForFirstSummationType(max64u, zero8s); + TestSuccessForFirstSummationType(max64s, zero8u); + TestSuccessForFirstSummationType(max64s, zero8s); + + // a few sums with known values + CPPUNIT_ASSERT_EQUAL(zero8s, GoodSum(zero8s, zero8u)); + CPPUNIT_ASSERT_EQUAL(zero64s, GoodSum(zero64s, zero64u)); + CPPUNIT_ASSERT_EQUAL(2, GoodSum(1, 1)); + CPPUNIT_ASSERT_EQUAL(uint64_t(2), GoodSum(one64u, one64s)); + CPPUNIT_ASSERT_EQUAL(6u, GoodSum(1u, 2, 3)); + CPPUNIT_ASSERT_EQUAL(max64u, GoodSum(zero64u, max64u)); + CPPUNIT_ASSERT_EQUAL(max64s, GoodSum(zero64s, max64s)); + CPPUNIT_ASSERT_EQUAL(one64u + max64s, GoodSum(one64u, max64s)); + CPPUNIT_ASSERT_EQUAL(max64u, GoodSum(max64u, zero8s)); + CPPUNIT_ASSERT_EQUAL(max64s, GoodSum(max64s, zero8s)); + + // long argument lists (odd and even lengths) + CPPUNIT_ASSERT_EQUAL(15, NaturalSum(1, 2, 3, 4, 5).value()); + CPPUNIT_ASSERT_EQUAL(21, NaturalSum(1, 2, 3, 4, 5, 6).value()); + + // test SetToNaturalSumOrMax() when the sum is too big for the variable + long expires = 0; + const auto result = SetToNaturalSumOrMax(expires, max64u, zero8u); + CPPUNIT_ASSERT_EQUAL(std::numeric_limits::max(), expires); + CPPUNIT_ASSERT_EQUAL(expires, result); +} +