C++: Simplify 'Buffer.qll' by avoiding 'asIndirectExpr'. This removes the flow from 'x' to 'x++', which makes the whole library a lot simpler.

2026-04-29 02:35:15 +02:00 · 2022-10-30 12:58:53 +01:00
parent 18d3801c92
commit aa8214addf
4 changed files with 14 additions and 96 deletions
--- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll
+++ b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll
@@ -24,18 +24,6 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
  exists(ArrayType t | t = v.getUnspecifiedType() | not t.getArraySize() > 1)
 }

-/**
- * Gets the expression associated with `n`. Unlike `n.asExpr()` this also gets the
- * expression underlying an indirect dataflow node.
- */
-private Expr getExpr(DataFlow::Node n, boolean isIndirect) {
-  result = n.asExpr() and isIndirect = false
-  or
-  result = n.asIndirectExpr() and isIndirect = true
-}
-
-private DataFlow::Node exprNode(Expr e, boolean isIndirect) { e = getExpr(result, isIndirect) }
-
 /**
 * Holds if `bufferExpr` is an allocation-like expression.
 *
@@ -85,77 +73,17 @@ private int isSource(Expr bufferExpr, Element why) {
  )
 }

-/** Holds if the value of `n2 + delta` may be equal to the value of `n1`. */
-private predicate localFlowIncrStep(DataFlow::Node n1, DataFlow::Node n2, int delta) {
-  DataFlow::localFlowStep(n1, n2) and
-  (
-    exists(IncrementOperation incr |
-      n1.asIndirectExpr() = incr.getOperand() and
-      delta = -1
-    )
-    or
-    exists(DecrementOperation decr |
-      n1.asIndirectExpr() = decr.getOperand() and
-      delta = 1
-    )
-    or
-    exists(AddExpr add, Expr e1, Expr e2 |
-      add.hasOperands(e1, e2) and
-      n1.asIndirectExpr() = e1 and
-      delta = -e2.getValue().toInt()
-    )
-    or
-    exists(SubExpr add, Expr e1, Expr e2 |
-      add.hasOperands(e1, e2) and
-      n1.asIndirectExpr() = e1 and
-      delta = e2.getValue().toInt()
-    )
-  )
-}
-
-/**
- * Holds if `n1` may flow to `n2` without passing through any back-edges.
- *
- * Back-edges are excluded to prevent infinite loops on examples like:
- * ```
- * while(true) { ++n; }
- * ```
- * which, when used in `localFlowStepRec`, would create infinite loop that continuously
- * increments the `delta` parameter.
- */
-private predicate localFlowNotIncrStep(DataFlow::Node n1, DataFlow::Node n2) {
-  not localFlowIncrStep(n1, n2, _) and
-  DataFlow::localFlowStep(n1, n2) and
-  not n1 = n2.(DataFlow::SsaPhiNode).getAnInput(true)
-}
-
 private predicate localFlowToExprStep(DataFlow::Node n1, DataFlow::Node n2) {
-  not exists([n1.asExpr(), n1.asIndirectExpr()]) and
-  localFlowNotIncrStep(n1, n2)
-}
-
-/** Holds if `mid2 + delta` may be equal to `n1`. */
-private predicate localFlowStepRec0(DataFlow::Node n1, DataFlow::Node mid2, int delta) {
-  exists(DataFlow::Node mid1, int d1, int d2 |
-    // Or we take a number of steps that adds `d1` to the pointer
-    localFlowStepRec(n1, mid1, d1) and
-    // followed by a step that adds `d2` to the pointer
-    localFlowIncrStep(mid1, mid2, d2) and
-    delta = d1 + d2
-  )
+  not exists(n2.asExpr()) and
+  DataFlow::localFlowStep(n1, n2)
 }

 /** Holds if `n2 + delta` may be equal to `n1`. */
-private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int delta) {
-  // Either we take one or more steps that doesn't modify the size of the buffer
-  localFlowNotIncrStep+(n1, n2) and
-  delta = 0
-  or
-  exists(DataFlow::Node mid2 |
-    // Or we step from `n1` to `mid2 + delta`
-    localFlowStepRec0(n1, mid2, delta) and
-    // and finally to the next `ExprNode`.
-    localFlowToExprStep*(mid2, n2)
+private predicate localFlowStepToExpr(DataFlow::Node n1, Expr e2) {
+  exists(DataFlow::Node mid, DataFlow::Node n2 |
+    localFlowToExprStep*(n1, mid) and
+    DataFlow::localFlowStep(mid, n2) and
+    n2.asExpr() = e2
  )
 }

@@ -181,9 +109,8 @@ private predicate step(Expr e1, Expr e2, int delta) {
    delta = bufferSize - parentClass.getSize()
  )
  or
-  exists(boolean isIndirect |
-    localFlowStepRec(exprNode(e1, isIndirect), exprNode(e2, isIndirect), delta)
-  )
+  localFlowStepToExpr(DataFlow::exprNode(e1), e2) and
+  delta = 0
 }

 /**
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
@@ -1,8 +1,6 @@
 | overflowdestination.cpp:46:2:46:7 | call to memcpy | This 'memcpy' operation accesses 128 bytes but the $@ is only 64 bytes. | overflowdestination.cpp:40:7:40:10 | dest | destination buffer |
-| tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
 | tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
 | tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
-| tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
 | tests.cpp:172:23:172:31 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:170:17:170:41 | {...} | array |
 | tests.cpp:176:23:176:30 | access to array | This array indexing operation accesses byte offset 31 but the $@ is only 24 bytes. | tests.cpp:170:17:170:41 | {...} | array |
 | tests.cpp:222:3:222:8 | call to memset | This 'memset' operation accesses 33 bytes but the $@ is only 32 bytes. | tests.cpp:214:8:214:14 | buffer1 | destination buffer |
@@ -38,19 +36,14 @@
 | tests.cpp:376:3:376:13 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 101 bytes. | tests.cpp:369:47:369:52 | call to malloc | array |
 | tests.cpp:446:3:446:24 | access to array | This array indexing operation accesses a negative index -3 on the $@. | tests.cpp:444:7:444:14 | intArray | array |
 | tests.cpp:454:3:454:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
-| tests.cpp:455:3:455:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:456:3:456:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
-| tests.cpp:456:3:456:15 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
-| tests.cpp:457:3:457:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:459:3:459:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:461:3:461:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:476:2:476:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:477:2:477:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
-| tests.cpp:479:2:479:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:481:2:481:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:485:2:485:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:487:2:487:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
-| tests.cpp:489:2:489:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
 | tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
 | tests.cpp:495:2:495:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
 | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer |
@@ -59,7 +52,6 @@
 | tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
 | tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
 | tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
-| tests.cpp:575:7:575:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:579:6:579:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:586:6:586:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected
@@ -1,5 +1,4 @@
 | overflowdestination.cpp:46:20:46:30 | sizeof(<expr>) | Potential buffer-overflow: 'dest' has size 64 not 128. |
-| tests.cpp:23:33:23:49 | sizeof(<expr>) | Potential buffer-overflow: 'bigbuffer' has size 10 not 20. |
 | tests.cpp:25:33:25:49 | sizeof(<expr>) | Potential buffer-overflow: 'smallbuffer' has size 10 not 20. |
 | tests.cpp:163:3:163:11 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
 | tests.cpp:164:8:164:16 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
@@ -20,7 +20,7 @@ void test1()
 	char bigbuffer[20];
 	
 	memcpy(bigbuffer, smallbuffer, sizeof(smallbuffer)); // GOOD
-	memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read
+	memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read [NOT DETECTED]
 	memcpy(smallbuffer, bigbuffer, sizeof(smallbuffer)); // GOOD
 	memcpy(smallbuffer, bigbuffer, sizeof(bigbuffer)); // BAD: over-write
 }
@@ -454,7 +454,7 @@ void test17(long long *longArray)
 		multi[-5][5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
 		multi[5][-5] = 0; // DUBIOUS: underrun write (this one is still within the bounds of the whole array)
 		multi[-5][-5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
-		multi[0][-5] = 0; // BAD: underrun write
+		multi[0][-5] = 0; // BAD: underrun write [NOT DETECTED]

 		multi[15][5] = 0; // BAD: overrun write
 		multi[5][15] = 0; // DUBIOUS: overrun write (this one is still within the bounds of the whole array)
@@ -476,7 +476,7 @@ void test18()
 	p1[-1] = 0; // BAD: underrun write
 	p2[-1] = 0; // BAD: underrun write
 	p2++;
-	p2[-1] = 0; // GOOD [FALSE POSITIVE]
+	p2[-1] = 0; // GOOD

 	p3[-1] = 0; // BAD
 	while (*p3 != 0) {
@@ -486,7 +486,7 @@ void test18()

 	p4[-1] = 0; // BAD: underrun write
 	p4++;
-	p4[-1] = 0; // GOOD [FALSE POSITIVE]
+	p4[-1] = 0; // GOOD

 	p5[-1] = 0; // BAD
 	while (*p5 != 0) {
@@ -572,7 +572,7 @@ void test21(bool cond)
 	if (cond)
 	{
 		ptr++;
-		if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0] [FALSE POSITIVE]
+		if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0]
 	} else {
 		if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1]
 	}