From 7b95ff5126f0365c101b08fa33b50ae387342484 Mon Sep 17 00:00:00 2001 From: leftibot Date: Sat, 11 Apr 2026 16:12:41 -0600 Subject: [PATCH] Fix #655: Async issues with threads outliving the chaiscript engine (#656) * Fix #655: Join async threads before engine destruction to prevent heap-use-after-free Issues #632 and #636 (PRs #651 and #653) both stem from the same root cause: async threads spawned via async() can outlive the Dispatch_Engine, accessing shared state (global objects map, type maps) after it has been destroyed. The fix moves async() registration from the stdlib module into ChaiScript_Basic, where spawned threads are tracked via Dispatch_Engine. The engine's destructor now joins all outstanding async threads before destroying shared data structures. Co-Authored-By: Claude Opus 4.6 (1M context) * Address review: follow rule of 5, explicitly default move operations Requested by @lefticus in PR #656 review. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: leftibot Co-authored-by: Claude Opus 4.6 (1M context) --- CMakeLists.txt | 3 ++ include/chaiscript/chaiscript_stdlib.hpp | 5 +- .../chaiscript/dispatchkit/dispatchkit.hpp | 43 +++++++++++++++++ .../chaiscript/language/chaiscript_engine.hpp | 20 ++++++++ unittests/async_engine_lifetime.chai | 17 +++++++ unittests/async_engine_lifetime_test.cpp | 46 +++++++++++++++++++ 6 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 unittests/async_engine_lifetime.chai create mode 100644 unittests/async_engine_lifetime_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 029f33da..7fcb4bc2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -413,6 +413,9 @@ if(BUILD_TESTING) "CHAI_USE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/unittests/" "CHAI_MODULE_PATH=${CMAKE_CURRENT_BINARY_DIR}/" ) + add_executable(async_engine_lifetime_test unittests/async_engine_lifetime_test.cpp) + target_link_libraries(async_engine_lifetime_test ${LIBS}) + add_test(NAME Async_Engine_Lifetime_Test COMMAND async_engine_lifetime_test) endif() add_executable(multifile_test diff --git a/include/chaiscript/chaiscript_stdlib.hpp b/include/chaiscript/chaiscript_stdlib.hpp index 5cb4fa68..0daac71e 100644 --- a/include/chaiscript/chaiscript_stdlib.hpp +++ b/include/chaiscript/chaiscript_stdlib.hpp @@ -49,9 +49,8 @@ namespace chaiscript { #ifndef CHAISCRIPT_NO_THREADS bootstrap::standard_library::future_type>("future", *lib); - lib->add(chaiscript::fun( - [](const std::function &t_func) { return std::async(std::launch::async, t_func); }), - "async"); + // Note: async() is registered in ChaiScript_Basic::build_eval_system() + // with thread tracking to prevent heap-use-after-free on engine destruction. #endif json_wrap::library(*lib); diff --git a/include/chaiscript/dispatchkit/dispatchkit.hpp b/include/chaiscript/dispatchkit/dispatchkit.hpp index b54d42b4..95627dbc 100644 --- a/include/chaiscript/dispatchkit/dispatchkit.hpp +++ b/include/chaiscript/dispatchkit/dispatchkit.hpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -370,6 +371,28 @@ namespace chaiscript { , m_parser(parser) { } + ~Dispatch_Engine() { + join_async_threads(); + } + + Dispatch_Engine(const Dispatch_Engine &) = delete; + Dispatch_Engine &operator=(const Dispatch_Engine &) = delete; + Dispatch_Engine(Dispatch_Engine &&) = default; + Dispatch_Engine &operator=(Dispatch_Engine &&) = default; + +#ifndef CHAISCRIPT_NO_THREADS + /// Track an async thread so it can be joined during destruction + void track_async_thread(std::thread t_thread) { + chaiscript::detail::threading::unique_lock l(m_async_mutex); + // Clean up already-finished threads to avoid unbounded growth + m_async_threads.erase( + std::remove_if(m_async_threads.begin(), m_async_threads.end(), + [](std::thread &t) { return !t.joinable(); }), + m_async_threads.end()); + m_async_threads.push_back(std::move(t_thread)); + } +#endif + /// \brief casts an object while applying any Dynamic_Conversion available template decltype(auto) boxed_cast(const Boxed_Value &bv) const { @@ -1165,6 +1188,21 @@ namespace chaiscript { get_function_objects_int().insert_or_assign(t_name, std::move(new_func)); } + void join_async_threads() { +#ifndef CHAISCRIPT_NO_THREADS + std::vector threads; + { + chaiscript::detail::threading::unique_lock l(m_async_mutex); + threads = std::move(m_async_threads); + } + for (auto &t : threads) { + if (t.joinable()) { + t.join(); + } + } +#endif + } + mutable chaiscript::detail::threading::shared_mutex m_mutex; Type_Conversions m_conversions; @@ -1174,6 +1212,11 @@ namespace chaiscript { mutable std::atomic_uint_fast32_t m_method_missing_loc = {0}; State m_state; + +#ifndef CHAISCRIPT_NO_THREADS + mutable chaiscript::detail::threading::shared_mutex m_async_mutex; + std::vector m_async_threads; +#endif }; class Dispatch_State { diff --git a/include/chaiscript/language/chaiscript_engine.hpp b/include/chaiscript/language/chaiscript_engine.hpp index c9f22e58..50dfb3c7 100644 --- a/include/chaiscript/language/chaiscript_engine.hpp +++ b/include/chaiscript/language/chaiscript_engine.hpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -179,6 +180,25 @@ namespace chaiscript { }), "namespace"); m_engine.add(fun([this](const std::string &t_namespace_name) { import(t_namespace_name); }), "import"); + +#ifndef CHAISCRIPT_NO_THREADS + // Register async() with thread tracking so the engine can join all + // async threads before destroying shared state (issues #632, #636). + m_engine.add(chaiscript::fun( + [this](const std::function &t_func) { + auto promise_ptr = std::make_shared>(); + auto future = promise_ptr->get_future(); + m_engine.track_async_thread(std::thread([promise_ptr, t_func]() { + try { + promise_ptr->set_value(t_func()); + } catch (...) { + promise_ptr->set_exception(std::current_exception()); + } + })); + return future; + }), + "async"); +#endif } /// Skip BOM at the beginning of file diff --git a/unittests/async_engine_lifetime.chai b/unittests/async_engine_lifetime.chai new file mode 100644 index 00000000..94ee7066 --- /dev/null +++ b/unittests/async_engine_lifetime.chai @@ -0,0 +1,17 @@ +// Regression test for #632 and #636: Heap-use-after-free in async threads +// Async threads must complete before the engine is destroyed. + +var func = fun(){ + var ret = 0; + for (var i = 0; i < 1000; ++i) { + ret += i; + } + return ret; +} + +var fut1 = async(func); +var fut2 = async(func); + +// Wait for results to verify correctness +assert_equal(fut1.get(), 499500); +assert_equal(fut2.get(), 499500); diff --git a/unittests/async_engine_lifetime_test.cpp b/unittests/async_engine_lifetime_test.cpp new file mode 100644 index 00000000..698f64c0 --- /dev/null +++ b/unittests/async_engine_lifetime_test.cpp @@ -0,0 +1,46 @@ +// Regression test for #632 and #636: +// Heap-use-after-free when async threads outlive the ChaiScript engine. +// The engine must join all outstanding async threads before destroying shared state. + +#include + +int main() { + // Test 1: Async threads still running when engine is destroyed. + // Without the fix, this triggers heap-use-after-free under ASAN/TSAN. + for (int iter = 0; iter < 3; ++iter) { + { + chaiscript::ChaiScript chai; + try { + chai.eval(R"( + var func = fun(){ + var ret = 0; + for (var i = 0; i < 5000; ++i) { + ret += i; + } + return ret; + } + + var fut1 = async(func); + var fut2 = async(func); + )"); + // Engine destroyed here without waiting for futures. + } catch (const std::exception &) { + // Exceptions are fine + } + } + } + + // Test 2: Verify async still works correctly (results are accessible). + { + chaiscript::ChaiScript chai; + auto result = chai.eval(R"( + var f = async(fun() { return 42; }); + f.get(); + )"); + if (result != 42) { + return EXIT_FAILURE; + } + } + + return EXIT_SUCCESS; +}