Merge pull request #682 from geoffw0/suspiciousaddsizeof

jbj · web-flow · commit 169bbcdfa06c · 2019-01-21T09:06:18.000+01:00
CPP: Fix false positive in SuspiciousAddWithSizeof.ql
diff --git a/change-notes/1.20/analysis-cpp.md b/change-notes/1.20/analysis-cpp.md
@@ -14,6 +14,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positives | Pointer arithmetic on `char * const` expressions (and other variations of `char *`) are now correctly excluded from the results. |
 | Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. |
 | Lossy function result cast (`cpp/lossy-function-result-cast`) | Fewer false positive results | The whitelist of rounding functions built into this query has been expanded. |
 | Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
diff --git a/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql b/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql
@@ -15,9 +15,9 @@ import IncorrectPointerScalingCommon
 
 private predicate isCharSzPtrExpr(Expr e) {
   exists (PointerType pt
-  | pt = e.getFullyConverted().getUnderlyingType()
-  | pt.getBaseType().getUnspecifiedType() instanceof CharType
-  or pt.getBaseType().getUnspecifiedType() instanceof VoidType)
+  | pt = e.getFullyConverted().getType().getUnspecifiedType()
+  | pt.getBaseType() instanceof CharType
+  or pt.getBaseType() instanceof VoidType)
 }
 
 from Expr sizeofExpr, Expr e
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/SuspiciousAddWithSizeof/SuspiciousAddWithSizeof.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/SuspiciousAddWithSizeof/SuspiciousAddWithSizeof.expected
@@ -4,3 +4,4 @@
 | test.cpp:30:25:30:35 | sizeof(int) | Suspicious sizeof offset in a pointer arithmetic expression. The type of the pointer is int *. |
 | test.cpp:38:30:38:40 | sizeof(int) | Suspicious sizeof offset in a pointer arithmetic expression. The type of the pointer is int *. |
 | test.cpp:61:27:61:37 | sizeof(int) | Suspicious sizeof offset in a pointer arithmetic expression. The type of the pointer is int *. |
+| test.cpp:89:43:89:55 | sizeof(MyABC) | Suspicious sizeof offset in a pointer arithmetic expression. The type of the pointer is myInt *const. |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/SuspiciousAddWithSizeof/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-468/semmle/SuspiciousAddWithSizeof/test.cpp
@@ -64,3 +64,32 @@ void test7(int i) {
   v = *(int *)(voidPointer + i); // GOOD (actually rather dubious, but this could be correct code)
   v = *(int *)(voidPointer + (i * sizeof(int))); // GOOD
 }
+
+typedef unsigned long size_t;
+
+void *malloc(size_t size);
+
+class MyABC
+{
+public:
+  int a, b, c;
+};
+
+typedef unsigned char myChar;
+typedef unsigned int myInt;
+
+class MyTest8Class
+{
+public:
+  MyTest8Class() :
+    myCharsPointer((myChar *)malloc(sizeof(MyABC) * 2)),
+    myIntsPointer((myInt *)malloc(sizeof(MyABC) * 2))
+  {
+    myChar *secondPtr = myCharsPointer + sizeof(MyABC); // GOOD
+    myInt *secondPtrInt = myIntsPointer + sizeof(MyABC); // BAD
+  }
+
+private:
+  myChar * const myCharsPointer;
+  myInt * const myIntsPointer;
+};
