From 5a5fcd26c5ed574df408323a5a99f3942e02da8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=88=98=E7=9A=93?= Date: Wed, 16 Apr 2025 21:39:04 -0400 Subject: [PATCH] Delete copy constructor for `stack_frame_guard` and `stack_frame` The copy constructors are causing problems when the `fiber.stack` vector gets reallocated when its capacity is full, since when vectors are reallocated, the elements are moved (or copied if there's no usable move constructor) to the reallocated memory and then the original elements are destroyed. This premature calling of destructors leads to double-free and use-after-free errors. I fixed it by deleting the copy constructors and explicitly defining move constructors. --- binding-sandbox/binding-base.cpp | 15 ++++++- binding-sandbox/binding-base.h | 75 ++++++++++++++++++------------- binding-sandbox/binding-sandbox.h | 15 +++++-- binding-sandbox/sandbox.h | 2 +- libretro/sandbox-bindgen.rb | 1 - src/meson.build | 1 - subprojects/boost_type_index.wrap | 4 -- 7 files changed, 69 insertions(+), 44 deletions(-) delete mode 100644 subprojects/boost_type_index.wrap diff --git a/binding-sandbox/binding-base.cpp b/binding-sandbox/binding-base.cpp index 60ca594a..eb6b8da8 100644 --- a/binding-sandbox/binding-base.cpp +++ b/binding-sandbox/binding-base.cpp @@ -37,10 +37,21 @@ using namespace mkxp_sandbox; -binding_base::stack_frame::stack_frame(struct binding_base &bind, void (*destructor)(void *ptr), boost::typeindex::type_index type, wasm_ptr_t ptr) : bind(bind), destructor(destructor), type(type), ptr(ptr) {} +binding_base::stack_frame::stack_frame(struct binding_base &bind, void (*destructor)(void *ptr), wasm_ptr_t ptr) : bind(&bind), destructor(destructor), ptr(ptr) {} + +binding_base::stack_frame::stack_frame(struct binding_base::stack_frame &&frame) noexcept : bind(std::exchange(frame.bind, nullptr)), destructor(std::exchange(frame.destructor, nullptr)), ptr(std::exchange(frame.ptr, 0)) {} + +struct binding_base::stack_frame &binding_base::stack_frame::operator=(struct binding_base::stack_frame &&frame) noexcept { + bind = std::exchange(frame.bind, nullptr); + destructor = std::exchange(frame.destructor, nullptr); + ptr = std::exchange(frame.ptr, 0); + return *this; +} binding_base::stack_frame::~stack_frame() { - destructor(*bind + ptr); + if (destructor != nullptr) { + destructor(**bind + ptr); + } } binding_base::binding_base(std::shared_ptr m) : next_func_ptr(-1), _instance(m) {} diff --git a/binding-sandbox/binding-base.h b/binding-sandbox/binding-base.h index b0b53d38..25cf4e37 100644 --- a/binding-sandbox/binding-base.h +++ b/binding-sandbox/binding-base.h @@ -27,9 +27,9 @@ #include #include #include +#include #include #include -#include #include #include #include "types.h" @@ -59,16 +59,18 @@ namespace mkxp_sandbox { struct binding_base { - private: - + private: typedef std::tuple key_t; struct stack_frame { - struct binding_base &bind; + struct binding_base *bind; void (*destructor)(void *ptr); - boost::typeindex::type_index type; wasm_ptr_t ptr; - stack_frame(struct binding_base &bind, void (*destructor)(void *ptr), boost::typeindex::type_index type, wasm_ptr_t ptr); + stack_frame(struct binding_base &bind, void (*destructor)(void *ptr), wasm_ptr_t ptr); + stack_frame(const struct stack_frame &frame) = delete; + stack_frame(struct stack_frame &&frame) noexcept; + struct stack_frame &operator=(const struct stack_frame &frame) = delete; + struct stack_frame &operator=(struct stack_frame &&frame) noexcept; ~stack_frame(); }; @@ -82,8 +84,7 @@ namespace mkxp_sandbox { std::shared_ptr _instance; std::unordered_map> fibers; - public: - + public: binding_base(std::shared_ptr m); ~binding_base(); struct w2c_ruby &instance() const noexcept; @@ -100,10 +101,9 @@ namespace mkxp_sandbox { template struct stack_frame_guard { friend struct binding_base; - private: - - struct binding_base &bind; - struct fiber &fiber; + private: + struct binding_base *bind; + struct fiber *fiber; wasm_ptr_t ptr; static void stack_frame_destructor(void *ptr) { @@ -134,7 +134,6 @@ namespace mkxp_sandbox { if (fiber.stack_ptr == fiber.stack.size()) { std::abort(); } - assert(fiber.stack[fiber.stack_ptr].type == boost::typeindex::type_id()); return fiber.stack[fiber.stack_ptr++].ptr; } @@ -145,10 +144,10 @@ namespace mkxp_sandbox { } ++fiber.stack_ptr; wasm_ptr_t sp = w2c_ruby_rb_wasm_get_stack_pointer(&bind.instance()) - SIZEOF_WASMSTACKALIGN(T); + fiber.stack.reserve(fiber.stack_ptr); fiber.stack.emplace_back( bind, stack_frame_destructor, - boost::typeindex::type_id(), sp ); assert(sp % sizeof(VALUE) == 0); @@ -158,33 +157,47 @@ namespace mkxp_sandbox { return sp; } - stack_frame_guard(struct binding_base &b) : bind(b), fiber(init_fiber(b)), ptr(init_inner(b, fiber)) {} + stack_frame_guard(struct binding_base &b) : bind(&b), fiber(&init_fiber(b)), ptr(init_inner(b, *fiber)) {} - public: + public: + stack_frame_guard(const stack_frame_guard &frame) = delete; + + stack_frame_guard(stack_frame_guard &&frame) noexcept : bind(std::exchange(frame.bind, nullptr)), fiber(std::exchange(frame.fiber, nullptr)), ptr(std::exchange(frame.ptr, 0)) {} + + struct stack_frame_guard &operator=(const stack_frame_guard &frame) = delete; + + struct stack_frame_guard &operator=(stack_frame_guard &&frame) noexcept { + bind = std::exchange(frame.bind, nullptr); + fiber = std::exchange(frame.fiber, nullptr); + ptr = std::exchange(frame.ptr, 0); + return *this; + } ~stack_frame_guard() { - if (get()->is_complete()) { - while (fiber.stack.size() > fiber.stack_ptr) { - fiber.stack.pop_back(); - } - - // Check for stack corruptions - assert(fiber.stack.size() == fiber.stack_ptr); - assert(fiber.stack.back().type == boost::typeindex::type_id()); - - w2c_ruby_rb_wasm_set_stack_pointer(&bind.instance(), fiber.stack.back().ptr + SIZEOF_WASMSTACKALIGN(T)); - fiber.stack.pop_back(); + if (fiber == nullptr) { + return; } - --fiber.stack_ptr; + if (get()->is_complete()) { + while (fiber->stack.size() > fiber->stack_ptr) { + fiber->stack.pop_back(); + } - if (fiber.stack.empty()) { - bind.fibers.erase(fiber.key); + assert(fiber->stack.size() == fiber->stack_ptr); + + w2c_ruby_rb_wasm_set_stack_pointer(&bind->instance(), fiber->stack.back().ptr + SIZEOF_WASMSTACKALIGN(T)); + fiber->stack.pop_back(); + } + + --fiber->stack_ptr; + + if (fiber->stack.empty()) { + bind->fibers.erase(fiber->key); } } inline T *get() const noexcept { - return (T *)(*bind + ptr); + return (T *)(**bind + ptr); } inline T &operator()() const noexcept { diff --git a/binding-sandbox/binding-sandbox.h b/binding-sandbox/binding-sandbox.h index 0f39c42d..ce88688e 100644 --- a/binding-sandbox/binding-sandbox.h +++ b/binding-sandbox/binding-sandbox.h @@ -233,20 +233,27 @@ namespace mkxp_sandbox { VALUE value; VALUE file; wasm_ptr_t ptr; - std::string path_str; + std::string *path_str; VALUE operator()(VALUE path) { BOOST_ASIO_CORO_REENTER (this) { + path_str = NULL; SANDBOX_AWAIT_AND_SET(ptr, rb_string_value_cstr, &path); - path_str = std::string("/mkxp-retro-game/"); - path_str.append((const char *)(**sb() + ptr)); - SANDBOX_AWAIT_AND_SET(file, rb_file_open, path_str.c_str(), "rb"); + path_str = new std::string("/mkxp-retro-game/"); + path_str->append((const char *)(**sb() + ptr)); + SANDBOX_AWAIT_AND_SET(file, rb_file_open, path_str->c_str(), "rb"); SANDBOX_AWAIT_AND_SET(value, rb_marshal_load, file); SANDBOX_AWAIT(rb_io_close, file); } return value; } + + ~coro() { + if (path_str != NULL) { + delete path_str; + } + } ) return sb()->bind()()(path); diff --git a/binding-sandbox/sandbox.h b/binding-sandbox/sandbox.h index 7933283d..c8ef4d9c 100644 --- a/binding-sandbox/sandbox.h +++ b/binding-sandbox/sandbox.h @@ -27,7 +27,7 @@ #include #include "types.h" -#define SANDBOX_COROUTINE(name, definition) struct name : boost::asio::coroutine { BOOST_TYPE_INDEX_REGISTER_CLASS inline name(struct mkxp_sandbox::binding_base &bind) {} definition }; +#define SANDBOX_COROUTINE(name, definition) struct name : boost::asio::coroutine { inline name(struct mkxp_sandbox::binding_base &bind) {} definition }; #define SANDBOX_AWAIT(coroutine, ...) \ do { \ diff --git a/libretro/sandbox-bindgen.rb b/libretro/sandbox-bindgen.rb index 5ea8bbf3..1eb34afc 100644 --- a/libretro/sandbox-bindgen.rb +++ b/libretro/sandbox-bindgen.rb @@ -617,7 +617,6 @@ File.readlines('tags', chomp: true).each do |line| coroutine_declaration = <<~HEREDOC struct #{func_name} : boost::asio::coroutine { friend struct bindings::stack_frame_guard; - BOOST_TYPE_INDEX_REGISTER_CLASS #{coroutine_ret} operator()(#{declaration_args.join(', ')}); #{coroutine_destructor.empty? ? '' : "~#{func_name}();\n "}private: struct binding_base &bind; diff --git a/src/meson.build b/src/meson.build index 7abdae76..3710918c 100755 --- a/src/meson.build +++ b/src/meson.build @@ -47,7 +47,6 @@ if is_libretro cmake.subproject('boost_throw_exception', options: boost_options).dependency('boost_throw_exception'), cmake.subproject('boost_core', options: boost_options).dependency('boost_core'), cmake.subproject('boost_container_hash', options: boost_options).dependency('boost_container_hash'), - cmake.subproject('boost_type_index', options: boost_options).dependency('boost_type_index'), cmake.subproject('boost_type_traits', options: boost_options).dependency('boost_type_traits'), cmake.subproject('boost_optional', options: boost_options).dependency('boost_optional'), ] diff --git a/subprojects/boost_type_index.wrap b/subprojects/boost_type_index.wrap deleted file mode 100644 index 37a778f2..00000000 --- a/subprojects/boost_type_index.wrap +++ /dev/null @@ -1,4 +0,0 @@ -[wrap-git] -url = https://github.com/boostorg/type_index -revision = boost-1.87.0 -depth = 1