[cfe-dev] clang-analyzer NewDeleteLeaks : best way to silence a false positive

[Benoit Belley via cfe-dev]

On 16-06-29 09:09, "Jonathan Roelofs" <jonathan at codesourcery.com> wrote:

>
>
>On 6/28/16 6:14 PM, Benoit Belley via cfe-dev wrote:
>> Hi everyone,
>>
>> The clang static analyzer gives a false positive warning on the
>> following program:
>>
>>     $ cat falseLeak.cpp
>>     struct CtrlBlk {
>>         explicit CtrlBlk() : m_useCount(1) {}
>>
>>         virtual void dispose();
>>
>>         void decUseCount() {
>>             -- m_useCount;
>>             if (m_useCount == 0) { dispose(); }
>>         }
>>
>>         int m_useCount;
>>     };
>>
>>     struct Ptr {
>>         Ptr() { m_cntrlBlk = new CtrlBlk; }
>>         ~Ptr() { m_cntrlBlk->decUseCount(); }
>>         CtrlBlk* m_cntrlBlk;
>>     };
>>
>>     void testCase() { Ptr px; }
>>
>>
>>     $ clang-tidy -checks=* falseLeak.cpp
>>     1 warning generated.
>>     /Users/benoit/tmp/falseLeak.cpp:20:27: warning: Potential leak of
>>     memory pointed to by 'px.m_cntrlBlk'
>>     [clang-analyzer-cplusplus.NewDeleteLeaks]
>>     void testCase() { Ptr px; }
>>                               ^
>>     /Users/benoit/tmp/falseLeak.cpp:20:23: note: Calling default
>>     constructor for 'Ptr'
>>     void testCase() { Ptr px; }
>>                           ^
>>     /Users/benoit/tmp/falseLeak.cpp:15:26: note: Memory is allocated
>>         Ptr() { m_cntrlBlk = new CtrlBlk; }
>>                              ^
>>     /Users/benoit/tmp/falseLeak.cpp:20:23: note: Returning from default
>>     constructor for 'Ptr'
>>     void testCase() { Ptr px; }
>>                           ^
>>     /Users/benoit/tmp/falseLeak.cpp:20:27: note: Potential leak of
>>     memory pointed to by 'px.m_cntrlBlk'
>>     void testCase() { Ptr px; }
>>                               ^
>>
>>
>> Here¹s my solution to silence up that false positive:
>>
>>     #ifdef __clang_analyzer__
>>         /// \brief Escape a variable from the analysis of the clang
>>     static analyzer
>>         ///
>>         /// This function takes a reference to a variable as an
>>     argument. This
>>         /// causes the static analyzer tool to notice that the variable
>>     has escape
>>         /// the scope of its analysis. It forces the static analyzer to
>>     make very
>>         /// conservative assumptions about the state of the variable.
>>     This also
>>         /// includes assumptions about any memory location referenced by
>>     this
>>         /// variable.
>>         ///
>>         /// This can be used to silence up false positives. In general,
>>     it is
>>         /// better to augment the information available to the static
>>     analyzer
>>         /// using attributes and/or assertions. Unfortunately, this
>>     isn't always
>>         /// possible. For example, it might be impossible to describe
>>     that a given
>>         /// object is destructed along all execution paths.
>>         template <typename V> static void
>>     escapeFromClangStaticAnalysis(V& escapedVar);
>>     #endif
>>
>>     struct CtrlBlk {
>>         explicit CtrlBlk() : m_useCount(1) {}
>>
>>         virtual void dispose();
>>
>>         void decUseCount() {
>>             -- m_useCount;
>>             if (m_useCount == 0) { dispose(); }
>>     #ifdef __clang_analyzer__
>>             escapeFromClangStaticAnalysis(m_useCount);
>>     #endif
>>         }
>>
>>         int m_useCount;
>>     };
>>
>>     struct Ptr {
>>         Ptr() { m_cntrlBlk = new CtrlBlk; }
>>         ~Ptr() { m_cntrlBlk->decUseCount(); }
>>         CtrlBlk* m_cntrlBlk;
>>     };
>>
>>     void testCase() { Ptr px; }
>>
>>
>> Would anyone have a better solution ?
>
>Does dispose() do `delete this`?


Yes. It does care of calling `delete this`.

This is a strip down test case. The actual code implements shared
ownership (i.e. somewhat mimicking std::shared_ptr). The actual full-blown
code is more involved. The control block can also be allocated using the
equivalent of std::make_shared() in which case it is allocated along with
the original object.


>
>If so, I think the first problem is that the analyzer cannot possibly
>see that it does in this example.

Correct. This is exactly what the problem is.

To avoid reporting the false positive, the static analyzer would to
construct a proof that eventually the use count has to reach zero and that
this will cause to owned object to go away (along with its control block).
This is theorem prover territory...


>
>Secondly, while legal, that's a very strange pattern, and it prevents
>you from using CtrlBlk with new[], placement new, as a stack variable,
>etc (i.e. if you call dispose(), you're only allowed to use it with
>plain old new).

It¹s the type erasure pattern. This follows the very same implementation
strategy as most implementation of std::shared_ptr.

For example, see 
https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3684.
__shared_count is equivalent to my control block. on_zero_shared_count()
is similar to my dispose(). Etc...

>
>
>Jon

Cheers,
Benoit