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

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked 7 inline comments as done.
boga95 added a comment.

Ping



================
Comment at: clang/test/Analysis/taint-generic.c:393-397
+void testConfigurationFilter3() {
+  int x = mySource1();
+  myFilter3(&x);
+  Buffer[x] = 1; // no-warning
+}
----------------
NoQ wrote:
> 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.
It could work with C++ despite it hasn't supported any specific feature (yet).

I pass it by `const int*` because it currently doesn't support the value semantics :(. It has been already in my TODO list.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:53-56
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddInteger(Arg);
+    ID.AddInteger(TagType);
+  }
----------------
Szelethus wrote:
> Interesting, isn't this predefined for `std::pair`?
Unfortunately, it's not. 


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:474-476
+  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
+  if (!FDecl || FDecl->getKind() != Decl::Function)
+    return;
----------------
Szelethus wrote:
> When do these happen? For implicit functions in C? 
For example, a lambda doesn't have an FunctionDecl.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:478-480
+  StringRef Name = C.getCalleeName(FDecl);
+  if (Name.empty())
+    return;
----------------
Szelethus wrote:
> And this? Lambdas maybe?
I haven't found any example of this. It's just a copy-paste refactoring so I don't know the intentions behind that.

I think the checker should work perfectly without it.


================
Comment at: lib/StaticAnalyzer/Checkers/Taint.h:25-28
 using TaintTagType = unsigned;
 
-static constexpr TaintTagType TaintTagGeneric = 0;
+static constexpr TaintTagType TaintTagNotTainted = 0;
+static constexpr TaintTagType TaintTagGeneric = 1;
----------------
Szelethus wrote:
> Is there a **very** good reason behind us not using an `enum` instead?
Unfortunately, enums cannot put into FoldingSet. It has to be a primitive type or a struct with the necessary functions. I can use enums and cast them to unsigned and back, but it isn't convenient.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59516/new/

https://reviews.llvm.org/D59516