False positive: returnDanglingLifetime

[Maciej Dudyński]

Recently updated Cppcheck to newest version (2.1) and this error appeared in many locations in our code. I prepared the simplest code that shows the error:

#include <functional>
#include <iostream>

template <typename T>
class Sequence {
public:
    typedef T value_type;

    struct Iterator : public std::iterator<std::bidirectional_iterator_tag, T, ptrdiff_t, T&, T> {
    public:
        Iterator(int i, std::function<T(int)>& at)
            : m_I(i)
            , m_At(at)
        {
        }
        inline operator int&()
        {
            return m_I;
        }
        inline T operator*() const
        {
            return m_At(m_I);
        }
        inline Iterator& operator++()
        {
            ++m_I;
            return *this;
        }
        inline Iterator& operator--()
        {
            --m_I;
            return *this;
        }
        bool operator==(const Iterator& rhs)
        {
            return m_I == rhs.m_I;
        }
        bool operator!=(const Iterator& rhs)
        {
            return m_I != rhs.m_I;
        }

    protected:
        int m_I;
        std::function<T(int)>& m_At;
    };

    Sequence(size_t size, const std::function<T(int)>& at)
        : m_Size([=] { return int(size); })
        , m_At(at)
    {
    }

    int size() const
    {
        return m_Size();
    }
    bool empty() const
    {
        return m_Size() == 0;
    }

    T operator[](int i)
    {
        return m_At(i);
    }
    const T operator[](int i) const
    {
        return m_At(i);
    }

    Iterator begin()
    {
        return Iterator(0, m_At);
    }
    Iterator end()
    {
        return Iterator(m_Size(), m_At);
    }

protected:
    std::function<int()> m_Size;
    std::function<T(int)> m_At;
};

int* get(std::vector<int>& container)
{
    auto at = [&container](int i) { return std::ref(container[i]); };
    Sequence<int&> seq(container.size(), at);

    for (auto& r : seq) {
        return &r; // return dangling lifetime ?
    }
    return &*seq.begin();
}

int main()
{
    std::vector<int> someContainer { 1, 2, 3 };

    auto ptr = get(someContainer);

    *ptr = 3;

    return 0;
}

I know that the for loop is useless here but it is here only to show when the error occurs. The get function returns valid pointer to first element of someContainer.

[CHR]

I can reproduce this with v2.3.

[CHR]

This snippet still shows the problem:

int* get(std::vector<int>& container)
{
    Sequence seq(container);

    for (auto& r : seq) {
        return &r; // return dangling lifetime ?
    }
    return &*seq.begin();
}

I don't think cppcheck could ever analyse all the code from the OP and determine that only addresses inside the container are returned. On the other hand, if cppcheck didn't warn in this case just to be safe, then not many instances of returnDanglingLifetime would remain.

[Daniel Marjamäki]

hmm.. cppcheck shouldn't warn unless it can determine that there is a bug.

[Daniel Marjamäki]

The error message is not 100% correct:

Returning object that points to local variable 'seq' that will be invalid when returning.

The r does not point at seq but it points at a member in seq.

[Daniel Marjamäki]

I have created ticket https://trac.cppcheck.net/ticket/10163

On the other hand, if cppcheck didn't warn in this case just to be safe, then not many instances of returnDanglingLifetime would remain.

That is true however our philosophy is to avoid false positives when we are unsure. I am hoping there will still be many instances that we can warn about.

[CHR]

I don't think the text of the warning is too far off. I guess there just is no differentiation between simple variables and containers.

The heuristic for avoiding the warning entirely would be something like this:
If there is a container-like object O locally created from another container C, and a function seemingly returns addresses to elements in O, assume that there is operator overloading and other shenanigans going on, so that the returned address safely points at an element in C.
I think there's a trade-of between "losing useful warnings" and "accomodating crazy code"...