clang-analyzer NewDeleteLeaks : best way to silence a false positive

Benoit_Belley · June 29, 2016, 12:14am

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 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 ?

Thanks,
Benoit

Benoit Belley

Sr Principal Developer

M&E-Product Development Group

MAIN +1 514 393 1616

DIRECT +1 438 448 6304

FAX +1 514 393 0110

Twitter

Facebook

Autodesk, Inc.

10 Duke Street

Montreal, Quebec, Canada H3C 2L7

www.autodesk.com

jroelofs · June 29, 2016, 1:09pm

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`?

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

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).

Jon

Benoit_Belley · June 29, 2016, 1:32pm

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

Topic		Replies	Views
Possible false positive in clang-tidy NewDeleteLeaks check clang-tidy	0	944	October 14, 2022
leaks in clang (many!) Clang Frontend	2	82	April 24, 2014
Static analyzer: possible memory leak false positive? Clang Frontend	10	114	September 28, 2012
clang-tidy false positive cplusplus.NewDeleteLeaks Clang Frontend	8	119	December 13, 2016
Suppress: Static Analysis false positive. Clang Frontend	3	84	February 25, 2010

clang-analyzer NewDeleteLeaks : best way to silence a false positive

Related topics