Fix __stop_state double delete. (fix #42)

[Grumpfy]

Repro steps:

• create a stop_source
• get two stop tokens
• destroy the source
• destroy one of the stop_token (the internal state is deleted)
• destroy the second stop_token (double delete of the internal state)

[Grumpfy]

Issue: https://github.com/josuttis/jthread/issues/42

[Grumpfy]

a concrete example:

TEST(future, stop_token) {
	std::stop_source source;
	auto token1 = source.get_token();
	auto token2 = source.get_token();
	source = std::stop_source{};
}

And the issue highlighted by clang memory sanitizer:

=================================================================
==66056==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300000ca30 at pc 0x00010cfb3888 bp 0x7ffee2c87e80 sp 0x7ffee2c87e78
WRITE of size 8 at 0x60300000ca30 thread T0
    #0 0x10cfb3887 in unsigned long long std::__1::__cxx_atomic_fetch_sub<unsigned long long>(std::__1::__cxx_atomic_base_impl<unsigned long long>*, unsigned long long, std::__1::memory_order) atomic:1069
    #1 0x10cfb3725 in std::__1::__atomic_base<unsigned long long, true>::fetch_sub(unsigned long long, std::__1::memory_order) atomic:1717
    #2 0x10cfb39c8 in std::__stop_state::__remove_token_reference() stop_token.hpp:60
    #3 0x10cfb398e in std::stop_token::~stop_token() stop_token.hpp:352
    #4 0x10cfb29a4 in std::stop_token::~stop_token() stop_token.hpp:350
    #5 0x10cfb25d1 in (anonymous namespace)::future_stop_token_Test::TestBody() future.cpp:54
    #6 0x10d0761a7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x47 (test_concurrency:x86_64+0x1000ff1a7)
    #7 0x10d0760fe in testing::Test::Run()+0x39e (test_concurrency:x86_64+0x1000ff0fe)
    #8 0x10d07797f in testing::TestInfo::Run()+0x3bf (test_concurrency:x86_64+0x10010097f)
    #9 0x10d078406 in testing::TestSuite::Run()+0x106 (test_concurrency:x86_64+0x100101406)
    #10 0x10d085da6 in testing::internal::UnitTestImpl::RunAllTests()+0x766 (test_concurrency:x86_64+0x10010eda6)
    #11 0x10d0854a7 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x47 (test_concurrency:x86_64+0x10010e4a7)
    #12 0x10d08542b in testing::UnitTest::Run()+0x6b (test_concurrency:x86_64+0x10010e42b)
    #13 0x10d064590 in RUN_ALL_TESTS() gtest.h:2478
    #14 0x10d064508 in main main.cpp:10
    #15 0x7fff2077ff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

0x60300000ca30 is located 0 bytes inside of 24-byte region [0x60300000ca30,0x60300000ca48)
freed by thread T0 here:
    #0 0x10dc210bd in wrap__ZdlPv+0x7d (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x550bd)
    #1 0x10cfb39fa in std::__stop_state::__remove_token_reference() stop_token.hpp:62
    #2 0x10cfb398e in std::stop_token::~stop_token() stop_token.hpp:352
    #3 0x10cfb29a4 in std::stop_token::~stop_token() stop_token.hpp:350
    #4 0x10cfb25c0 in (anonymous namespace)::future_stop_token_Test::TestBody() future.cpp:54
    #5 0x10d0761a7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x47 (test_concurrency:x86_64+0x1000ff1a7)
    #6 0x10d0760fe in testing::Test::Run()+0x39e (test_concurrency:x86_64+0x1000ff0fe)
    #7 0x10d07797f in testing::TestInfo::Run()+0x3bf (test_concurrency:x86_64+0x10010097f)
    #8 0x10d078406 in testing::TestSuite::Run()+0x106 (test_concurrency:x86_64+0x100101406)
    #9 0x10d085da6 in testing::internal::UnitTestImpl::RunAllTests()+0x766 (test_concurrency:x86_64+0x10010eda6)
    #10 0x10d0854a7 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x47 (test_concurrency:x86_64+0x10010e4a7)
    #11 0x10d08542b in testing::UnitTest::Run()+0x6b (test_concurrency:x86_64+0x10010e42b)
    #12 0x10d064590 in RUN_ALL_TESTS() gtest.h:2478
    #13 0x10d064508 in main main.cpp:10
    #14 0x7fff2077ff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

previously allocated by thread T0 here:
    #0 0x10dc20c9d in wrap__Znwm+0x7d (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x54c9d)
    #1 0x10cfb29cd in std::stop_source::stop_source() stop_token.hpp:415
    #2 0x10cfb2764 in std::stop_source::stop_source() stop_token.hpp:415
    #3 0x10cfb2555 in (anonymous namespace)::future_stop_token_Test::TestBody() future.cpp:50
    #4 0x10d0761a7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x47 (test_concurrency:x86_64+0x1000ff1a7)
    #5 0x10d0760fe in testing::Test::Run()+0x39e (test_concurrency:x86_64+0x1000ff0fe)
    #6 0x10d07797f in testing::TestInfo::Run()+0x3bf (test_concurrency:x86_64+0x10010097f)
    #7 0x10d078406 in testing::TestSuite::Run()+0x106 (test_concurrency:x86_64+0x100101406)
    #8 0x10d085da6 in testing::internal::UnitTestImpl::RunAllTests()+0x766 (test_concurrency:x86_64+0x10010eda6)
    #9 0x10d0854a7 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x47 (test_concurrency:x86_64+0x10010e4a7)
    #10 0x10d08542b in testing::UnitTest::Run()+0x6b (test_concurrency:x86_64+0x10010e42b)
    #11 0x10d064590 in RUN_ALL_TESTS() gtest.h:2478
    #12 0x10d064508 in main main.cpp:10
    #13 0x7fff2077ff3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

SUMMARY: AddressSanitizer: heap-use-after-free atomic:1069 in unsigned long long std::__1::__cxx_atomic_fetch_sub<unsigned long long>(std::__1::__cxx_atomic_base_impl<unsigned long long>*, unsigned long long, std::__1::memory_order)

[Grumpfy]

A discussion about the proposed fix is available here: https://github.com/josuttis/jthread/pull/39

[kalman5]

As reported in #39 I would add a new __emptyState and use that instead.

[josuttis]

Thanks for your work. I am currently very busy. But I come back..

Am 22. Juni 2021 22:27:41 MESZ schrieb Gaetano @.***>:

As reported in #39 I would add a new __emptyState and use that instead.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/josuttis/jthread/pull/43#issuecomment-866310286 -- Nico Josuttis (sent from my mobile phone)

[Grumpfy]

I agree with you Gaetano, the code could and should be more expressive. I'll do the suggested update.