NoQ added a comment.
I'd like the test cases to actually demonstrate the correct use of the filters
and the correct behavior of the Analyzer when the filters are annotated
correctly, but it looks to me that they either demonstrate behavior when the
annotation is //not// used correctly, or we disagree about how the taint should
work in the first place. Testing the behavior when the annotation is misplaced
is fine (with enough comments and probably FIXMEs where applicable), but
"positive" tests are more valuable because they are the actual common cases
(hopefully).
================
Comment at: clang/test/Analysis/taint-generic.c:381-385
+void testConfigurationFilter1() {
+ int x = mySource1();
+ myFilter1(&x);
+ Buffer[x] = 1; // no-warning
+}
----------------
`myFilter1` will create a new conjured symbol within `x` when evaluated
conservatively. In this case there clearly shouldn't be a warning on `Buffer[x]
= 1`, because nobody marked the new symbol as tainted to begin with. Therefore
i think that this test doesn't really test the new functionality.
================
Comment at: clang/test/Analysis/taint-generic.c:387-391
+void testConfigurationFilter2() {
+ int x = mySource1();
+ myFilter2(&x);
+ Buffer[x] = 1; // no-warning
+}
----------------
Assuming `myFilter2` is inlined, we have two execution paths here. On one of
them `x` is reset to a concrete 0. Because concrete values cannot carry taint
to begin with, this execution path doesn't test the new functionality. On the
other path `x` indeed has the old tainted value but it's now constrained to
`[0, INT_MAX]`. Because it's still not constrained to be less than `BUFSIZE`,
i'd say this looks more like a false negative and the new functionality makes
the analysis worse. So this test does test the new functionality, but it kinda
makes the new functionality look bad rather than good.
================
Comment at: clang/test/Analysis/taint-generic.c:393-397
+void testConfigurationFilter3() {
+ int x = mySource1();
+ myFilter3(&x);
+ Buffer[x] = 1; // no-warning
+}
----------------
In this example `myFilter3` promises not to alter the value of `x` due to
`const`-qualifier on the pointee type of the parameter. Additionally, the
function has no chance of preventing the program from reaching the buffer
overflow line, other than crashing the program (given that it's C and there are
no exceptions). Therefore i'd say this is also a false negative.
A better motivational example that doesn't immediately look like a false
negative may look like this:
```lang=c
void testConfigurationFilter3() {
int x = mySource1();
if (isOutOfRange(x)) // the filter function
return;
Buffer[x] = 1; // no-warning
}
```
In this case the function looks at the value of `x` (there's really no need to
pass it by pointer) and notifies the caller through its return value that it is
unsafe to access the buffer with such index. This is probably the only
situation where a "filter" annotation is actually worth it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59516/new/
https://reviews.llvm.org/D59516
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
