Problem when exception thrown during tbb::task_arena::execute call

[Dr15Jones]

After switching to explicit use of tbb::task_arena one of our tests was having a segmentation fault when an exception was thrown. The problem seemed to be coming from an object, whose lifetime was managed by std::shared_ptr<>, being deleted twice. A shared_ptr<> to the object was the return value from the call to tbb::task_arena::execute when the passed in lambda threw an exception. Using valgrind did pinpoint problems with access to the shared_ptr internals after the they had been destroyed. Changing from using the return value from tbb::task_arena::execute to instead passing in the value by reference to the lambda capture made the problem go away.

What follows is a simplified program which also exhibits problems under valgrind.

#include <memory>
#include <iostream>
#include <exception>
#include "tbb/task_arena.h"

struct Foo {
  Foo() { ++count;}
  ~Foo() { --count;}

  static int count;
};
int Foo::count = 0;

struct Bar {
  std::shared_ptr<Foo> releaseFoo() {
    std::shared_ptr<Foo> temp;
    temp = foo;
    foo.reset();
    throw std::logic_error("force error");
    return temp;
  }
  std::shared_ptr<Foo> foo;
};

#define RETURNVALUE
int main() {

  Bar bar;
  bar.foo = std::make_shared<Foo>();
  try {
    tbb::task_arena arena;
#if defined(RETURNVALUE)
    //leads to valgrind problems
    auto v = arena.execute([&bar]() {
        return bar.releaseFoo();
      });
#else
    //no problems.
    std::shared_ptr<Foo> v;
    arena.execute([&bar, &v]() {
        v = bar.releaseFoo();
      });
#endif
  } catch(...) {}

  std::cout <<"count "<<Foo::count<<std::endl;

  return 0;
}

The problem reported by valgrind is

==27687== Invalid read of size 4
==27687==    at 0x401548: __gnu_cxx::__exchange_and_add(int volatile*, int) (atomicity.h:49)
==27687==    by 0x4014E8: __gnu_cxx::__exchange_and_add_dispatch(int*, int) (atomicity.h:82)
==27687==    by 0x401A87: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:152)
==27687==    by 0x401A59: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:728)
==27687==    by 0x40282D: std::__shared_ptr<Foo, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1167)
==27687==    by 0x401967: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:103)
==27687==    by 0x40170B: tbb::interface7::internal::delegated_function<main::$_0 const, std::shared_ptr<Foo> >::~delegated_function() (task_arena.h:88)
==27687==    by 0x401619: std::shared_ptr<Foo> tbb::interface7::task_arena::execute_impl<std::shared_ptr<Foo>, main::$_0 const>(main::$_0 const&) (task_arena.h:273)
==27687==    by 0x4014AA: tbb::interface7::internal::return_type_or_void<main::$_0>::type tbb::interface7::task_arena::execute<main::$_0>(main::$_0 const&) (task_arena.h:433)
==27687==    by 0x40139D: main (test_exception.cc:35)
==27687==  Address 0x575bc88 is 8 bytes inside a block of size 24 free'd
==27687==    at 0x402EDD0: operator delete(void*) (in /cvmfs/cms-ib.cern.ch/nweek-02644/slc7_amd64_gcc820/external/valgrind/3.15.0-bcolbf2/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27687==    by 0x4026AF: __gnu_cxx::new_allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> >::deallocate(std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2>*, unsigned long) (new_allocator.h:125)
==27687==    by 0x402687: std::allocator_traits<std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> > >::deallocate(std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> >&, std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2>*, unsigned long) (alloc_traits.h:462)
==27687==    by 0x401FF3: std::__allocated_ptr<std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> > >::~__allocated_ptr() (allocated_ptr.h:73)
==27687==    by 0x402324: std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2>::_M_destroy() (shared_ptr_base.h:564)
==27687==    by 0x401AE1: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:171)
==27687==    by 0x401A59: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:728)
==27687==    by 0x40282D: std::__shared_ptr<Foo, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1167)
==27687==    by 0x401967: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:103)
==27687==    by 0x402A65: Bar::releaseFoo() (test_exception.cc:23)
==27687==    by 0x4017D5: main::$_0::operator()() const (test_exception.cc:36)
==27687==    by 0x401775: tbb::interface7::internal::delegated_function<main::$_0 const, std::shared_ptr<Foo> >::operator()() const (task_arena.h:79)
==27687==  Block was alloc'd at
==27687==    at 0x402DDB2: operator new(unsigned long) (in /cvmfs/cms-ib.cern.ch/nweek-02644/slc7_amd64_gcc820/external/valgrind/3.15.0-bcolbf2/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==27687==    by 0x4020F3: __gnu_cxx::new_allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> >::allocate(unsigned long, void const*) (new_allocator.h:111)
==27687==    by 0x402063: std::allocator_traits<std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> > >::allocate(std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> >&, unsigned long) (alloc_traits.h:436)
==27687==    by 0x401E49: std::__allocated_ptr<std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> > > std::__allocate_guarded<std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> > >(std::allocator<std::_Sp_counted_ptr_inplace<Foo, std::allocator<Foo>, (__gnu_cxx::_Lock_policy)2> >&) (allocated_ptr.h:97)
==27687==    by 0x401CDF: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<Foo, std::allocator<Foo>>(Foo*&, std::_Sp_alloc_shared_tag<std::allocator<Foo> >) (shared_ptr_base.h:675)
==27687==    by 0x401C7F: std::__shared_ptr<Foo, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<Foo>>(std::_Sp_alloc_shared_tag<std::allocator<Foo> >) (shared_ptr_base.h:1342)
==27687==    by 0x401C37: std::shared_ptr<Foo>::shared_ptr<std::allocator<Foo>>(std::_Sp_alloc_shared_tag<std::allocator<Foo> >) (shared_ptr.h:359)
==27687==    by 0x401BBA: std::shared_ptr<Foo> std::allocate_shared<Foo, std::allocator<Foo>>(std::allocator<Foo> const&) (shared_ptr.h:705)
==27687==    by 0x4018C3: std::shared_ptr<Foo> std::make_shared<Foo>() (shared_ptr.h:721)
==27687==    by 0x401343: main (test_exception.cc:31)

The problem happens using gcc 8.2, 9.0, 10.0 and clang 10.1.

[alexey-katranov]

Thank you for the report. The logic is broken when exception is thrown. The root cause is that we call dtor for the object (shared_ptr) that was not constructed.

template<typename F, typename R>
class task_arena_function : public delegate_base {
    F &my_func;
    aligned_space<R> my_return_storage;
    mutable bool my_constructed{};                          // FIX
    // The function should be called only once.
    bool operator()() const override {
        new (my_return_storage.begin()) R(my_func());       // CALL USER FUNCTOR WITH EXCEPTION
        my_constructed = true;                              // FIX
        return true;
    }
public:
    task_arena_function(F& f) : my_func(f) {}
    // The function can be called only after operator() and only once.
    R consume_result() const {
        __TBB_ASSERT(my_constructed, NULL);
        return std::move(*(my_return_storage.begin()));
    }
    ~task_arena_function() override {
        if (my_constructed) {                                // FIX
            my_return_storage.begin()->~R();                 // HERE IS THE PROBLEM
        }
    }
};

[alexey-katranov]

The issue was fixed in oneTBB 2021.1: https://software.intel.com/content/www/us/en/develop/articles/intel-oneapi-threading-building-blocks-release-notes.html#inpage-nav-1-4