From 8c02f37ff9e6f26d168cb08d0caa1aab7800afe4 Mon Sep 17 00:00:00 2001 From: mutouyun Date: Sun, 24 Sep 2023 18:02:37 +0800 Subject: [PATCH] Fixed memory access exception in multithreading. --- 3rdparty/gtest/src/gtest-death-test.cc | 4 ++-- src/libipc/ipc.cpp | 30 ++++++++++++++++--------- src/libipc/platform/posix/shm_posix.cpp | 16 ++++++++----- test/test_ipc.cpp | 10 ++++++--- test/test_sync.cpp | 2 +- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/3rdparty/gtest/src/gtest-death-test.cc b/3rdparty/gtest/src/gtest-death-test.cc index da09a1c..7419ba2 100644 --- a/3rdparty/gtest/src/gtest-death-test.cc +++ b/3rdparty/gtest/src/gtest-death-test.cc @@ -1296,8 +1296,8 @@ static void StackLowerThanAddress(const void* ptr, bool* result) { GTEST_ATTRIBUTE_NO_SANITIZE_ADDRESS_ GTEST_ATTRIBUTE_NO_SANITIZE_HWADDRESS_ static bool StackGrowsDown() { - int dummy; - bool result; + int dummy {}; + bool result {}; StackLowerThanAddress(&dummy, &result); return result; } diff --git a/src/libipc/ipc.cpp b/src/libipc/ipc.cpp index 3f3d317..38e3f1d 100755 --- a/src/libipc/ipc.cpp +++ b/src/libipc/ipc.cpp @@ -182,6 +182,7 @@ struct chunk_info_t { auto& chunk_storages() { class chunk_handle_t { ipc::unordered_map handles_; + std::mutex lock_; static bool make_handle(ipc::shm::handle &h, ipc::string const &shm_name, std::size_t chunk_size) { if (!h.valid() && @@ -197,11 +198,15 @@ auto& chunk_storages() { chunk_info_t *get_info(conn_info_head *inf, std::size_t chunk_size) { ipc::string pref {(inf == nullptr) ? ipc::string{} : inf->prefix_}; ipc::string shm_name {ipc::make_prefix(pref, {"CHUNK_INFO__", ipc::to_string(chunk_size)})}; - ipc::shm::handle &h = handles_[pref]; - if (!make_handle(h, shm_name, chunk_size)) { - return nullptr; + ipc::shm::handle *h; + { + std::lock_guard guard {lock_}; + h = &(handles_[pref]); + if (!make_handle(*h, shm_name, chunk_size)) { + return nullptr; + } } - auto *info = static_cast(h.get()); + auto *info = static_cast(h->get()); if (info == nullptr) { ipc::error("[chunk_storages] chunk_shm.id_info_.get failed: chunk_size = %zd\n", chunk_size); return nullptr; @@ -209,7 +214,9 @@ auto& chunk_storages() { return info; } }; - static ipc::map chunk_hs; + using deleter_t = void (*)(chunk_handle_t*); + using chunk_handle_ptr_t = std::unique_ptr; + static ipc::map chunk_hs; return chunk_hs; } @@ -220,13 +227,17 @@ chunk_info_t *chunk_storage_info(conn_info_head *inf, std::size_t chunk_size) { static ipc::rw_lock lock; IPC_UNUSED_ std::shared_lock guard {lock}; if ((it = storages.find(chunk_size)) == storages.end()) { - using chunk_handle_t = std::decay_t::value_type::second_type; + using chunk_handle_ptr_t = std::decay_t::value_type::second_type; + using chunk_handle_t = chunk_handle_ptr_t::element_type; guard.unlock(); IPC_UNUSED_ std::lock_guard guard {lock}; - it = storages.emplace(chunk_size, chunk_handle_t{}).first; + it = storages.emplace(chunk_size, chunk_handle_ptr_t{ + ipc::mem::alloc(), [](chunk_handle_t *p) { + ipc::mem::destruct(p); + }}).first; } } - return it->second.get_info(inf, chunk_size); + return it->second->get_info(inf, chunk_size); } std::pair acquire_storage(conn_info_head *inf, std::size_t size, ipc::circ::cc_t conns) { @@ -356,8 +367,7 @@ struct queue_generator { "QU_CONN__", ipc::to_string(DataSize), "__", ipc::to_string(AlignSize), "__", - name}).c_str()} { - } + name}).c_str()} {} void disconnect_receiver() { bool dis = que_.disconnect(); diff --git a/src/libipc/platform/posix/shm_posix.cpp b/src/libipc/platform/posix/shm_posix.cpp index c111aa5..e29bd6f 100644 --- a/src/libipc/platform/posix/shm_posix.cpp +++ b/src/libipc/platform/posix/shm_posix.cpp @@ -49,6 +49,9 @@ id_t acquire(char const * name, std::size_t size, unsigned mode) { ipc::error("fail acquire: name is empty\n"); return nullptr; } + // For portable use, a shared memory object should be identified by name of the form /somename. + // see: https://man7.org/linux/man-pages/man3/shm_open.3.html + ipc::string op_name = ipc::string{"/"} + name; // Open the object for read-write access. int flag = O_RDWR; switch (mode) { @@ -65,17 +68,17 @@ id_t acquire(char const * name, std::size_t size, unsigned mode) { flag |= O_CREAT; break; } - int fd = ::shm_open(name, flag, S_IRUSR | S_IWUSR | - S_IRGRP | S_IWGRP | - S_IROTH | S_IWOTH); + int fd = ::shm_open(op_name.c_str(), flag, S_IRUSR | S_IWUSR | + S_IRGRP | S_IWGRP | + S_IROTH | S_IWOTH); if (fd == -1) { - ipc::error("fail shm_open[%d]: %s\n", errno, name); + ipc::error("fail shm_open[%d]: %s\n", errno, op_name.c_str()); return nullptr; } auto ii = mem::alloc(); ii->fd_ = fd; ii->size_ = size; - ii->name_ = name; + ii->name_ = std::move(op_name); return ii; } @@ -158,7 +161,8 @@ std::int32_t release(id_t id) { std::int32_t ret = -1; auto ii = static_cast(id); if (ii->mem_ == nullptr || ii->size_ == 0) { - ipc::error("fail release: invalid id (mem = %p, size = %zd)\n", ii->mem_, ii->size_); + ipc::error("fail release: invalid id (mem = %p, size = %zd), name = %s\n", + ii->mem_, ii->size_, ii->name_.c_str()); } else if ((ret = acc_of(ii->mem_, ii->size_).fetch_sub(1, std::memory_order_acq_rel)) <= 1) { ::munmap(ii->mem_, ii->size_); diff --git a/test/test_ipc.cpp b/test/test_ipc.cpp index a1f0108..fb46670 100755 --- a/test/test_ipc.cpp +++ b/test/test_ipc.cpp @@ -151,11 +151,15 @@ void test_sr(char const * name, int s_cnt, int r_cnt) { } // internal-linkage -TEST(IPC, basic) { +TEST(IPC, basic_ssu) { test_basic("ssu"); - //test_basic("smu"); - //test_basic("mmu"); +} + +TEST(IPC, basic_smb) { test_basic("smb"); +} + +TEST(IPC, basic_mmb) { test_basic("mmb"); } diff --git a/test/test_sync.cpp b/test/test_sync.cpp index 4cb82e6..e905df9 100644 --- a/test/test_sync.cpp +++ b/test/test_sync.cpp @@ -131,7 +131,7 @@ TEST(Sync, Condition) { { std::lock_guard guard {lock}; while (que.empty()) { - EXPECT_TRUE(cond.wait(lock, 1000)); + ASSERT_TRUE(cond.wait(lock, 1000)); } val = que.front(); que.pop_front();