This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Suppress MallocChecker positives in destructors with atomics.
ClosedPublic

Authored by NoQ on Feb 26 2018, 3:18 PM.

Details

Summary

This patch is a targeted suppression heuristic for false positives MallocChecker produces when a shared / reference-counting pointer is copied (including, but not limited to, llvm::IntrusiveRefCntPtr). The program increments the reference count through an atomic fetch_add (which is often the C++11 std::atomic<T>::fetch_add() that executes the respective C11 atomic when inlined), then decrements it via fetch_sub (or via fetch_add while adding -1), then sees if the reference count is zero by comparing the value returned by fetch_sub to 1, then deletes the object if the reference count is indeed zero.

These false positives get amplified by inlining temporary destructors, but even in code without any temporary destructors these positives are reproducible, as the tests suggest.

We cannot easily model the comparison to 1 correctly: even if we model all atomic expressions precisely, the original reference count may still have been symbolic to begin with. And if we tried to assume that no overflows occur on reference counts, this would still require pattern-matching heuristics to figure out if a certain variable is a reference counter.

The proposed fix is to suppress MallocChecker positives that are caused by releasing memory in a destructor stack frame (or its children stack frames) after performing any C11 atomic fetch_add or fetch_sub in that destructor's stack frame (or its children stack frames). This is done in a visitor suppression.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Feb 26 2018, 3:18 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
527

Docstring.

2844

IMO would be slightly easier to read with logic reversed, e.g.

if (AE == dyn_cast<>(...))
   if (Op == add || Op == sub)
      for (...)
        if (...)
          return true
return false

But this is a preference and can be ignored.

2886

I'm not sure what is going on here.
We are just traversing the stack frame, finding the first destructor in the isReleased branch?

NoQ updated this revision to Diff 136020.Feb 26 2018, 6:13 PM

Address comments.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2844

Ok. Just in case, this is usually regulated via https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code but here it seems fine.

2886

Do the updated comments make it more clear?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2886

I'm still confused :( you're going up the chain of frames until you see a destructor. How do you know that memory was released in a destructor at all, or that it was released in this particular destructor?

NoQ added inline comments.Feb 26 2018, 6:40 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2886

if (isReleased(RS, RSPrev, S)) says that the pointer against which the report is thrown was released in the current node.

george.karpenkov accepted this revision.Feb 26 2018, 6:50 PM

OK I would still appreciate a comment on why you expect fetch_add and friends in the lowest destructor in the stack.

This revision is now accepted and ready to land.Feb 26 2018, 6:50 PM
NoQ updated this revision to Diff 136114.Feb 27 2018, 11:16 AM

Add one more comment.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.