C++: Tighten up the heuristic in cpp/unterminated-variadic-call.

Author: geoffw0

cpp/ql/src/Security/CWE/CWE-121/UnterminatedVarargsCall.ql

@@ -56,14 +56,15 @@ class VarargsFunction extends Function {
     result = strictcount(FunctionCall fc | fc = this.getACallToThisFunction())
   }
 
-  string normalTerminator(int cnt) {
+  string normalTerminator(int cnt, int totalCount) {
+    // the terminator is 0 or -1
     result = ["0", "-1"] and
-    cnt = this.trailingArgValueCount(result) and
-    2 * cnt > this.totalCount() and
-    not exists(FunctionCall fc, int index |
-      // terminator value is used in a non-terminating position
-      this.nonTrailingVarArgValue(fc, index) = result
-    )
+    // at least 80% of calls have the terminator
+    cnt = trailingArgValueCount(result) and
+    totalCount = totalCount() and
+    100 * cnt / totalCount >= 80 and
+    // terminator value is not used in a non-terminating position
+    not exists(FunctionCall fc, int index | nonTrailingVarArgValue(fc, index) = result)
   }
 
   predicate isWhitelisted() {
@@ -73,12 +74,12 @@ class VarargsFunction extends Function {
   }
 }
 
-from VarargsFunction f, FunctionCall fc, string terminator, int cnt
+from VarargsFunction f, FunctionCall fc, string terminator, int cnt, int totalCount
 where
-  terminator = f.normalTerminator(cnt) and
+  terminator = f.normalTerminator(cnt, totalCount) and
   fc = f.getACallToThisFunction() and
   not normalisedExprValue(f.trailingArgumentIn(fc)) = terminator and
   not f.isWhitelisted()
 select fc,
-  "Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " calls do).", f,
-  f.getQualifiedName()
+  "Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " of " +
+    totalCount + " calls do).", f, f.getQualifiedName()

cpp/ql/test/query-tests/Security/CWE/CWE-121/semmle/tests/UnterminatedVarargsCall.expected

@@ -1,9 +1,2 @@
-| more_tests.cpp:23:2:23:12 | call to myFunction2 | Calls to $@ should use the value -1 as a terminator (4 calls do). | more_tests.cpp:5:6:5:16 | myFunction2 | myFunction2 |
-| more_tests.cpp:34:2:34:12 | call to myFunction4 | Calls to $@ should use the value 0 as a terminator (3 calls do). | more_tests.cpp:7:6:7:16 | myFunction4 | myFunction4 |
-| more_tests.cpp:44:2:44:12 | call to myFunction6 | Calls to $@ should use the value 0 as a terminator (3 calls do). | more_tests.cpp:9:6:9:16 | myFunction6 | myFunction6 |
-| more_tests.cpp:55:2:55:12 | call to myFunction7 | Calls to $@ should use the value 0 as a terminator (7 calls do). | more_tests.cpp:10:6:10:16 | myFunction7 | myFunction7 |
-| more_tests.cpp:56:2:56:12 | call to myFunction7 | Calls to $@ should use the value 0 as a terminator (7 calls do). | more_tests.cpp:10:6:10:16 | myFunction7 | myFunction7 |
-| tests.c:34:2:34:3 | call to f1 | Calls to $@ should use the value 0 as a terminator (4 calls do). | tests.c:4:6:4:7 | f1 | f1 |
-| tests.c:67:2:67:3 | call to f6 | Calls to $@ should use the value -1 as a terminator (3 calls do). | tests.c:24:6:24:7 | f6 | f6 |
-| tests.c:68:2:68:3 | call to f6 | Calls to $@ should use the value -1 as a terminator (3 calls do). | tests.c:24:6:24:7 | f6 | f6 |
-| tests.c:73:2:73:3 | call to f7 | Calls to $@ should use the value 0 as a terminator (3 calls do). | tests.c:28:6:28:7 | f7 | f7 |
+| more_tests.cpp:23:2:23:12 | call to myFunction2 | Calls to $@ should use the value -1 as a terminator (4 of 5 calls do). | more_tests.cpp:5:6:5:16 | myFunction2 | myFunction2 |
+| tests.c:34:2:34:3 | call to f1 | Calls to $@ should use the value 0 as a terminator (4 of 5 calls do). | tests.c:4:6:4:7 | f1 | f1 |

cpp/ql/test/query-tests/Security/CWE/CWE-121/semmle/tests/more_tests.cpp

@@ -13,35 +13,35 @@ int main()
 {
 	int x;
 
-	myFunction1("%i", 0); // not common enough to be assumed a terminator
+	myFunction1("%i", 0); // GOOD: not common enough to be assumed a terminator
 	myFunction1("%i", x);
 
 	myFunction2(-1);
 	myFunction2(0, -1);
 	myFunction2(0, 1, -1);
 	myFunction2(0, 1, 2, -1);
-	myFunction2(0, 1, 2, 3); // missing terminator
+	myFunction2(0, 1, 2, 3); // BAD: missing terminator [NOT DETECTED]
 
 	myFunction3(-1);
 	myFunction3(0, -1);
-	myFunction3(-1, 1, -1); // -1 isn't a terminator because it's used in a non-terminal position
+	myFunction3(-1, 1, -1); // GOOD: -1 isn't a terminator because it's used in a non-terminal position
 	myFunction3(0, 1, 2, -1);
 	myFunction3(0, 1, 2, 3);
 
 	myFunction4(x, x, 0);
 	myFunction4(0, x, 1, 0);
 	myFunction4(0, 0, 1, 1, 0);
-	myFunction4(x, 0, 1, 1, 1); // missing terminator
+	myFunction4(x, 0, 1, 1, 1); // BAD: missing terminator [NOT DETECTED]
 
-	myFunction5('a', 'b', 'c', 0); // ambiguous terminator
+	myFunction5('a', 'b', 'c', 0); // GOOD: ambiguous terminator
 	myFunction5('a', 'b', 'c', 0);
 	myFunction5('a', 'b', 'c', 0);
 	myFunction5('a', 'b', 'c', -1);
 	myFunction5('a', 'b', 'c', -1);
 	myFunction5('a', 'b', 'c', -1);
 
 	myFunction6(0.0);
-	myFunction6(1.0); // missing terminator
+	myFunction6(1.0); // BAD: missing terminator [NOT DETECTED]
 	myFunction6(1.0, 2.0, 0.0);
 	myFunction6(1.0, 2.0, 3.0, 0.0);
 
@@ -52,8 +52,8 @@ int main()
 	myFunction7("one", "two", "three", 0);
 	myFunction7("alpha", "beta", "gamma", 0);
 	myFunction7("", 0);
-	myFunction7("yes", "no"); // missing terminator
-	myFunction7(); // missing terminator
+	myFunction7("yes", "no"); // BAD: missing terminator [NOT DETECTED]
+	myFunction7(); // BAD: missing terminator [NOT DETECTED]
 
 	return 0;
 }

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

@@ -64,13 +64,13 @@ int main(int argc, char *argv[])
 	f6("fsdf", 3, 8, -1);
 	f6("a", 7, 9, 10, -1);
 	f6("a", 1, 22, 6, 17, 2, -1);
-	f6("fgasfgas", 5, 6, argc); // BAD: not (necessarily) terminated with -1
-	f6("sadfsaf"); // BAD: not terminated with -1
+	f6("fgasfgas", 5, 6, argc); // BAD: not (necessarily) terminated with -1 [NOT DETECTED]
+	f6("sadfsaf"); // BAD: not terminated with -1 [NOT DETECTED]
 
 	f7("", 0);
 	f7("", 0);
 	f7("", 0);
-	f7(""); // BAD: not terminated with 0
+	f7(""); // BAD: not terminated with 0 [NOT DETECTED]
 
 	return 0;
 }