lewissbaker / generator

Reference implementation of std::generator proposal
Boost Software License 1.0
25 stars 7 forks source link

nested_test.cpp memory problems #15

Open ciaranm opened 3 months ago

ciaranm commented 3 months ago

I'm not clever enough to figure out whether this is a compiler problem, a library problem, or a tests problem, but nested_test isn't happy on any of GitHub's ubuntu-22.04 runner, g++ 13.2.0 on Ubuntu 24.04, or clang 15.0.0 on OS X 14.5. I get either segfaults, bad_alloc being thrown, or valgrind errors, depending upon the system.

On Ubuntu 24.04, the test runs, but:

$ valgrind --tool=memcheck ./build/_deps/generator-build/tests/nested_test 
==116182== Memcheck, a memory error detector
==116182== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==116182== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==116182== Command: ./build/_deps/generator-build/tests/nested_test
==116182== 
-> test_yielding_elements_of_empty_generator
-> test_yielding_elements_of_nested_one_level
-> test_yielding_elements_of_recursive
-> test_yielding_elements_of_generator_with_different_value_type
-> test_yielding_elements_of_generator_with_different_reference_type
-> test_yielding_elements_of_generator_with_different_allocator_type
-> test_elementsof_with_allocator_args
-> test_yielding_elements_of_vector
-> test_nested_generator_scopes_exit_innermost_scope_first
==116182== Use of uninitialised value of size 8
==116182==    at 0x10DA64: emplace_back<int> (vector.tcc:114)
==116182==    by 0x10DA64: push_back (stl_vector.h:1299)
==116182==    by 0x10DA64: operator() (nested_test.cpp:257)
==116182==    by 0x10DA64: ~scope_guard (nested_test.cpp:245)
==116182==    by 0x10DA64: operator() (nested_test.cpp:260)
==116182==    by 0x10DA64: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()() const::_ZZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEvENKUlvE0_clEv.Frame*) [clone .destroy] (nested_test.cpp:255)
==116182==    by 0x10CFB9: destroy (coroutine:137)
==116182==    by 0x10CFB9: ~generator (__generator.hpp:691)
==116182==    by 0x10CFB9: ~__yield_sequence_awaiter (__generator.hpp:419)
==116182==    by 0x10CFB9: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::_ZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEv.Frame*) [clone .actor] (nested_test.cpp:265)
==116182==    by 0x10F3E7: destroy (coroutine:137)
==116182==    by 0x10F3E7: ~generator (__generator.hpp:691)
==116182==    by 0x10F3E7: test_nested_generator_scopes_exit_innermost_scope_first() (nested_test.cpp:272)
==116182==    by 0x109F9C: main (nested_test.cpp:325)
==116182== 
==116182== Use of uninitialised value of size 8
==116182==    at 0x10DA80: _M_realloc_insert<int> (vector.tcc:445)
==116182==    by 0x10DA80: emplace_back<int> (vector.tcc:123)
==116182==    by 0x10DA80: push_back (stl_vector.h:1299)
==116182==    by 0x10DA80: operator() (nested_test.cpp:257)
==116182==    by 0x10DA80: ~scope_guard (nested_test.cpp:245)
==116182==    by 0x10DA80: operator() (nested_test.cpp:260)
==116182==    by 0x10DA80: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()() const::_ZZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEvENKUlvE0_clEv.Frame*) [clone .destroy] (nested_test.cpp:255)
==116182==    by 0x10CFB9: destroy (coroutine:137)
==116182==    by 0x10CFB9: ~generator (__generator.hpp:691)
==116182==    by 0x10CFB9: ~__yield_sequence_awaiter (__generator.hpp:419)
==116182==    by 0x10CFB9: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::_ZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEv.Frame*) [clone .actor] (nested_test.cpp:265)
==116182==    by 0x10F3E7: destroy (coroutine:137)
==116182==    by 0x10F3E7: ~generator (__generator.hpp:691)
==116182==    by 0x10F3E7: test_nested_generator_scopes_exit_innermost_scope_first() (nested_test.cpp:272)
==116182==    by 0x109F9C: main (nested_test.cpp:325)
==116182== 
==116182== Use of uninitialised value of size 8
==116182==    at 0x10DB10: _M_realloc_insert<int> (vector.tcc:520)
==116182==    by 0x10DB10: emplace_back<int> (vector.tcc:123)
==116182==    by 0x10DB10: push_back (stl_vector.h:1299)
==116182==    by 0x10DB10: operator() (nested_test.cpp:257)
==116182==    by 0x10DB10: ~scope_guard (nested_test.cpp:245)
==116182==    by 0x10DB10: operator() (nested_test.cpp:260)
==116182==    by 0x10DB10: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()() const::_ZZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEvENKUlvE0_clEv.Frame*) [clone .destroy] (nested_test.cpp:255)
==116182==    by 0x10CFB9: destroy (coroutine:137)
==116182==    by 0x10CFB9: ~generator (__generator.hpp:691)
==116182==    by 0x10CFB9: ~__yield_sequence_awaiter (__generator.hpp:419)
==116182==    by 0x10CFB9: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::_ZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEv.Frame*) [clone .actor] (nested_test.cpp:265)
==116182==    by 0x10F3E7: destroy (coroutine:137)
==116182==    by 0x10F3E7: ~generator (__generator.hpp:691)
==116182==    by 0x10F3E7: test_nested_generator_scopes_exit_innermost_scope_first() (nested_test.cpp:272)
==116182==    by 0x109F9C: main (nested_test.cpp:325)
==116182== 
==116182== Use of uninitialised value of size 8
==116182==    at 0x10DB2E: deallocate (new_allocator.h:172)
==116182==    by 0x10DB2E: deallocate (allocator.h:210)
==116182==    by 0x10DB2E: deallocate (alloc_traits.h:517)
==116182==    by 0x10DB2E: _M_deallocate (stl_vector.h:390)
==116182==    by 0x10DB2E: _M_realloc_insert<int> (vector.tcc:519)
==116182==    by 0x10DB2E: emplace_back<int> (vector.tcc:123)
==116182==    by 0x10DB2E: push_back (stl_vector.h:1299)
==116182==    by 0x10DB2E: operator() (nested_test.cpp:257)
==116182==    by 0x10DB2E: ~scope_guard (nested_test.cpp:245)
==116182==    by 0x10DB2E: operator() (nested_test.cpp:260)
==116182==    by 0x10DB2E: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::{lambda()#2}::operator()() const::_ZZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEvENKUlvE0_clEv.Frame*) [clone .destroy] (nested_test.cpp:255)
==116182==    by 0x10CFB9: destroy (coroutine:137)
==116182==    by 0x10CFB9: ~generator (__generator.hpp:691)
==116182==    by 0x10CFB9: ~__yield_sequence_awaiter (__generator.hpp:419)
==116182==    by 0x10CFB9: test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()(test_nested_generator_scopes_exit_innermost_scope_first()::{lambda()#1}::operator()() const::_ZZ55test_nested_generator_scopes_exit_innermost_scope_firstvENKUlvE_clEv.Frame*) [clone .actor] (nested_test.cpp:265)
==116182==    by 0x10F3E7: destroy (coroutine:137)
==116182==    by 0x10F3E7: ~generator (__generator.hpp:691)
==116182==    by 0x10F3E7: test_nested_generator_scopes_exit_innermost_scope_first() (nested_test.cpp:272)
==116182==    by 0x109F9C: main (nested_test.cpp:325)
==116182== 
-> test_exception_propagating_from_nested_generator
==116182== 
==116182== HEAP SUMMARY:
==116182==     in use at exit: 0 bytes in 0 blocks
==116182==   total heap usage: 34 allocs, 34 frees, 78,120 bytes allocated
==116182== 
==116182== All heap blocks were freed -- no leaks are possible
==116182== 
==116182== Use --track-origins=yes to see where uninitialised values come from
==116182== For lists of detected and suppressed errors, rerun with: -s
==116182== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

On OS X with clang, this crashes with a bad_alloc:

-> test_nested_generator_scopes_exit_innermost_scope_first
libc++abi: terminating due to uncaught exception of type std::bad_alloc: std::bad_alloc
Process 91005 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x000000019bfeea60 libsystem_kernel.dylib`__pthread_kill + 8
libsystem_kernel.dylib`:
->  0x19bfeea60 <+8>:  b.lo   0x19bfeea80               ; <+40>
    0x19bfeea64 <+12>: pacibsp 
    0x19bfeea68 <+16>: stp    x29, x30, [sp, #-0x10]!
    0x19bfeea6c <+20>: mov    x29, sp
Target 0: (nested_test) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x000000019bfeea60 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000019c026c20 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x000000019bf33a30 libsystem_c.dylib`abort + 180
    frame #3: 0x000000019bfddd08 libc++abi.dylib`abort_message + 132
    frame #4: 0x000000019bfcdfa4 libc++abi.dylib`demangling_terminate_handler() + 320
    frame #5: 0x000000019bc6c1e0 libobjc.A.dylib`_objc_terminate() + 160
    frame #6: 0x000000019bfdd0cc libc++abi.dylib`std::__terminate(void (*)()) + 16
    frame #7: 0x000000019bfe05f8 libc++abi.dylib`__cxa_rethrow + 204
    frame #8: 0x0000000100005c68 nested_test`std::__generator_promise_base<int>::unhandled_exception(this=<unavailable>) at __generator.hpp:372:13 [opt]
    frame #9: 0x000000010000844c nested_test`test_nested_generator_scopes_exit_innermost_scope_first()::$_8::operator()(this=<unavailable>) const at nested_test.cpp:251:20 [opt]
    frame #10: 0x00000001000056c4 nested_test`test_nested_generator_scopes_exit_innermost_scope_first() [inlined] std::__1::coroutine_handle<void>::resume[abi:ue170006](this=<unavailable>) const at coroutine_handle.h:78:9 [opt]
    frame #11: 0x00000001000056bc nested_test`test_nested_generator_scopes_exit_innermost_scope_first() [inlined] std::generator<int, int, std::use_allocator_arg>::begin(this=<unavailable>) at __generator.hpp:766:17 [opt]
    frame #12: 0x00000001000056bc nested_test`test_nested_generator_scopes_exit_innermost_scope_first() at nested_test.cpp:269:23 [opt]
    frame #13: 0x0000000100005b9c nested_test`main at nested_test.cpp:325:5 [opt]
    frame #14: 0x000000019bc9e0e0 dyld`start + 2360

This is reproducible under GitHub actions, see e.g. https://github.com/ciaranm/glasgow-constraint-solver/actions/runs/9374592559/job/25810964264?pr=54

lewissbaker commented 3 months ago

It looks like this is like a bug in the test:

    std::vector<int> events;
    auto makeGen = [&]() -> std::generator<int> {
        events.push_back(1);
        scope_guard f{[&] { events.push_back(2); }};

        auto nested = [&]() -> std::generator<int> {
            events.push_back(3);
            scope_guard g{[&] { events.push_back(4); }};

            co_yield 42;
        }();

        scope_guard h{[&] { events.push_back(5); }};

        co_yield std::ranges::elements_of(std::move(nested));
    };

    {
        auto gen = makeGen();
        auto it = gen.begin();
        CHECK(*it == 42);
        CHECK((events == std::vector{1, 3}));
    }

    CHECK((events == std::vector{1, 3, 4, 5, 2}));

The use of an immediately-invoked-lambda-with-captures which returns a generator which is not immediately consumed is going to leave the coroutine with a dangling reference to the lambda object which could be causing the sanitizer failure/crash.

To fix this bug the test would need to be changed to:

    std::vector<int> events;
    auto makeGen = [](std::vector<int>& events) -> std::generator<int> {
        events.push_back(1);
        scope_guard f{[&] { events.push_back(2); }};

        auto nested = [](std::vector<int>& events) -> std::generator<int> {
            events.push_back(3);
            scope_guard g{[&] { events.push_back(4); }};

            co_yield 42;
        }(events);

        scope_guard h{[&] { events.push_back(5); }};

        co_yield std::ranges::elements_of(std::move(nested));
    };

    {
        auto gen = makeGen(events);
        auto it = gen.begin();
        CHECK(*it == 42);
        CHECK((events == std::vector{1, 3}));
    }

    CHECK((events == std::vector{1, 3, 4, 5, 2}));

I have not yet tested this, however, so it's possible there may still be other issues. Please try this out and see if it fixes the problem.