Merge pull request github#5616 from geoffw0/unsigneddiff2

C++: Improve cpp/unsigned-difference-expression-compared-zero

Author: MathiasVP

cpp/change-notes/2021-04-09-unsigned-difference-expression-compared-zero.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The 'Unsigned difference expression compared to zero' (cpp/unsigned-difference-expression-compared-zero) query has been improved to produce fewer false positive results.

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -12,29 +12,60 @@
 
 import cpp
 import semmle.code.cpp.commons.Exclusions
-import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.dataflow.DataFlow
 
-/** Holds if `sub` is guarded by a condition which ensures that `left >= right`. */
+/**
+ *  Holds if `sub` is guarded by a condition which ensures that
+ * `left >= right`.
+ */
 pragma[noinline]
 predicate isGuarded(SubExpr sub, Expr left, Expr right) {
-  exists(GuardCondition guard |
-    guard.controls(sub.getBasicBlock(), true) and
-    guard.ensuresLt(left, right, 0, sub.getBasicBlock(), false)
+  exists(GuardCondition guard, int k |
+    guard.controls(sub.getBasicBlock(), _) and
+    guard.ensuresLt(left, right, k, sub.getBasicBlock(), false) and
+    k >= 0
   )
 }
 
-/** Holds if `sub` will never be negative. */
-predicate nonNegative(SubExpr sub) {
-  not exprMightOverflowNegatively(sub.getFullyConverted())
+/**
+ * Holds if `n` is known or suspected to be less than or equal to
+ * `sub.getLeftOperand()`.
+ */
+predicate exprIsSubLeftOrLess(SubExpr sub, DataFlow::Node n) {
+  n.asExpr() = sub.getLeftOperand()
+  or
+  exists(DataFlow::Node other |
+    // dataflow
+    exprIsSubLeftOrLess(sub, other) and
+    (
+      DataFlow::localFlowStep(n, other) or
+      DataFlow::localFlowStep(other, n)
+    )
+  )
+  or
+  exists(DataFlow::Node other |
+    // guard constraining `sub`
+    exprIsSubLeftOrLess(sub, other) and
+    isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
+  )
+  or
+  exists(DataFlow::Node other, float p, float q |
+    // linear access of `other`
+    exprIsSubLeftOrLess(sub, other) and
+    linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
+    p <= 1 and
+    q <= 0
+  )
   or
-  // The subtraction is guarded by a check of the form `left >= right`.
-  exists(GVN left, GVN right |
-    // This is basically a poor man's version of a directional unbind operator.
-    strictcount([left, globalValueNumber(sub.getLeftOperand())]) = 1 and
-    strictcount([right, globalValueNumber(sub.getRightOperand())]) = 1 and
-    isGuarded(sub, left.getAnExpr(), right.getAnExpr())
+  exists(DataFlow::Node other, float p, float q |
+    // linear access of `n`
+    exprIsSubLeftOrLess(sub, other) and
+    linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
+    p >= 1 and
+    q >= 0
   )
 }
 
@@ -45,5 +76,6 @@ where
   ro.getLesserOperand().getValue().toInt() = 0 and
   ro.getGreaterOperand() = sub and
   sub.getFullyConverted().getUnspecifiedType().(IntegralType).isUnsigned() and
-  not nonNegative(sub)
+  exprMightOverflowNegatively(sub.getFullyConverted()) and // generally catches false positives involving constants
+  not exprIsSubLeftOrLess(sub, DataFlow::exprNode(sub.getRightOperand())) // generally catches false positives where there's a relation between the left and right operands
 select ro, "Unsigned subtraction can never be negative."

cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/UnsignedDifferenceExpressionComparedZero.expected

@@ -1,10 +1,15 @@
 | test.cpp:6:5:6:13 | ... > ... | Unsigned subtraction can never be negative. |
 | test.cpp:10:8:10:24 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:15:9:15:25 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:32:12:32:20 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:39:12:39:20 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:47:5:47:13 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:55:5:55:13 | ... > ... | Unsigned subtraction can never be negative. |
 | test.cpp:62:5:62:13 | ... > ... | Unsigned subtraction can never be negative. |
-| test.cpp:69:5:69:13 | ... > ... | Unsigned subtraction can never be negative. |
 | test.cpp:75:8:75:16 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:101:6:101:14 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:128:6:128:14 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:137:6:137:14 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:146:7:146:15 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:152:7:152:15 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:182:6:182:14 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:208:6:208:14 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:252:10:252:18 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:266:10:266:24 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:276:11:276:19 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:288:10:288:18 | ... > ... | Unsigned subtraction can never be negative. |

cpp/ql/test/query-tests/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero/test.cpp

@@ -12,7 +12,7 @@ void test(unsigned x, unsigned y, bool unknown) {
 	}
 
 	if(total <= limit) {
-		while(limit - total > 0) { // GOOD [FALSE POSITIVE]
+		while(limit - total > 0) { // GOOD
 			total += getAnInt();
 			if(total > limit) break;
 		}
@@ -29,30 +29,30 @@ void test(unsigned x, unsigned y, bool unknown) {
 	} else {
 		y = x;
 	}
-	bool b1 = x - y > 0; // GOOD [FALSE POSITIVE]
+	bool b1 = x - y > 0; // GOOD
 
 	x = getAnInt();
 	y = getAnInt();
 	if(y > x) {
 		y = x - 1;
 	}
-	bool b2 = x - y > 0; // GOOD [FALSE POSITIVE]
+	bool b2 = x - y > 0; // GOOD
 
 	int N = getAnInt();
 	y = x;
 	while(cond()) {
 		if(unknown) { y--; }
 	}
 
-	if(x - y > 0) { } // GOOD [FALSE POSITIVE]
+	if(x - y > 0) { } // GOOD
 
 	x = y;
 	while(cond()) {
 		if(unknown) break;
 		y--;
 	}
 
-	if(x - y > 0) { } // GOOD [FALSE POSITIVE]
+	if(x - y > 0) { } // GOOD
 
 	y = 0;
 	for(int i = 0; i < x; ++i) {
@@ -66,12 +66,235 @@ void test(unsigned x, unsigned y, bool unknown) {
 		if(unknown) { x++; }
 	}
 
-	if(x - y > 0) { } // GOOD [FALSE POSITIVE]
+	if(x - y > 0) { } // GOOD
 
 	int n = getAnInt();
 	if (n > x - y) { n = x - y; }
 	if (n > 0) {
   	y += n; // NOTE: `n` is at most `x - y` at this point.
   	if (x - y > 0) {} // GOOD [FALSE POSITIVE]
 	}
-}
+}
+
+void test2() {
+	unsigned int a = getAnInt();
+	unsigned int b = a;
+
+	if (a - b > 0) { // GOOD (as a = b)
+		// ...
+	}
+}
+
+void test3() {
+	unsigned int a = getAnInt();
+	unsigned int b = a - 1;
+
+	if (a - b > 0) { // GOOD (as a >= b)
+		// ...
+	}
+}
+
+void test4() {
+	unsigned int a = getAnInt();
+	unsigned int b = a + 1;
+
+	if (a - b > 0) { // BAD
+		// ...
+	}
+}
+
+void test5() {
+	unsigned int b = getAnInt();
+	unsigned int a = b;
+
+	if (a - b > 0) { // GOOD (as a = b)
+		// ...
+	}
+}
+
+void test6() {
+	unsigned int b = getAnInt();
+	unsigned int a = b + 1;
+
+	if (a - b > 0) { // GOOD (as a >= b)
+		// ...
+	}
+}
+
+void test7() {
+	unsigned int b = getAnInt();
+	unsigned int a = b - 1;
+
+	if (a - b > 0) { // BAD
+		// ...
+	}
+}
+
+void test8() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (a - b > 0) { // BAD
+		// ...
+	}
+
+	if (a >= b) { // GOOD
+		if (a - b > 0) { // GOOD (as a >= b)
+			// ...
+		}
+	} else {
+		if (a - b > 0) { // BAD
+			// ...
+		}
+	}
+
+	if (b >= a) { // GOOD
+		if (a - b > 0) { // BAD
+			// ...
+		}
+	} else {
+		if (a - b > 0) { // GOOD (as a > b)
+			// ...
+		}
+	}
+
+	while (a >= b) { // GOOD
+		if (a - b > 0) { // GOOD (as a >= b)
+			// ...
+		}
+	}
+
+	if (a < b) return;
+
+	if (a - b > 0) { // GOOD (as a >= b)
+		// ...
+	}
+}
+
+void test9() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (a < b) {
+		b = 0;
+	}
+
+	if (a - b > 0) { // GOOD (as a >= b) [FALSE POSITIVE]
+		// ...
+	}
+}
+
+void test10() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (a < b) {
+		a = b;
+	}
+
+	if (a - b > 0) { // GOOD (as a >= b)
+		// ...
+	}
+}
+
+void test11() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (a < b) return;
+
+	b = getAnInt();
+
+	if (a - b > 0) { // BAD
+		// ...
+	}
+}
+
+void test12() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+	unsigned int c;
+
+	if ((b <= c) && (c <= a)) {
+		if (a - b > 0) { // GOOD (as b <= a)
+			// ...
+		}
+	}
+
+	if (b <= c) {
+		if (c <= a) {
+			if (a - b > 0) { // GOOD (as b <= a)
+				// ...
+			}
+		}
+	}
+}
+
+int test13() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (b != 0) {
+		return 0;
+	} // b = 0
+
+	return (a - b > 0); // GOOD (as b = 0)
+}
+
+int test14() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (!b) {
+		return 0;
+	} // b != 0
+
+	return (a - b > 0); // BAD
+}
+
+struct Numbers
+{
+	unsigned int a, b;
+};
+
+int test15(Numbers *n) {
+
+	if (!n) {
+		return 0;
+	}
+
+	return (n->a - n->b > 0); // BAD
+}
+
+int test16() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (!b) {
+		return 0;
+	} else {
+		return (a - b > 0); // BAD
+	}
+}
+
+int test17() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (b == 0) {
+		return 0;
+	} // b != 0
+
+	return (a - b > 0); // BAD
+}
+
+int test18() {
+	unsigned int a = getAnInt();
+	unsigned int b = getAnInt();
+
+	if (b) {
+		return 0;
+	} // b == 0
+
+	return (a - b > 0); // GOOD (as b = 0)
+}