Merge pull request #4527 from geoffw0/odasa3940

C++: Improve SizeCheck queries

Author: jbj

cpp/change-notes/2020-10-21-size-check-queries.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The 'Not enough memory allocated for pointer type' (cpp/allocation-too-small) and 'Not enough memory allocated for array of pointer type' (cpp/suspicious-allocation-size) queries have been improved. Previously some allocations would be reported by both queries, this no longer occurs. In addition more allocation functions are now understood by both queries.

cpp/ql/src/Critical/SizeCheck.ql

@@ -13,30 +13,9 @@
  */
 
 import cpp
+import semmle.code.cpp.models.Models
 
-class Allocation extends FunctionCall {
-  Allocation() {
-    exists(string name |
-      this.getTarget().hasGlobalOrStdName(name) and
-      (name = "malloc" or name = "calloc" or name = "realloc")
-    )
-  }
-
-  private string getName() { this.getTarget().hasGlobalOrStdName(result) }
-
-  int getSize() {
-    this.getName() = "malloc" and
-    this.getArgument(0).getValue().toInt() = result
-    or
-    this.getName() = "realloc" and
-    this.getArgument(1).getValue().toInt() = result
-    or
-    this.getName() = "calloc" and
-    result = this.getArgument(0).getValue().toInt() * this.getArgument(1).getValue().toInt()
-  }
-}
-
-predicate baseType(Allocation alloc, Type base) {
+predicate baseType(AllocationExpr alloc, Type base) {
   exists(PointerType pointer |
     pointer.getBaseType() = base and
     (
@@ -54,11 +33,12 @@ predicate decideOnSize(Type t, int size) {
   size = min(t.getSize())
 }
 
-from Allocation alloc, Type base, int basesize, int allocated
+from AllocationExpr alloc, Type base, int basesize, int allocated
 where
   baseType(alloc, base) and
-  allocated = alloc.getSize() and
+  allocated = alloc.getSizeBytes() and
   decideOnSize(base, basesize) and
+  alloc.(FunctionCall).getTarget() instanceof AllocationFunction and // exclude `new` and similar
   basesize > allocated
 select alloc,
   "Type '" + base.getName() + "' is " + basesize.toString() + " bytes, but only " +

cpp/ql/src/Critical/SizeCheck2.ql

@@ -13,25 +13,9 @@
  */
 
 import cpp
+import semmle.code.cpp.models.Models
 
-class Allocation extends FunctionCall {
-  Allocation() { this.getTarget().hasGlobalOrStdName(["malloc", "calloc", "realloc"]) }
-
-  private string getName() { this.getTarget().hasGlobalOrStdName(result) }
-
-  int getSize() {
-    this.getName() = "malloc" and
-    this.getArgument(0).getValue().toInt() = result
-    or
-    this.getName() = "realloc" and
-    this.getArgument(1).getValue().toInt() = result
-    or
-    this.getName() = "calloc" and
-    result = this.getArgument(0).getValue().toInt() * this.getArgument(1).getValue().toInt()
-  }
-}
-
-predicate baseType(Allocation alloc, Type base) {
+predicate baseType(AllocationExpr alloc, Type base) {
   exists(PointerType pointer |
     pointer.getBaseType() = base and
     (
@@ -44,16 +28,23 @@ predicate baseType(Allocation alloc, Type base) {
   )
 }
 
-from Allocation alloc, Type base, int basesize, int allocated
+predicate decideOnSize(Type t, int size) {
+  // If the codebase has more than one type with the same name, it can have more than one size.
+  size = min(t.getSize())
+}
+
+from AllocationExpr alloc, Type base, int basesize, int allocated
 where
   baseType(alloc, base) and
-  allocated = alloc.getSize() and
+  allocated = alloc.getSizeBytes() and
+  decideOnSize(base, basesize) and
+  alloc.(FunctionCall).getTarget() instanceof AllocationFunction and // exclude `new` and similar
   // If the codebase has more than one type with the same name, check if any matches
   not exists(int size | base.getSize() = size |
     size = 0 or
     (allocated / size) * size = allocated
   ) and
-  basesize = min(base.getSize())
+  not basesize > allocated // covered by SizeCheck.ql
 select alloc,
   "Allocated memory (" + allocated.toString() + " bytes) is not a multiple of the size of '" +
     base.getName() + "' (" + basesize.toString() + " bytes)."

…E/CWE-131/semmle/SizeCheck/test.expected …ts/Critical/SizeCheck/SizeCheck.expectedcpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/SizeCheck/test.expected renamed to cpp/ql/test/query-tests/Critical/SizeCheck/SizeCheck.expected cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/SizeCheck/test.expected renamed to cpp/ql/test/query-tests/Critical/SizeCheck/SizeCheck.expected

@@ -2,3 +2,4 @@
 | test.c:17:20:17:25 | call to malloc | Type 'double' is 8 bytes, but only 5 bytes are allocated. |
 | test.c:32:19:32:24 | call to malloc | Type 'float' is 4 bytes, but only 2 bytes are allocated. |
 | test.c:33:20:33:25 | call to malloc | Type 'double' is 8 bytes, but only 4 bytes are allocated. |
+| test.c:59:15:59:20 | call to malloc | Type 'MyUnion' is 128 bytes, but only 8 bytes are allocated. |

…/CWE/CWE-131/semmle/SizeCheck/test.qlref …tests/Critical/SizeCheck/SizeCheck.qlrefcpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/SizeCheck/test.qlref renamed to cpp/ql/test/query-tests/Critical/SizeCheck/SizeCheck.qlref cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/SizeCheck/test.qlref renamed to cpp/ql/test/query-tests/Critical/SizeCheck/SizeCheck.qlref

@@ -0,0 +1,4 @@
+| test2.c:16:23:16:28 | call to malloc | Allocated memory (27 bytes) is not a multiple of the size of 'long long' (8 bytes). |
+| test2.c:17:20:17:25 | call to malloc | Allocated memory (33 bytes) is not a multiple of the size of 'double' (8 bytes). |
+| test2.c:32:23:32:28 | call to malloc | Allocated memory (28 bytes) is not a multiple of the size of 'long long' (8 bytes). |
+| test2.c:33:20:33:25 | call to malloc | Allocated memory (20 bytes) is not a multiple of the size of 'double' (8 bytes). |

cpp/ql/test/query-tests/Critical/SizeCheck/SizeCheck2.expected

@@ -43,5 +43,18 @@ void good1(void) {
     free(dptr);
 }
 
-
-
+typedef struct _myStruct
+{
+	int x, y;
+} MyStruct;
+
+typedef union _myUnion
+{
+	MyStruct ms;
+	char data[128];
+} MyUnion;
+
+void test_union() {
+	MyUnion *a = malloc(sizeof(MyUnion)); // GOOD
+	MyUnion *b = malloc(sizeof(MyStruct)); // BAD (too small)
+}