Merge pull request #17037 from geoffw0/sizecheck

C++: Fix issue with cpp/suspicious-allocation-size

cpp/ql/src/Critical/SizeCheck2.ql

@@ -15,6 +15,7 @@
 
 import cpp
 import semmle.code.cpp.models.Models
+import semmle.code.cpp.commons.Buffer
 
 predicate baseType(AllocationExpr alloc, Type base) {
   exists(PointerType pointer |
@@ -30,7 +31,8 @@ predicate baseType(AllocationExpr alloc, Type base) {
 }
 
 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.
+  // If the codebase has more than one type with the same name, it can have more than one size. For
+  // most purposes in this query, we use the smallest.
   size = min(t.getSize())
 }
 
@@ -45,7 +47,8 @@ where
     size = 0 or
     (allocated / size) * size = allocated
   ) and
-  not basesize > allocated // covered by SizeCheck.ql
+  not basesize > allocated and // covered by SizeCheck.ql
+  not memberMayBeVarSize(base.getUnspecifiedType(), _) // exclude variable size types
 select alloc,
   "Allocated memory (" + allocated.toString() + " bytes) is not a multiple of the size of '" +
     base.getName() + "' (" + basesize.toString() + " bytes)."

cpp/ql/src/change-notes/2024-07-22-suspicious-allocation-size.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/suspicious-allocation-size` ("Not enough memory allocated for array of pointer type") query no longer produces false positives on "variable size" `struct`s.

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

@@ -2,3 +2,4 @@
 | 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). |
+| test2.c:85:24:85:29 | call to malloc | Allocated memory (1159 bytes) is not a multiple of the size of 'MyFixedStruct' (1032 bytes). |

cpp/ql/test/query-tests/Critical/SizeCheck/test.c

@@ -60,7 +60,7 @@ void test_union() {
 }
 
 // --- custom allocators ---
- 
+
 void *MyMalloc1(size_t size) { return malloc(size); }
 void *MyMalloc2(size_t size);
 

cpp/ql/test/query-tests/Critical/SizeCheck/test2.c

@@ -44,7 +44,7 @@ void good1(void) {
 }
 
 // --- custom allocators ---
- 
+
 void *MyMalloc1(size_t size) { return malloc(size); }
 void *MyMalloc2(size_t size);
 
@@ -53,3 +53,34 @@ void customAllocatorTests()
     double *dptr1 = MyMalloc1(33); // BAD -- Not a multiple of sizeof(double) [NOT DETECTED]
     double *dptr2 = MyMalloc2(33); // BAD -- Not a multiple of sizeof(double) [NOT DETECTED]
 }
+
+// --- variable length data structures ---
+
+typedef unsigned char uint8_t;
+
+typedef struct _MyVarStruct1 {
+    size_t dataLen;
+    uint8_t data[0];
+} MyVarStruct1;
+
+typedef struct _MyVarStruct2 {
+    size_t dataLen;
+    uint8_t data[1];
+} MyVarStruct2;
+
+typedef struct _MyVarStruct3 {
+    size_t dataLen;
+    uint8_t data[];
+} MyVarStruct3;
+
+typedef struct _MyFixedStruct {
+    size_t dataLen;
+    uint8_t data[1024];
+} MyFixedStruct;
+
+void varStructTests() {
+    MyVarStruct1 *a = malloc(sizeof(MyVarStruct1) + 127); // GOOD
+    MyVarStruct2 *b = malloc(sizeof(MyVarStruct2) + 127); // GOOD
+    MyVarStruct3 *c = malloc(sizeof(MyVarStruct3) + 127); // GOOD
+    MyFixedStruct *d = malloc(sizeof(MyFixedStruct) + 127); // BAD --- Not a multiple of sizeof(MyFixedStruct)
+}