Skip to content

Commit fd57153

Browse files
geoffw0geoffw0
authored
Merge pull request github#10706 from geoffw0/vaheuristic
C++: Tune cpp/unterminated-variadic-call
2 parents 60fe370 + 0598645 commit fd57153

File tree

5 files changed

+68
-38
lines changed

5 files changed

+68
-38
lines changed

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

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,29 +56,26 @@ class VarargsFunction extends Function {
5656
result = strictcount(FunctionCall fc | fc = this.getACallToThisFunction())
5757
}
5858

59-
string normalTerminator(int cnt) {
59+
string normalTerminator(int cnt, int totalCount) {
60+
// the terminator is 0 or -1
6061
result = ["0", "-1"] and
62+
// at least 80% of calls have the terminator
6163
cnt = this.trailingArgValueCount(result) and
62-
2 * cnt > this.totalCount() and
63-
not exists(FunctionCall fc, int index |
64-
// terminator value is used in a non-terminating position
65-
this.nonTrailingVarArgValue(fc, index) = result
66-
)
64+
totalCount = this.totalCount() and
65+
100 * cnt / totalCount >= 80 and
66+
// terminator value is not used in a non-terminating position
67+
not exists(FunctionCall fc, int index | this.nonTrailingVarArgValue(fc, index) = result)
6768
}
6869

69-
predicate isWhitelisted() {
70-
this.hasGlobalName("open") or
71-
this.hasGlobalName("fcntl") or
72-
this.hasGlobalName("ptrace")
73-
}
70+
predicate isWhitelisted() { this.hasGlobalName(["open", "fcntl", "ptrace", "mremap"]) }
7471
}
7572

76-
from VarargsFunction f, FunctionCall fc, string terminator, int cnt
73+
from VarargsFunction f, FunctionCall fc, string terminator, int cnt, int totalCount
7774
where
78-
terminator = f.normalTerminator(cnt) and
75+
terminator = f.normalTerminator(cnt, totalCount) and
7976
fc = f.getACallToThisFunction() and
8077
not normalisedExprValue(f.trailingArgumentIn(fc)) = terminator and
8178
not f.isWhitelisted()
8279
select fc,
83-
"Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " calls do).", f,
84-
f.getQualifiedName()
80+
"Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " of " +
81+
totalCount + " calls do).", f, f.getQualifiedName()

‎cpp/ql/src/change-notes/2022-10-06-unterminated-variadic-call.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The "Unterminated variadic call" (`cpp/unterminated-variadic-call`) query has been tuned to produce fewer false positive results.

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
| 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 |
2-
| 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 |
3-
| 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 |
4-
| 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 |
5-
| 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 |
6-
| 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 |
7-
| 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 |
8-
| 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 |
9-
| 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 |
1+
| more_tests.cpp:25:2:25:12 | call to myFunction2 | Calls to $@ should use the value -1 as a terminator (5 of 6 calls do). | more_tests.cpp:5:6:5:16 | myFunction2 | myFunction2 |
2+
| more_tests.cpp:39:2:39:12 | call to myFunction4 | Calls to $@ should use the value 0 as a terminator (5 of 6 calls do). | more_tests.cpp:7:6:7:16 | myFunction4 | myFunction4 |
3+
| more_tests.cpp:49:2:49:12 | call to myFunction6 | Calls to $@ should use the value 0 as a terminator (5 of 6 calls do). | more_tests.cpp:9:6:9:16 | myFunction6 | myFunction6 |
4+
| more_tests.cpp:64:2:64:12 | call to myFunction7 | Calls to $@ should use the value 0 as a terminator (9 of 11 calls do). | more_tests.cpp:10:6:10:16 | myFunction7 | myFunction7 |
5+
| more_tests.cpp:65:2:65:12 | call to myFunction7 | Calls to $@ should use the value 0 as a terminator (9 of 11 calls do). | more_tests.cpp:10:6:10:16 | myFunction7 | myFunction7 |
6+
| 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 |
7+
| tests.c:78:2:78:3 | call to f6 | Calls to $@ should use the value -1 as a terminator (10 of 12 calls do). | tests.c:24:6:24:7 | f6 | f6 |
8+
| tests.c:79:2:79:3 | call to f6 | Calls to $@ should use the value -1 as a terminator (10 of 12 calls do). | tests.c:24:6:24:7 | f6 | f6 |
9+
| tests.c:84:2:84:3 | call to f7 | Calls to $@ should use the value 0 as a terminator (12 of 13 calls do). | tests.c:28:6:28:7 | f7 | f7 |

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

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,47 +13,56 @@ int main()
1313
{
1414
int x;
1515

16-
myFunction1("%i", 0); // not common enough to be assumed a terminator
16+
myFunction1("%i", 0); // GOOD: not common enough to be assumed a terminator
17+
myFunction1("%i", 0);
1718
myFunction1("%i", x);
1819

1920
myFunction2(-1);
2021
myFunction2(0, -1);
2122
myFunction2(0, 1, -1);
2223
myFunction2(0, 1, 2, -1);
23-
myFunction2(0, 1, 2, 3); // missing terminator
24+
myFunction2(0, 1, 2, 3, -1);
25+
myFunction2(0, 1, 2, 3, 4); // BAD: missing terminator
2426

2527
myFunction3(-1);
2628
myFunction3(0, -1);
27-
myFunction3(-1, 1, -1); // -1 isn't a terminator because it's used in a non-terminal position
29+
myFunction3(-1, 1, -1); // GOOD: -1 isn't a terminator because it's used in a non-terminal position
2830
myFunction3(0, 1, 2, -1);
29-
myFunction3(0, 1, 2, 3);
31+
myFunction3(0, 1, 2, 3, -1);
32+
myFunction3(0, 1, 2, 3, 4);
3033

3134
myFunction4(x, x, 0);
3235
myFunction4(0, x, 1, 0);
3336
myFunction4(0, 0, 1, 1, 0);
34-
myFunction4(x, 0, 1, 1, 1); // missing terminator
37+
myFunction4(0, x, 1, 1, 1, 0);
38+
myFunction4(0, 0, 1, 1, 1, 1, 0);
39+
myFunction4(x, 0, 1, 1, 1, 1, 1); // BAD: missing terminator
3540

36-
myFunction5('a', 'b', 'c', 0); // ambiguous terminator
41+
myFunction5('a', 'b', 'c', 0); // GOOD: ambiguous terminator
3742
myFunction5('a', 'b', 'c', 0);
3843
myFunction5('a', 'b', 'c', 0);
3944
myFunction5('a', 'b', 'c', -1);
4045
myFunction5('a', 'b', 'c', -1);
4146
myFunction5('a', 'b', 'c', -1);
4247

4348
myFunction6(0.0);
44-
myFunction6(1.0); // missing terminator
49+
myFunction6(1.0); // BAD: missing terminator
4550
myFunction6(1.0, 2.0, 0.0);
4651
myFunction6(1.0, 2.0, 3.0, 0.0);
52+
myFunction6(1.0, 2.0, 3.0, 4.0, 0.0);
53+
myFunction6(1.0, 2.0, 3.0, 4.0, 5.0, 0.0);
4754

4855
myFunction7(NULL);
4956
myFunction7("hello", "world", NULL);
5057
myFunction7("apple", "banana", "pear", "mango", NULL);
5158
myFunction7("dog", "cat", "elephant", "badger", "fish", NULL);
5259
myFunction7("one", "two", "three", 0);
60+
myFunction7("four", "five", "six", 0);
61+
myFunction7("seven", "eight", "nine", 0);
5362
myFunction7("alpha", "beta", "gamma", 0);
5463
myFunction7("", 0);
55-
myFunction7("yes", "no"); // missing terminator
56-
myFunction7(); // missing terminator
64+
myFunction7("yes", "no"); // BAD: missing terminator
65+
myFunction7(); // BAD: missing terminator
5766

5867
return 0;
5968
}

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

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ int main(int argc, char *argv[])
4242

4343
// GOOD: 0 is not common enough to be sure it's a terminator
4444
f3("", 0);
45+
f3("", 0);
4546
f3("", 10);
4647

4748
// GOOD: -1 is not common enough to be sure it's a terminator
@@ -50,6 +51,9 @@ int main(int argc, char *argv[])
5051
f4("", -1);
5152
f4("", -1);
5253
f4("", -1);
54+
f4("", -1);
55+
f4("", -1);
56+
f4("", -1);
5357
f4("", 1);
5458

5559
// GOOD: no obvious required terminator
@@ -61,16 +65,32 @@ int main(int argc, char *argv[])
6165
f5("", 0);
6266
f5("", 10);
6367

64-
f6("fsdf", 3, 8, -1);
65-
f6("a", 7, 9, 10, -1);
66-
f6("a", 1, 22, 6, 17, 2, -1);
67-
f6("fgasfgas", 5, 6, argc); // BAD: not (necessarily) terminated with -1
68-
f6("sadfsaf"); // BAD: not terminated with -1
68+
f6("a", 3, 8, -1);
69+
f6("b", 7, 9, 10, -1);
70+
f6("c", 1, 22, 6, 17, 2, -1);
71+
f6("d", 1, -1);
72+
f6("e", 1, 2, -1);
73+
f6("f", 1, 2, 3, -1);
74+
f6("g", 1, 2, 3, 4, -1);
75+
f6("h", 5, -1);
76+
f6("i", 5, 6, -1);
77+
f6("j", 5, 6, 7, -1);
78+
f6("k", 5, 6, argc); // BAD: not (necessarily) terminated with -1
79+
f6("l"); // BAD: not terminated with -1
6980

7081
f7("", 0);
7182
f7("", 0);
7283
f7("", 0);
7384
f7(""); // BAD: not terminated with 0
85+
f7("", 0);
86+
f7("", 0);
87+
f7("", 0);
88+
f7("", 0);
89+
f7("", 0);
90+
f7("", 0);
91+
f7("", 0);
92+
f7("", 0);
93+
f7("", 0);
7494

7595
return 0;
7696
}

0 commit comments

Comments
 (0)