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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
