llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.24k stars 12.07k forks source link

clang-analyzer-cplusplus.NewDeleteLeaks false positive in C++17 with std::unique_pointer in std::tuple from libstdc++ #47814

Open gumb0 opened 3 years ago

gumb0 commented 3 years ago
Bugzilla Link 48470
Version 11.0
OS Linux
CC @chfast,@devincoughlin,@haoNoQ

Extended Description

For the following code the warning is generated about potential memory leak of unique_ptr's data returnd in a tuple and put into a struct, but also some warning about vector's data leaked, I believe both are false positives.

This is the most minimal code I was able to produce, the issues disappear when I delete anything inside fn().

#include <memory>
#include <vector>

struct I
{
    std::unique_ptr<unsigned> m;
};

static std::tuple<std::unique_ptr<unsigned>, unsigned> alloc1()
{
    return {std::make_unique<unsigned>(1), 0};
}

static std::tuple<std::unique_ptr<unsigned>, unsigned> alloc2()
{
    const std::vector<unsigned> v1;
    const std::vector<unsigned> v2;

    if (v1.size() == 1)
    {
        if (v1[0] > 0)
            throw 1;

        return {std::make_unique<unsigned>(1), 0};
    }
    else if (v2.size() == 1)
    {
        if (v2[0] > 0)
            throw 2;

        return {std::make_unique<unsigned>(1), 0};
    }
    else
        return {std::unique_ptr<unsigned>{}, 0};
}

struct V
{
    unsigned m;
    V(unsigned v) : m{v} {}
};

static V check(unsigned index)
{
    const std::vector<unsigned> v1;
    const std::vector<unsigned> v2;
    if (index >= v2.size() + v1.size())
        return {0u};
    else if (index < v2.size())
        return {v2[index]};
    else
        return {v1[index - v2.size()]};
}

static void fn()
{
    auto [m1, ignore1] = alloc1();
    I i2{std::move(m1)};

    std::vector<unsigned> v1;
    for (auto const& _ : std::vector<unsigned>{})
        v1.emplace_back(0);

    auto [m2, ignore2] = alloc2();
    auto [m3, ignore3] = alloc2();

    auto buff{std::make_unique<int[]>(100)};
    for (const auto& data : std::vector<unsigned>{})
    {
        if (check(data).m > *m2)
            throw 3;
    }
    for (const auto& data : std::vector<unsigned>{})
    {
        if (check(data).m > *m3)
            throw 4;
    }
}
% clang-tidy a.cpp  -- -std=c++17
2 warnings generated.
/home/andrei/dev/a.cpp:58:10: warning: Potential leak of memory pointed to by '._M_head_impl._M_t._M_t._M_head_impl' [clang-analyzer-cplusplus.NewDeleteLeaks]
    I i2{std::move(m1)};
         ^
/home/andrei/dev/a.cpp:57:26: note: Calling 'alloc1'
    auto [m1, ignore1] = alloc1();
                         ^
/home/andrei/dev/a.cpp:11:13: note: Calling 'make_unique<unsigned int, int>'
    return {std::make_unique<unsigned>(1), 0};
            ^
/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:962:30: note: Memory is allocated
    { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                             ^
/home/andrei/dev/a.cpp:11:13: note: Returned allocated memory
    return {std::make_unique<unsigned>(1), 0};
            ^
/home/andrei/dev/a.cpp:57:26: note: Returned allocated memory
    auto [m1, ignore1] = alloc1();
                         ^
/home/andrei/dev/a.cpp:58:10: note: Potential leak of memory pointed to by '._M_head_impl._M_t._M_t._M_head_impl'
    I i2{std::move(m1)};
         ^
/home/andrei/dev/a.cpp:73:29: warning: Potential leak of memory pointed to by '._M_head_impl._M_t._M_t._M_head_impl' [clang-analyzer-cplusplus.NewDeleteLeaks]
    for (const auto& data : std::vector<unsigned>{})
                            ^
/home/andrei/dev/a.cpp:64:26: note: Calling 'alloc2'
    auto [m2, ignore2] = alloc2();
                         ^
/home/andrei/dev/a.cpp:19:9: note: Assuming the condition is true
    if (v1.size() == 1)
        ^
/home/andrei/dev/a.cpp:19:5: note: Taking true branch
    if (v1.size() == 1)
    ^
/home/andrei/dev/a.cpp:21:13: note: Assuming the condition is false
        if (v1[0] > 0)
            ^
/home/andrei/dev/a.cpp:21:9: note: Taking false branch
        if (v1[0] > 0)
        ^
/home/andrei/dev/a.cpp:24:17: note: Calling 'make_unique<unsigned int, int>'
        return {std::make_unique<unsigned>(1), 0};
                ^
/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:962:30: note: Memory is allocated
    { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                             ^
/home/andrei/dev/a.cpp:24:17: note: Returned allocated memory
        return {std::make_unique<unsigned>(1), 0};
                ^
/home/andrei/dev/a.cpp:64:26: note: Returned allocated memory
    auto [m2, ignore2] = alloc2();
                         ^
/home/andrei/dev/a.cpp:70:9: note: Taking false branch
        if (check(data).m > *m2)
        ^
/home/andrei/dev/a.cpp:73:29: note: Potential leak of memory pointed to by '._M_head_impl._M_t._M_t._M_head_impl'
    for (const auto& data : std::vector<unsigned>{})

https://godbolt.org/z/ceG3ce

gumb0 commented 3 years ago

I tried with libc++, too, and can confirm that it's not reproduced with it.

gumb0 commented 3 years ago

Preprocessed example Here's the preprocessed file.

I'm also using libstdc++ locally.

haoNoQ commented 3 years ago

Surprisingly, i can't reproduce this one. Godbolt uses libstdc++ which i don't have, and also godbolt doesn't seem to produce sane preprocessed files when fed the -E option (it filters a lot of stuff that makes sense to filter in normal compiler stdout). Regardless of whether i take my own libc++ or i try to correct errors in godbolt's preprocessed file until it compiles, i don't see the buggy warning. Andrei, do you happen to have a preprocessed file of your example?

gumb0 commented 3 years ago

assigned to @devincoughlin

chfast commented 2 years ago

This may be the simpler variant of it. https://godbolt.org/z/516YdMW4r

#include <memory>

std::unique_ptr<int> select(bool c)
{
    return c ? std::make_unique<int>() : nullptr;
}

void test(bool c) { select(c); }
> clang-tidy-14 --checks='-*,clang-analyzer-cplusplus.NewDeleteLeaks' source.cpp -extra-arg=-std=c++17

source.cpp:8:21: error: Potential leak of memory pointed to by field '_M_head_impl' [clang-analyzer-cplusplus.NewDeleteLeaks,-warnings-as-errors]
void test(bool c) { select(c); }
                    ^
source.cpp:8:21: note: Calling 'select'
void test(bool c) { select(c); }
                    ^~~~~~~~~
source.cpp:5:12: note: Assuming 'c' is true
    return c ? std::make_unique<int>() : nullptr;
           ^
source.cpp:5:12: note: '?' condition is true
source.cpp:5:16: note: Calling 'make_unique<int, >'
    return c ? std::make_unique<int>() : nullptr;
               ^~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/unique_ptr.h:984:30: note: Memory is allocated
    { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
source.cpp:5:16: note: Returned allocated memory
    return c ? std::make_unique<int>() : nullptr;
               ^~~~~~~~~~~~~~~~~~~~~~~
source.cpp:8:21: note: Returned allocated memory
void test(bool c) { select(c); }
                    ^~~~~~~~~
source.cpp:8:21: note: Potential leak of memory pointed to by field '_M_head_impl'
1 warning treated as error