Page MenuHomePhabricator

[analyzer] MallocChecker: Prevent Integer Set Library false positives
ClosedPublic

Authored by Charusso on Jul 12 2019, 4:46 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Jul 12 2019, 4:46 PM
NoQ added a comment.Jul 12 2019, 5:10 PM

Oh damn, i just realized that this way we track much more pointers than before, because we cannot restrict ourselves to pointers that have been explicitly malloc()ed during analysis. After all, we don't need to see the allocation site to diagnose use-after-free.

I'm afraid that it's going to be too many pointers.

Change of plans: let's suppress the warning when our free() is done within the function that has __isl_take in its definition. So, like, ascend the chain of location contexts and check your callers when you're about to mark the pointer as released. If any of the callers contain __isl_take, mark it as escaped instead.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
56–58 ↗(On Diff #209642)

We already have Escaped, it's the same thing in practice.

2937–2942 ↗(On Diff #209642)

Charusso updated this revision to Diff 209654.Jul 12 2019, 5:32 PM
Charusso marked 2 inline comments as done.
  • Fix.
  • Move the logic to free() for better matching.
In D64680#1584076, @NoQ wrote:

Change of plans: let's suppress the warning when our free() is done within the function that has __isl_take in its definition. So, like, ascend the chain of location contexts and check your callers when you're about to mark the pointer as released. If any of the callers contain __isl_take, mark it as escaped instead.

I think if the __isl_* macro is in use it should be used in the immediate StackFrame. Btw: some magic happened and with the previous approach we did not suppress some reports, now we do. Thanks!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
56–58 ↗(On Diff #209642)

It is more strict than Escaped, also it made for the purpose of PSK_EscapeOther to force out we lost the entire pointer and do not make false warnings of use-after-free.

2937–2942 ↗(On Diff #209642)

I wanted to make it bulletproof, but your meme-proof is way more better.

Here is an example of the mentioned use-after-free by pointer-escaping as an argument:
https://llvm.org/reports/scan-build/report-DeclBase.cpp-getFromVoidPointer-0-1.html#EndPath

NoQ added a comment.Jul 12 2019, 10:33 PM

Here is an example of the mentioned use-after-free by pointer-escaping as an argument:
https://llvm.org/reports/scan-build/report-DeclBase.cpp-getFromVoidPointer-0-1.html#EndPath

Not sure how is this false positive related to that report, but this false positive looks super weird and i'd love to debug it more.

P.S. I think you should attach the report to Phabricator directly, as the link will expire as soon as these reports get regenerated.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
56–58 ↗(On Diff #209642)

How exactly is it more strict? I.e., what warnings are getting suppressed by you that aren't going to be suppressed if you use Escaped instead?

Charusso updated this revision to Diff 209849.Jul 15 2019, 7:02 AM
  • Remove unnecessary DoNothing kind.
Charusso marked 4 inline comments as done.Jul 15 2019, 7:05 AM
In D64680#1584315, @NoQ wrote:

P.S. I think you should attach the report to Phabricator directly, as the link will expire as soon as these reports get regenerated.

Luckily the stable scan-build namings are stable, so that is why I picked that handy option.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
56–58 ↗(On Diff #209642)

After some measurements the previously attached report has nothing to do with strictness, just we really miss some escaping. Reverted that.

Charusso marked an inline comment as done.Jul 15 2019, 7:07 AM
NoQ added inline comments.Jul 15 2019, 10:20 PM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2547–2548 ↗(On Diff #209849)

const auto *FD = dyn_cast<FunctionDecl>(C.getStackFrame()->getDecl()).

2549–2552 ↗(On Diff #209849)

I'm slightly worried that it'll crash when free() is being called from within a body farm.

For now it probably cannot happen because none of the bodyfarmed functions can call free() directly, but i'd anyway rather add a check that the source locations we're taking are valid.

2554–2559 ↗(On Diff #209849)

If the string is empty, it clearly cannot contain __isl_, so the first check is redundant.

2564–2566 ↗(On Diff #209849)

C.getSVal(Arg).

2569 ↗(On Diff #209849)

remove is unnecessary, we overwrite it anyway.

Charusso updated this revision to Diff 210347.Jul 17 2019, 9:21 AM
Charusso marked 9 inline comments as done.
  • Fix.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2549–2552 ↗(On Diff #209849)

Oh, I missed that, thanks! I wanted to check for everything, yes.

2554–2559 ↗(On Diff #209849)

The first check is: "We got a body and its decl?", the second check is: "We got an ISL macro?". Yea, it is kind of redundant, just I like to pack one check in one IfStmt, and now that two question merges. Also I like to make them one-liners so they are self-explanatory.

Here is an example why I like it:

// Escape pointers passed into the list, unless it's an ObjC boxed       
// expression which is not a boxable C structure.                        
if (!(isa<ObjCBoxedExpr>(Ex) &&                                          
      !cast<ObjCBoxedExpr>(Ex)->getSubExpr()                             
                              ->getType()->isRecordType()))
  • from ExprEngine::Visit() - Expr::ObjCBoxedExprClass case.
2569 ↗(On Diff #209849)

I believe in so as well, just the official code base has this semantic. I have rewritten that, see below in checkPointerEscapeAux().

NoQ added inline comments.Jul 17 2019, 4:06 PM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2549–2552 ↗(On Diff #209849)

I think this is not fixed yet. I'm thinking of something like if (!Body->getBeginLoc().isValid()) { ... }.

2569 ↗(On Diff #209849)

Yeah, and the official codebase is wrong :)

2570 ↗(On Diff #210347)

The check is unnecessary. addTransition automatically takes care of the "no changes have happened" case and throws away the transition.

Moreover, in the current patch you aren't really checking if any changes have happened: the symbol may already be escaped, so in this case the flag is set to true but the state remains the same.

Charusso updated this revision to Diff 210458.Jul 17 2019, 4:41 PM
Charusso marked 4 inline comments as done.
  • More fix.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2549–2552 ↗(On Diff #209849)

Ugh, silly mistake, thanks!

NoQ accepted this revision.Jul 17 2019, 4:49 PM

Great, thanks!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
363–364 ↗(On Diff #210458)

One last thing: let's make it obvious what does the function do.

  • It not only checks and returns a boolean value, it also adds transitions. It is very important to know that the function adds transitions so that to avoid accidental state splits.
  • In this case we're talking about a deallocation rather than allocation.
  • Technically, "not modeled" is not quite correct, as we *are* modeling it, just differently.

I suggest something like this:

/// See if deallocation happens in a suspicious context. If so, escape the pointers
/// that otherwise would have been deallocated and return true.
bool suppressDeallocationsInSuspiciousContexts(const CallExpr *CE, CheckerContext &C) const;
This revision is now accepted and ready to land.Jul 17 2019, 4:49 PM
In D64680#1590619, @NoQ wrote:

Great, thanks!

Thanks for the review! I like that new name.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 5:05 PM