diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/SignedComparisons.ql b/cpp/ql/src/Likely Bugs/Arithmetic/SignedComparisons.ql new file mode 100644 index 00000000000..b6dd8fb1d42 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Arithmetic/SignedComparisons.ql @@ -0,0 +1,33 @@ +/** + * @name Undefined result of signed test for overflow + * @description Testing for signed integer overflow by adding a value to + * a variable (or subtracting a value from a variable) and + * then comparing the result to said variable is not defined + * by the C or C++ standards. The comparison may produce an + * unintended result, or may be deleted by the compiler + * entirely. + * @kind problem + * @problem.severity warning + * @precision medium + * @id cpp/signed-overflow-check + * @tags reliability + * security + */ + +import cpp + +from RelationalOperation ro, BinaryArithmeticOperation bao, VariableAccess va1, VariableAccess va2 +where + ro.getAnOperand() = bao and + (bao instanceof AddExpr or bao instanceof SubExpr) and + bao.getAnOperand() = va1 and + ro.getAnOperand() = va2 and + va1.getTarget() = va2.getTarget() and + /* + * if the addition/subtraction (`bao`) has been promoted to a signed type, + * then the other operand (`va2`) must also be signed and we have a signed + * comparison + */ + + bao.getFullyConverted().getType().(IntegralType).isSigned() +select ro, "Testing for signed overflow/underflow may produce undefined results." diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.cpp b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.cpp new file mode 100644 index 00000000000..32d3bab19c7 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.cpp @@ -0,0 +1,38 @@ +// Signed-comparison tests + +/* 1. Signed-signed comparison. The semantics are undefined. */ +bool cannotHoldAnother8(int n1) { + // clang 8.0.0 -O2: deleted (silently) + // gcc 9.2 -O2: deleted (silently) + // msvc 19.22 /O2: not deleted + return n1 + 8 < n1; // BAD +} +bool canHoldPreceding16(int n1) { + return n1 - 16 < n1; +} + +/* 2. Signed comparison with a narrower unsigned type. The narrower + type gets promoted to the (signed) larger type, and so the + semantics are undefined. */ +bool cannotHoldAnotherUShort(int n1, unsigned short delta) { + // clang 8.0.0 -O2: deleted (silently) + // gcc 9.2 -O2: deleted (silently) + // msvc 19.22 /O2: not deleted + return n1 + delta < n1; // BAD +} +bool canHoldPrecedingUShort(int n1, unsigned short delta) { + return n1 - delta < n1; +} + +/* 3. Signed comparison with a non-narrower unsigned type. The + signed type gets promoted to (a possibly wider) unsigned type, + and the resulting comparison is unsigned. */ +bool cannotHoldAnotherUInt(int n1, unsigned int delta) { + // clang 8.0.0 -O2: not deleted + // gcc 9.2 -O2: not deleted + // msvc 19.22 /O2: not deleted + return n1 + delta < n1; // GOOD +} +bool canHoldPrecedingUInt(int n1, unsigned int delta) { + return n1 - delta < n1; +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.qlref b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.qlref new file mode 100644 index 00000000000..f645e2e2408 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/SignedComparisons/SignedComparisons.qlref @@ -0,0 +1 @@ +Likely Bugs/Arithmetic/SignedComparisons.ql