C++: Use existing getSize / getRootType to find more generous bounds for arrays inside classes (though it sometimes fails, costing us TPs).

Author: geoffw0

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

@@ -105,7 +105,8 @@ private int getSize(VariableAccess va) {
 private int isSource(Expr bufferExpr, Element why) {
   exists(Variable bufferVar | bufferVar = bufferExpr.(VariableAccess).getTarget() |
     // buffer is a fixed size array
-    result = bufferVar.getUnspecifiedType().(ArrayType).getSize() and
+    exists(bufferVar.getUnspecifiedType().(ArrayType).getSize()) and
+    result = getSize(bufferExpr) and // more generous than .getSize() itself, when the array is a class field or similar.
     why = bufferVar and
     not memberMayBeVarSize(_, bufferVar) and
     not exists(BuiltInOperationBuiltInOffsetOf offsetof | offsetof.getAChild*() = bufferExpr) and

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

@@ -17,7 +17,6 @@
 | tests.cpp:285:3:285:8 | call to memset | This 'memset' operation accesses 128 bytes but the $@ is only 64 bytes. | tests.cpp:283:12:283:23 | new[] | destination buffer |
 | tests.cpp:292:3:292:8 | call to memset | This 'memset' operation accesses 11 bytes but the $@ is only 10 bytes. | tests.cpp:289:8:289:12 | array | destination buffer |
 | tests.cpp:310:2:310:7 | call to memset | This 'memset' operation accesses 21 bytes but the $@ is only 20 bytes. | tests.cpp:301:10:301:14 | myVar | destination buffer |
-| tests.cpp:312:2:312:7 | call to memset | This 'memset' operation accesses 17 bytes but the $@ is only 16 bytes. | tests.cpp:298:7:298:12 | buffer | destination buffer |
 | tests.cpp:314:2:314:7 | call to memset | This 'memset' operation accesses 8 bytes but the $@ is only 4 bytes. | tests.cpp:299:6:299:10 | field | destination buffer |
 | tests.cpp:348:2:348:14 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:342:7:342:15 | charArray | array |
 | tests.cpp:351:2:351:14 | access to array | This array indexing operation accesses byte offset 10 but the $@ is only 10 bytes. | tests.cpp:342:7:342:15 | charArray | array |
@@ -83,30 +82,12 @@
 | tests.cpp:886:5:886:10 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 4 bytes. | tests.cpp:833:12:833:12 | u | destination buffer |
 | tests.cpp:887:5:887:10 | call to memset | This 'memset' operation accesses 8 bytes but the $@ is only 4 bytes. | tests.cpp:833:12:833:12 | u | destination buffer |
 | tests.cpp:888:5:888:10 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 4 bytes. | tests.cpp:833:12:833:12 | u | destination buffer |
-| tests.cpp:913:2:913:17 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:915:2:915:17 | access to array | This array indexing operation accesses byte offset 399 but the $@ is only 40 bytes. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:916:2:916:18 | access to array | This array indexing operation accesses byte offset 403 but the $@ is only 40 bytes. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:917:2:917:17 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:919:2:919:17 | access to array | This array indexing operation accesses byte offset 399 but the $@ is only 40 bytes. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:920:2:920:18 | access to array | This array indexing operation accesses byte offset 403 but the $@ is only 40 bytes. | tests.cpp:897:7:897:8 | as | array |
-| tests.cpp:924:2:924:11 | access to array | This array indexing operation accesses byte offset 43 but the $@ is only 40 bytes. | tests.cpp:903:4:903:5 | ds | array |
-| tests.cpp:927:2:927:16 | access to array | This array indexing operation accesses byte offset 4 but the $@ is only 4 bytes. | tests.cpp:902:8:902:9 | cs | array |
-| tests.cpp:928:2:928:17 | access to array | This array indexing operation accesses byte offset 39 but the $@ is only 4 bytes. | tests.cpp:902:8:902:9 | cs | array |
-| tests.cpp:929:2:929:17 | access to array | This array indexing operation accesses byte offset 40 but the $@ is only 4 bytes. | tests.cpp:902:8:902:9 | cs | array |
-| tests.cpp:932:2:932:16 | access to array | This array indexing operation accesses byte offset 4 but the $@ is only 4 bytes. | tests.cpp:902:8:902:9 | cs | array |
-| tests.cpp:934:2:934:17 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:906:11:906:12 | xs | array |
-| tests.cpp:936:2:936:17 | access to array | This array indexing operation accesses byte offset 399 but the $@ is only 40 bytes. | tests.cpp:906:11:906:12 | xs | array |
-| tests.cpp:937:2:937:18 | access to array | This array indexing operation accesses byte offset 403 but the $@ is only 40 bytes. | tests.cpp:906:11:906:12 | xs | array |
-| tests.cpp:938:2:938:17 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:906:11:906:12 | xs | array |
-| tests.cpp:940:2:940:17 | access to array | This array indexing operation accesses byte offset 399 but the $@ is only 40 bytes. | tests.cpp:906:11:906:12 | xs | array |
-| tests.cpp:941:2:941:18 | access to array | This array indexing operation accesses byte offset 403 but the $@ is only 40 bytes. | tests.cpp:906:11:906:12 | xs | array |
 | tests.cpp:984:2:984:9 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:981:6:981:8 | arr | array |
 | tests.cpp:989:2:989:9 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:981:6:981:8 | arr | array |
 | tests.cpp:994:2:994:9 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:981:6:981:8 | arr | array |
 | tests.cpp:1001:2:1001:9 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:981:6:981:8 | arr | array |
 | tests.cpp:1009:2:1009:9 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:981:6:981:8 | arr | array |
-| tests.cpp:1028:2:1028:7 | call to memset | This 'memset' operation accesses 120 bytes but the $@ is only 40 bytes. | tests.cpp:1020:12:1020:15 | arr1 | destination buffer |
-| tests.cpp:1031:2:1031:7 | call to memset | This 'memset' operation accesses 130 bytes but the $@ is only 40 bytes. | tests.cpp:1020:12:1020:15 | arr1 | destination buffer |
+| tests.cpp:1031:2:1031:7 | call to memset | This 'memset' operation accesses 130 bytes but the $@ is only 120 bytes. | tests.cpp:1020:12:1020:15 | arr1 | destination buffer |
 | 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: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 |
@@ -115,5 +96,4 @@
 | 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/OverflowStatic.expected

@@ -5,8 +5,4 @@
 | tests.cpp:245:42:245:42 | 6 | Potential buffer-overflow: 'global_array_5' has size 5 not 6. |
 | tests.cpp:351:2:351:14 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. |
 | tests.cpp:352:17:352:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. |
-| tests.cpp:927:2:927:16 | access to array | Potential buffer-overflow: 'cs' has size 4 but 'cs[4]' may be accessed here. |
-| tests.cpp:928:2:928:17 | access to array | Potential buffer-overflow: 'cs' has size 4 but 'cs[39]' may be accessed here. |
-| tests.cpp:929:2:929:17 | access to array | Potential buffer-overflow: 'cs' has size 4 but 'cs[40]' may be accessed here. |
-| tests.cpp:932:2:932:16 | access to array | Potential buffer-overflow: 'cs' has size 4 but 'cs[4]' may be accessed here. |
 | var_size_struct.cpp:103:39:103:41 | 129 | Potential buffer-overflow: 'str' has size 128 not 129. |

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

@@ -309,7 +309,7 @@ void test12()
 	memset(&myVar, 0, sizeof(myVar)); // GOOD
 	memset(&myVar, 0, sizeof(myVar) + 1); // BAD: overrun write of myVar
 	memset(myVar.buffer, 0, 16); // GOOD
-	memset(myVar.buffer, 0, 17); // BAD: overrun write of myVar.buffer
+	memset(myVar.buffer, 0, 17); // DUBIOUS: overrun write of myVar.buffer, but not out of myVar itself [NOT DETECTED]
 	memset(&(myVar.field), 0, sizeof(int)); // GOOD
 	memset(&(myVar.field), 0, sizeof(int) * 2); // BAD: overrun write of myVar.field
 
@@ -912,33 +912,33 @@ void test26() {
 
 	maa.bs[0].as[-1] = 0; // BAD: underrun write [NOT DETECTED]
 	maa.bs[0].as[0] = 0; // GOOD
-	maa.bs[0].as[99] = 0; // GOOD (overflows into bs[9]) [FALSE POSITIVE]
-	maa.bs[0].as[100] = 0; // BAD: overrun write
-	maa.bs[1].as[-1] = 0; // GOOD (underflows into bs[0]) [FALSE POSITIVE]
+	maa.bs[0].as[99] = 0; // GOOD (overflows into bs[9])
+	maa.bs[0].as[100] = 0; // BAD: overrun write [NOT DETECTED]
+	maa.bs[1].as[-1] = 0; // GOOD (underflows into bs[0])
 	maa.bs[1].as[0] = 0; // GOOD
-	maa.bs[1].as[99] = 0; // BAD: overrun write
-	maa.bs[1].as[100] = 0; // BAD: overrun write
+	maa.bs[1].as[99] = 0; // BAD: overrun write [NOT DETECTED]
+	maa.bs[1].as[100] = 0; // BAD: overrun write[ NOT DETECTED]
 
 	maa.ds[0].i = 0; // GOOD
 	maa.ds[9].i = 0; // GOOD
-	maa.ds[10].i = 0; // BAD: overrun write
+	maa.ds[10].i = 0; // BAD: overrun write [NOT DETECTED]
 	maa.ds[0].cs[0] = 0; // GOOD
 	maa.ds[0].cs[3] = 0; // GOOD
-	maa.ds[0].cs[4] = 0; // GOOD (overflows into vs[1] [FALSE POSITIVE]
-	maa.ds[0].cs[39] = 0; // GOOD (overflows into vs[9] [FALSE POSITIVE]
-	maa.ds[0].cs[40] = 0; // BAD: overrun write
+	maa.ds[0].cs[4] = 0; // GOOD (overflows into vs[1])
+	maa.ds[0].cs[39] = 0; // GOOD (overflows into vs[9])
+	maa.ds[0].cs[40] = 0; // BAD: overrun write [NOT DETECTED]
 	maa.ds[9].cs[0] = 0; // GOOD
 	maa.ds[9].cs[3] = 0; // GOOD
-	maa.ds[9].cs[4] = 0; // BAD: overrun write
+	maa.ds[9].cs[4] = 0; // BAD: overrun write [NOT DETECTED]
 
-	maa.ys[0].xs[-1] = 0; // BAD: underrun write
+	maa.ys[0].xs[-1] = 0; // BAD: underrun write [NOT DETECTED]
 	maa.ys[0].xs[0] = 0; // GOOD
-	maa.ys[0].xs[99] = 0; // GOOD (overflows into bs[9]) [FALSE POSITIVE]
-	maa.ys[0].xs[100] = 0; // BAD: overrun write
-	maa.ys[1].xs[-1] = 0; // GOOD (underflows into ys[0]) [FALSE POSITIVE]
+	maa.ys[0].xs[99] = 0; // GOOD (overflows into bs[9])
+	maa.ys[0].xs[100] = 0; // BAD: overrun write [NOT DETECTED]
+	maa.ys[1].xs[-1] = 0; // GOOD (underflows into ys[0])
 	maa.ys[1].xs[0] = 0; // GOOD
-	maa.ys[1].xs[99] = 0; // BAD: overrun write
-	maa.ys[1].xs[100] = 0; // BAD: overrun write
+	maa.ys[1].xs[99] = 0; // BAD: overrun write [NOT DETECTED]
+	maa.ys[1].xs[100] = 0; // BAD: overrun write [NOT DETECTED]
 
 	char zs[2][2];
 	zs[0][-1] = 0; // BAD: underrun write [NOT DETECTED]
@@ -1025,7 +1025,7 @@ typedef _myStruct29 myStruct29;
 void test29() {
 	myStruct29 *ptr;
 
-	memset(ptr->arr1, 0, sizeof(ptr->arr1) + sizeof(ptr->arr2)); // GOOD (overwrites arr1, arr2) [FALSE POSITIVE]
+	memset(ptr->arr1, 0, sizeof(ptr->arr1) + sizeof(ptr->arr2)); // GOOD (overwrites arr1, arr2)
 	memset(&(ptr->arr1[0]), 0, sizeof(ptr->arr1) + sizeof(ptr->arr2)); // GOOD (overwrites arr1, arr2)
 
 	memset(ptr->arr1, 0, sizeof(ptr->arr1) + sizeof(ptr->arr2) + 10); // BAD

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

@@ -96,9 +96,9 @@ void testNotVarStruct1() {
   notVarStruct1 *nvs1 = (notVarStruct1 *)malloc(sizeof(notVarStruct1) * 2);
 
   memset(nvs1->str, 0, 128); // GOOD
-  memset(nvs1->str, 0, 129); // BAD: buffer overflow
+  memset(nvs1->str, 0, 129); // DUBIOUS: buffer overflow (overflows nvs1->str but not nvs1 overall)
   memset(nvs1[1].str, 0, 128); // GOOD
-  memset(nvs1[1].str, 0, 129); // BAD: buffer overflow
+  memset(nvs1[1].str, 0, 129); // BAD: buffer overflow [NOT DETECTED]
   strncpy(nvs1->str, "Hello, world!", 128); // GOOD
   strncpy(nvs1->str, "Hello, world!", 129); // BAD
 }