Skip to content

Commit cace411

Browse files
committed
C++: NonConstantFormat taint only for string types
To speed up the taint analysis in `NonConstantFormat.ql` and to remove FPs that were due to taint spreading from `i` to `a[i]`, this commit stops the taint tracking in `NonConstantFormat.ql` at every node that could not possibly contain a string. I tested performance on Wireshark, and it's fine. Pulling out the `isSanitizerNode` prevented `isSanitizer` from turning into four half-slow RA predicates due to both CPE and `#antijoin_rhs` transformations happening.
1 parent e99c688 commit cace411

File tree

3 files changed

+22
-4
lines changed

3 files changed

+22
-4
lines changed

cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@ predicate underscoreMacro(Expr e) {
5151
)
5252
}
5353

54+
/**
55+
* Holds if `t` cannot hold a character array, directly or indirectly.
56+
*/
57+
predicate cannotContainString(Type t) {
58+
t.getUnspecifiedType() instanceof BuiltInType
59+
or
60+
t.getUnspecifiedType() instanceof IntegralOrEnumType
61+
}
62+
5463
predicate isNonConst(DataFlow::Node node) {
5564
exists(Expr e | e = node.asExpr() |
5665
exists(FunctionCall fc | fc = e.(FunctionCall) |
@@ -99,16 +108,26 @@ predicate isNonConst(DataFlow::Node node) {
99108
node instanceof DataFlow::DefinitionByReferenceNode
100109
}
101110

111+
pragma[noinline]
112+
predicate isSanitizerNode(DataFlow::Node node) {
113+
underscoreMacro(node.asExpr())
114+
or
115+
cannotContainString(node.getType())
116+
}
117+
102118
class NonConstFlow extends TaintTracking::Configuration {
103119
NonConstFlow() { this = "NonConstFlow" }
104120

105-
override predicate isSource(DataFlow::Node source) { isNonConst(source) }
121+
override predicate isSource(DataFlow::Node source) {
122+
isNonConst(source) and
123+
not cannotContainString(source.getType())
124+
}
106125

107126
override predicate isSink(DataFlow::Node sink) {
108127
exists(FormattingFunctionCall fc | sink.asExpr() = fc.getArgument(fc.getFormatParameterIndex()))
109128
}
110129

111-
override predicate isSanitizer(DataFlow::Node node) { underscoreMacro(node.asExpr()) }
130+
override predicate isSanitizer(DataFlow::Node node) { isSanitizerNode(node) }
112131
}
113132

114133
from FormattingFunctionCall call, Expr formatString

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,3 @@
1818
| test.cpp:85:12:85:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
1919
| test.cpp:90:12:90:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
2020
| test.cpp:107:12:107:24 | new[] | The format string argument to printf should be constant to prevent security issues and other potential errors. |
21-
| test.cpp:142:10:142:20 | access to array | The format string argument to printf should be constant to prevent security issues and other potential errors. |

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,5 +139,5 @@ void set_value_of(int *i);
139139
void print_ith_message() {
140140
int i;
141141
set_value_of(&i);
142-
printf(messages[i], 1U); // GOOD [FALSE POSITIVE]
142+
printf(messages[i], 1U); // GOOD
143143
}

0 commit comments

Comments
 (0)