Merge pull request #11722 from MathiasVP/make-buffer.qll-unique-again

C++: Use `unique` in `getBufferSize`

Author: Robert Marsh

cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll

@@ -73,67 +73,29 @@ private int isSource(Expr bufferExpr, Element why) {
   )
 }
 
-/**
- * Holds if `e2` is an expression that is derived from `e1` such that if `e1[n]` is a
- * well-defined expression for some number `n`, then `e2[n + delta]` is also a well-defined
- * expression.
- */
-private predicate step(Expr e1, Expr e2, int delta) {
-  getBufferSizeCand0(e1) and
-  (
-    exists(Variable bufferVar, Class parentClass, VariableAccess parentPtr, int bufferSize |
-      e1 = parentPtr
-    |
-      bufferVar = e2.(VariableAccess).getTarget() and
-      // buffer is the parentPtr->bufferVar of a 'variable size struct'
-      memberMayBeVarSize(parentClass, bufferVar) and
-      parentPtr = e2.(VariableAccess).getQualifier() and
-      parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and
-      (
-        if exists(bufferVar.getType().getSize())
-        then bufferSize = bufferVar.getType().getSize()
-        else bufferSize = 0
-      ) and
-      delta = bufferSize - parentClass.getSize()
-    )
-    or
-    DataFlow::localExprFlowStep(e1, e2) and
-    delta = 0
-  )
-}
-
-pragma[nomagic]
-private predicate getBufferSizeCand0(Expr e) {
-  exists(isSource(e, _))
-  or
-  exists(Expr e0 |
-    getBufferSizeCand0(e0) and
-    step(e0, e, _)
-  )
-}
-
-/**
- * Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
- *
- * NOTE: There can be multiple `(result, why)` for a given `bufferExpr`.
- */
-private int getBufferSizeCand(Expr bufferExpr, Element why) {
-  getBufferSizeCand0(bufferExpr) and
-  (
-    result = isSource(bufferExpr, why)
-    or
-    exists(Expr e0, int delta, int size |
-      size = getBufferSizeCand(e0, why) and
-      step(e0, bufferExpr, delta) and
-      result = size + delta
-    )
-  )
-}
-
 /**
  * Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
  */
+language[monotonicAggregates]
 int getBufferSize(Expr bufferExpr, Element why) {
-  result = max( | | getBufferSizeCand(bufferExpr, _)) and
-  result = getBufferSizeCand(bufferExpr, why)
+  result = isSource(bufferExpr, why)
+  or
+  exists(Class parentClass, VariableAccess parentPtr, int bufferSize, Variable bufferVar |
+    bufferVar = bufferExpr.(VariableAccess).getTarget() and
+    // buffer is the parentPtr->bufferVar of a 'variable size struct'
+    memberMayBeVarSize(parentClass, bufferVar) and
+    why = bufferVar and
+    parentPtr = bufferExpr.(VariableAccess).getQualifier() and
+    parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and
+    result = getBufferSize(parentPtr, _) + bufferSize - parentClass.getSize()
+  |
+    if exists(bufferVar.getType().getSize())
+    then bufferSize = bufferVar.getType().getSize()
+    else bufferSize = 0
+  )
+  or
+  // dataflow (all sources must be the same size)
+  result = unique(Expr def | DataFlow::localExprFlowStep(def, bufferExpr) | getBufferSize(def, _)) and
+  // find reason
+  exists(Expr def | DataFlow::localExprFlowStep(def, bufferExpr) | exists(getBufferSize(def, why)))
 }

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected

@@ -42,25 +42,25 @@
 | 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: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: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 |
 | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:510:16:510:21 | call to malloc | destination buffer |
 | 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: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 |
 | tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer |
 | unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |
+| unions.cpp:27:2:27:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
+| unions.cpp:29:2:29:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
+| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
 | unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
 | unions.cpp:34:2:34:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:16:7:16:11 | large | destination buffer |
-| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer |
-| var_size_struct.cpp:73:3:73:9 | call to strncpy | This 'strncpy' operation may access 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer |
-| var_size_struct.cpp:87:3:87:19 | access to array | This array indexing operation accesses byte offset 67 but the $@ is only 64 bytes. | var_size_struct.cpp:82:35:82:40 | call to malloc | array |
+| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:63:8:63:11 | data | destination buffer |
+| var_size_struct.cpp:73:3:73:9 | call to strncpy | This 'strncpy' operation may access 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:63:8:63:11 | data | destination buffer |
+| var_size_struct.cpp:87:3:87:19 | access to array | This array indexing operation accesses byte offset 67 but the $@ is only 64 bytes. | var_size_struct.cpp:78:7:78:14 | elements | array |
 | var_size_struct.cpp:99:3:99:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |
 | var_size_struct.cpp:101:3:101:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |
 | var_size_struct.cpp:103:3:103:9 | call to strncpy | This 'strncpy' operation may access 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

@@ -482,7 +482,7 @@ void test18()
 	while (*p3 != 0) {
 		p3 = update(p3);
 	}
-	p3[-1] = 0; // GOOD [FALSE POSITIVE]
+	p3[-1] = 0; // GOOD
 
 	p4[-1] = 0; // BAD: underrun write
 	p4++;
@@ -492,7 +492,7 @@ void test18()
 	while (*p5 != 0) {
 		p5 = update(p5);
 	}
-	p5[-1] = 0; // GOOD [FALSE POSITIVE]
+	p5[-1] = 0; // GOOD
 }
 
 void test19(bool b)
@@ -576,7 +576,7 @@ void test21(bool cond)
 	} else {
 		if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1]
 	}
-	if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1] or buffer[0]
+	if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1] or buffer[0] [NOT DETECTED]
 
 	ptr = buffer;
 	for (i = 0; i < 2; i++)

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected

@@ -17,5 +17,6 @@
 | tests.c:186:3:186:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 2 bytes. |
 | tests.c:189:3:189:9 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. |
 | unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. |
+| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 15 bytes. |
 | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. |
 | var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. |