Merge pull request github#3326 from Cornelius-Riemenschneider/alloc-size-mul

C++: Allocation.qll: Analyze common pattern of malloc() invocations to provide more accurate getSizeMult()

Author: geoffw0

cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll

@@ -270,6 +270,36 @@ class OperatorNewAllocationFunction extends AllocationFunction {
   }
 }
 
+/**
+ * The predicate analyzes a `sizeExpr`, which is an argument to an allocation
+ * function like malloc, and tries to split it into an expression `lengthExpr`
+ * that describes the length of the allocated array, and the size of the allocated
+ * element type `sizeof`.
+ * If this is not possible, the allocation is considered to be of size 1 and of
+ * length `sizeExpr`.
+ */
+private predicate deconstructSizeExpr(Expr sizeExpr, Expr lengthExpr, int sizeof) {
+  if
+    sizeExpr instanceof MulExpr and
+    exists(SizeofOperator sizeofOp, Expr lengthOp |
+      sizeofOp = sizeExpr.(MulExpr).getAnOperand() and
+      lengthOp = sizeExpr.(MulExpr).getAnOperand() and
+      not lengthOp instanceof SizeofOperator and
+      exists(sizeofOp.getValue().toInt())
+    )
+  then
+    exists(SizeofOperator sizeofOp |
+      sizeofOp = sizeExpr.(MulExpr).getAnOperand() and
+      lengthExpr = sizeExpr.(MulExpr).getAnOperand() and
+      not lengthExpr instanceof SizeofOperator and
+      sizeof = sizeofOp.getValue().toInt()
+    )
+  else (
+    lengthExpr = sizeExpr and
+    sizeof = 1
+  )
+}
+
 /**
  * An allocation expression that is a function call, such as call to `malloc`.
  */
@@ -287,15 +317,25 @@ class CallAllocationExpr extends AllocationExpr, FunctionCall {
     not exists(NewOrNewArrayExpr new | new.getAllocatorCall() = this)
   }
 
-  override Expr getSizeExpr() { result = getArgument(target.getSizeArg()) }
+  override Expr getSizeExpr() {
+    exists(Expr sizeExpr | sizeExpr = getArgument(target.getSizeArg()) |
+      if exists(target.getSizeMult())
+      then result = sizeExpr
+      else
+        exists(Expr lengthExpr |
+          deconstructSizeExpr(sizeExpr, lengthExpr, _) and
+          result = lengthExpr
+        )
+    )
+  }
 
   override int getSizeMult() {
     // malloc with multiplier argument that is a constant
     result = getArgument(target.getSizeMult()).getValue().toInt()
     or
     // malloc with no multiplier argument
     not exists(target.getSizeMult()) and
-    result = 1
+    deconstructSizeExpr(getArgument(target.getSizeArg()), _, result)
   }
 
   override int getSizeBytes() { result = getSizeExpr().getValue().toInt() * getSizeMult() }

cpp/ql/test/library-tests/allocators/allocators.cpp

@@ -149,3 +149,15 @@ void directOperatorCall() {
 	ptr = operator new(sizeof(int));
 	operator delete(ptr);
 }
+
+void *malloc(size_t);
+
+void testMalloc(size_t count) {
+  malloc(5);
+  malloc(5 * sizeof(int));
+  malloc(count);
+  malloc(count * sizeof(int));
+  malloc(count * sizeof(int) + 1);
+  malloc(((int) count) * sizeof(void *));
+  malloc(sizeof(void*) * sizeof(int));
+}

cpp/ql/test/library-tests/allocators/allocators.expected

@@ -55,6 +55,7 @@ allocationFunctions
 | allocators.cpp:122:7:122:20 | operator new[] | getPlacementArgument = 1, getSizeArg = 0 |
 | allocators.cpp:123:7:123:18 | operator new | getSizeArg = 0, requiresDealloc |
 | allocators.cpp:124:7:124:20 | operator new[] | getSizeArg = 0, requiresDealloc |
+| allocators.cpp:153:7:153:12 | malloc | getSizeArg = 0, requiresDealloc |
 | file://:0:0:0:0 | operator new | getSizeArg = 0, requiresDealloc |
 | file://:0:0:0:0 | operator new | getSizeArg = 0, requiresDealloc |
 | file://:0:0:0:0 | operator new[] | getSizeArg = 0, requiresDealloc |
@@ -84,6 +85,13 @@ allocationExprs
 | allocators.cpp:143:13:143:28 | new[] | getSizeBytes = 400, requiresDealloc |
 | allocators.cpp:144:13:144:31 | new[] | getSizeExpr = x, getSizeMult = 900, requiresDealloc |
 | allocators.cpp:149:8:149:19 | call to operator new | getSizeBytes = 4, getSizeExpr = sizeof(int), getSizeMult = 1, requiresDealloc |
+| allocators.cpp:156:3:156:8 | call to malloc | getSizeBytes = 5, getSizeExpr = 5, getSizeMult = 1, requiresDealloc |
+| allocators.cpp:157:3:157:8 | call to malloc | getSizeBytes = 20, getSizeExpr = 5, getSizeMult = 4, requiresDealloc |
+| allocators.cpp:158:3:158:8 | call to malloc | getSizeExpr = count, getSizeMult = 1, requiresDealloc |
+| allocators.cpp:159:3:159:8 | call to malloc | getSizeExpr = count, getSizeMult = 4, requiresDealloc |
+| allocators.cpp:160:3:160:8 | call to malloc | getSizeExpr = ... + ..., getSizeMult = 1, requiresDealloc |
+| allocators.cpp:161:3:161:8 | call to malloc | getSizeExpr = count, getSizeMult = 8, requiresDealloc |
+| allocators.cpp:162:3:162:8 | call to malloc | getSizeBytes = 32, getSizeExpr = ... * ..., getSizeMult = 1, requiresDealloc |
 deallocationFunctions
 | allocators.cpp:11:6:11:20 | operator delete | getFreedArg = 0 |
 | allocators.cpp:12:6:12:22 | operator delete[] | getFreedArg = 0 |

cpp/ql/test/query-tests/Critical/OverflowCalculated/NoSpaceForZeroTerminator.expected

@@ -1,7 +1,9 @@
 | tests1.cpp:26:21:26:26 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests1.cpp:36:21:36:26 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests1.cpp:56:21:56:27 | call to realloc | This allocation does not include space to null-terminate the string. |
 | tests1.cpp:67:21:67:26 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests1.cpp:89:25:89:30 | call to malloc | This allocation does not include space to null-terminate the string. |
+| tests1.cpp:109:25:109:30 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests3.cpp:25:21:25:31 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests3.cpp:30:21:30:31 | call to malloc | This allocation does not include space to null-terminate the string. |
 | tests3.cpp:53:17:53:44 | new[] | This allocation does not include space to null-terminate the string. |

cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected

@@ -1 +1,2 @@
 | tests2.cpp:34:4:34:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 33) |
+| tests2.cpp:52:4:52:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 51) |

cpp/ql/test/query-tests/Critical/OverflowCalculated/tests1.cpp

@@ -33,7 +33,7 @@ void tests1(int case_num)
 			break;
 
 		case 3:
-			buffer = (char *)malloc(strlen(str) * sizeof(char)); // BAD [NOT DETECTED]
+			buffer = (char *)malloc(strlen(str) * sizeof(char)); // BAD
 			strcpy(buffer, str);
 			break;
 
@@ -106,7 +106,7 @@ void tests1(int case_num)
 			break;
 
 		case 105:
-			wbuffer = (wchar_t *)malloc(wcslen(wstr) * sizeof(wchar_t)); // BAD [NOT DETECTED]
+			wbuffer = (wchar_t *)malloc(wcslen(wstr) * sizeof(wchar_t)); // BAD
 			wcscpy(wbuffer, wstr);
 			break;
 

cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp

@@ -47,7 +47,7 @@ void tests2(int case_num)
 			break;
 
 		case 4:
-			buffer = (char *)malloc((strlen(str1) + 1) * sizeof(char)); // BAD [NOT DETECTED]
+			buffer = (char *)malloc((strlen(str1) + 1) * sizeof(char)); // BAD
 			strcpy(buffer, str1);
 			strcat(buffer, str2);
 			break;

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected

@@ -3,7 +3,9 @@
 | test.c:16:20:16:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.c:32:20:32:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.c:49:20:49:25 | call to malloc | This allocation does not include space to null-terminate the string. |
+| test.c:64:20:64:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. |
+| test.cpp:31:35:31:40 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:45:28:45:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:55:28:55:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. |

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.c

@@ -60,7 +60,7 @@ void good2(char *str) {
 }
 
 void bad3(char *str) {
-    // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
+    // BAD -- Not allocating space for '\0' terminator
     char *buffer = malloc(strlen(str) * sizeof(char));
     strcpy(buffer, str);
     free(buffer);

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp

@@ -27,7 +27,7 @@ void bad1(wchar_t *wstr) {
 }
 
 void bad2(wchar_t *wstr) {
-    // BAD -- Not allocating space for '\0' terminator [NOT DETECTED]
+    // BAD -- Not allocating space for '\0' terminator
     wchar_t *wbuffer = (wchar_t *)malloc(wcslen(wstr) * sizeof(wchar_t));
     wcscpy(wbuffer, wstr);
     free(wbuffer);