Clang analyzer potential memory leak - false positive

Question

I have an implementation of static function in my codebase, and while running clang-tidy on it, I noticed that the static analyzer points towards a possible memory leak when I am pretty sure the code is correct. (I have verified it with sanitizers). I think this is most likely due to static analyzers missing some branch statement, but I am not 100% sure.

Here is a slim down version of the code:

#include <array>
#include <functional>
#include <utility>

struct SmallFunction {
  struct Base {
    virtual ~Base() = default;
    virtual void destroy() = 0;
  };

  template <typename T>
  struct Inner : Base {
    Inner(T&& f) : f_(std::move(f)) {}
    void destroy() override { f_.~T(); }
    T f_;
  };

  template <typename T>
  SmallFunction(T&& f) : empty(false) {
    static_assert(sizeof(T) <= 32);
    new (storage) Inner<T>(std::forward<T>(f));
  }

  ~SmallFunction() {
    if (!empty) {
      reinterpret_cast<Base*>(storage)->destroy();
    }
  }

  bool empty = true;
  alignas(8) char storage[40];  // 32 + 8
};

int main() {
  std::array<char, 64> large;
  auto lambda = [large] {};
  std::function<void()> f = lambda;
  SmallFunction sf = std::move(f);
}

Here is the clang-tidy analysis:

/home/ce/example.cpp:39:1: warning: Potential memory leak [clang-analyzer-cplusplus.NewDeleteLeaks]
}
^
/home/ce/example.cpp:37:29: note: Calling constructor for 'function<void ()>'
  std::function<void()> f = lambda;
                            ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:675:2: note: Taking true branch
        if (_My_handler::_M_not_empty_function(__f))
        ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:677:6: note: Calling '_Base_manager::_M_init_functor'
            _My_handler::_M_init_functor(_M_functor, std::move(__f));
            ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:223:4: note: Calling '_Base_manager::_M_init_functor'
        { _M_init_functor(__functor, std::move(__f), _Local_storage()); }
          ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:252:39: note: Memory is allocated
        { __functor._M_access<_Functor*>() = new _Functor(std::move(__f)); }
                                             ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:223:4: note: Returned allocated memory
        { _M_init_functor(__functor, std::move(__f), _Local_storage()); }
          ^
/opt/compiler-explorer/gcc-9.2.0/lib/gcc/x86_64-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/std_function.h:677:6: note: Returned allocated memory
            _My_handler::_M_init_functor(_M_functor, std::move(__f));
            ^
/home/ce/example.cpp:37:29: note: Returning from constructor for 'function<void ()>'
  std::function<void()> f = lambda;
                            ^
/home/ce/example.cpp:39:1: note: Potential memory leak
}
^
1 warning generated.

Here is godbolt link with clang-tidy enabled.

Answer 1

The reporting from clang-tidy is definitely a bit weird and could use some clarification.

It's upset about the placement-new of Inner<T> without seeing a matching explicit destructor call. You have this strange destroy() method that isn't even necessary since the Base destructor is virtual and the implicit Inner destructor will clean up Inner::f_.

This is trivially fixed in the following ways:

1. Replace bool SmallFunction::empty with Base *SmallFunction::value and store the result of the placement-new in it. (This is not strictly necessary, but I find the code flows better without requiring the reinterpret_cast, and it's easier to get right since the compiler can type-check.)
2. In SmallFunction::~SmallFunction, replace the destroy call with value->~Base().
3. Remove the destroy() method; it's not needed.

This satisfies clang-tidy (see here).

I don't think there was a memory leak, but there was an object (the Inner<T>) that was constructed and never destructed. There is no consequence that I can see, but it doesn't hurt to do things the right way -- and it makes the job of static analyzers easier, anyway.