[cfe-dev] Fwd: Fwd: [analyzer] false positive in loop?

[Artem Dergachev via cfe-dev]

Had a closer look.

The false positive is not occuring while analyzing loopExample(); it's 
occuring while analyzing walkthrough() as a top-level function. You 
should have noticed it when looking at the reported path (eg. in html 
report, see attached file; i'm also attaching the raw source code).

So at a glance, it's a topological-sort problem. Let's go a bit deeper 
though:


   $ clang --analyze test.cpp -Xanalyzer -analyzer-display-progress
   ANALYZE (Syntax): test.cpp IntegerList
   ANALYZE (Syntax): test.cpp ~IntegerList
   ANALYZE (Syntax): test.cpp IntegerList
   ANALYZE (Syntax): test.cpp getSize
   ANALYZE (Syntax): test.cpp at
   ANALYZE (Syntax): test.cpp walkthrough
   ANALYZE (Syntax): test.cpp hasEvenNumbers
   ANALYZE (Syntax): test.cpp loopExample
   ANALYZE (Path,  Inline_Regular): test.cpp loopExample
   ANALYZE (Path,  Inline_Regular): test.cpp walkthrough
   ANALYZE (Path,  Inline_Regular): test.cpp IntegerList
   ANALYZE (Path,  Inline_Regular): test.cpp ~IntegerList
   ANALYZE (Path,  Inline_Regular): test.cpp IntegerList
   test.cpp:37:7: warning: Attempt to free released memory
         free(obj);
         ^~~~~~~~~
   test.cpp:56:5: warning: Use of memory after it is freed
       walkthrough(list, obj);
       ^~~~~~~~~~~~~~~~~~~~~~
   2 warnings generated.


What we see there is that walkthrough() is truly analyzed as a top-level 
function. However, we shouldn't be analyzing this function as top-level, 
as it was already inlined during analysis of loopExample() (regardless 
of the fix proposed in http://reviews.llvm.org/D15410 , this case 
accidentally works correctly).

Now, was it? No, in fact, it wasn't inlined during analysis of 
loopExample(). Because there's another warning on line 56, thrown 
*before* the call (on PreStmt<CallExpr>). This warning generates a sink, 
and analysis of walkthrough() doesn't even start. So the analyzer 
decides to have a look at walkthrough separately, and notices that it 
would double-free the given pointer if at least two numbers in the list 
are even, which is, at least, a vacuous truth.

IMHO follow-up:
(1) Sinks are often evil, and this particular sink is certainly 
misplaced, as the error of sending a dead pointer to a function is not 
fatal(?)
(2) Perhaps we shouldn't be analyzing static (anonymous-namespaced, 
etc.) functions as top-level at all(?) This still needs to check 
functions exported as callbacks, probably.
(3) The code, i think, still has non-fatal problems, i'd prefer to 
refactor any code that relies on passing dead pointers around or 
inobviously freeing the same pointer multiple times depending on 
complicated computations, and i don't think it's really all that bad 
that the static analyzer warns here, though the warning is a bit unobvious.
(4) Probably add note: diagnostics to the console diagnostic printer, so 
that people who don't use HTML diagnostics (or scan-build) had a chance 
to notice that we also provide a path to look at? We may add it as a 
-cc1 option disabled by default but enabled by --analyze, so that tests 
didn't break too much.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20151222/75302c93/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.cpp
Type: text/x-c++src
Size: 1218 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20151222/75302c93/attachment.cpp>