[PATCH] D59516: [analyzer] Add custom filter functions for GenericTaintChecker

[Artem Dergachev via Phabricator via cfe-commits]

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