From 37f16cea4715e3cc3c2b20d132610fd863057f21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E5=A4=B4=E4=BA=91?= Date: Sat, 6 Dec 2025 06:52:56 +0000 Subject: [PATCH 1/2] Fix mutex unlock timing to avoid interfering with other references Problem: The previous fix unconditionally called pthread_mutex_unlock() at the beginning of close(), which could interfere with other threads/processes that still had valid references to the mutex. This caused test failures on FreeBSD when running tests multiple times (ShmTest.RemoveByName would fail on the second run). Root cause: Calling unlock() too early could affect the mutex state for other references that are still using it, leading to unexpected behavior. Solution: Move pthread_mutex_unlock() to only be called when we're about to destroy the mutex (i.e., when we're the last reference: shm_->ref() <= 1 && self_ref <= 1). This ensures: 1. We don't interfere with other threads/processes using the mutex 2. We still unlock before destroying to avoid FreeBSD robust list issues 3. The unlock happens at the correct time - right before pthread_mutex_destroy() This is the correct approach because: - Only the last reference holder should clean up the mutex - Unlocking should be paired with destroying for the final cleanup - Other references should not be affected by one reference closing Fixes the second-run test failure on FreeBSD while maintaining the segfault fix. --- src/libipc/platform/posix/mutex.h | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/libipc/platform/posix/mutex.h b/src/libipc/platform/posix/mutex.h index 8a4ac5f..a19ace8 100644 --- a/src/libipc/platform/posix/mutex.h +++ b/src/libipc/platform/posix/mutex.h @@ -150,21 +150,19 @@ public: void close() noexcept { if ((ref_ != nullptr) && (shm_ != nullptr) && (mutex_ != nullptr)) { - // Try to unlock the mutex before destroying it. - // This is important for robust mutexes on FreeBSD, which maintain - // a per-thread robust list. If we destroy a mutex while it's in - // the robust list (even if not locked), FreeBSD may encounter - // dangling pointers later, leading to segfaults. - // We ignore any errors from unlock() since: - // 1. If we don't hold the lock, EPERM is expected and harmless - // 2. If the mutex is already unlocked, this is a no-op - // 3. If there's an error, we still want to proceed with cleanup - ::pthread_mutex_unlock(mutex_); - if (shm_->name() != nullptr) { release_mutex(shm_->name(), [this] { auto self_ref = ref_->fetch_sub(1, std::memory_order_relaxed); if ((shm_->ref() <= 1) && (self_ref <= 1)) { + // Before destroying the mutex, try to unlock it. + // This is important for robust mutexes on FreeBSD, which maintain + // a per-thread robust list. If we destroy a mutex while it's locked + // or still in the robust list, FreeBSD may encounter dangling pointers + // later, leading to segfaults. + // Only unlock here (when we're the last reference) to avoid + // interfering with other threads that might be using the mutex. + ::pthread_mutex_unlock(mutex_); + int eno; if ((eno = ::pthread_mutex_destroy(mutex_)) != 0) { ipc::error("fail pthread_mutex_destroy[%d]\n", eno); @@ -182,11 +180,11 @@ public: void clear() noexcept { if ((shm_ != nullptr) && (mutex_ != nullptr)) { - // Try to unlock before destroying, same reasoning as in close() - ::pthread_mutex_unlock(mutex_); - if (shm_->name() != nullptr) { release_mutex(shm_->name(), [this] { + // Unlock before destroying, same reasoning as in close() + ::pthread_mutex_unlock(mutex_); + int eno; if ((eno = ::pthread_mutex_destroy(mutex_)) != 0) { ipc::error("fail pthread_mutex_destroy[%d]\n", eno); From 2593213d9c0746d8e6ae830bebb461f82dceddff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9C=A8=E5=A4=B4=E4=BA=91?= Date: Sat, 6 Dec 2025 07:12:39 +0000 Subject: [PATCH 2/2] Fix FreeBSD shm_unlink failure by adding '/' prefix in remove() Problem: Tests fail on the second run on FreeBSD with ShmTest.RemoveByName failing. After the first test run completes, subsequent runs fail because shared memory objects are not properly removed. Root cause: FreeBSD's shm_unlink() is stricter than Linux about POSIX compliance. The remove(char const * name) function was calling shm_unlink() without the '/' prefix, while acquire() was using '/'+name format. This inconsistency caused: - Linux: Silently tolerates both /name and name formats - FreeBSD: Strictly requires /name format, shm_unlink("name") fails When shm_unlink() fails to remove the shared memory object: 1. First test run creates /remove_by_name_test_1 2. Test calls shm::remove("remove_by_name_test_1") 3. shm_unlink("remove_by_name_test_1") fails on FreeBSD (missing '/') 4. Shared memory object remains in the system 5. Second test run tries to reuse the same name -> conflict -> test fails Solution: 1. Fix remove(char const * name) to prepend '/' to the name for consistency with acquire() function, ensuring POSIX compliance 2. Add error checking for all shm_unlink() calls to log failures with errno This ensures proper cleanup on FreeBSD and maintains compatibility with Linux. Changes: - Modified remove(char const * name) to use '/'+name format - Added error logging for all three shm_unlink() calls - Now consistent with POSIX requirement: shared memory names must be /somename Tested on FreeBSD 15: Multiple consecutive test runs now pass without failures. --- src/libipc/platform/posix/shm_posix.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/libipc/platform/posix/shm_posix.cpp b/src/libipc/platform/posix/shm_posix.cpp index 968a202..bcf7918 100644 --- a/src/libipc/platform/posix/shm_posix.cpp +++ b/src/libipc/platform/posix/shm_posix.cpp @@ -173,7 +173,10 @@ std::int32_t release(id_t id) noexcept { else if ((ret = acc_of(ii->mem_, ii->size_).fetch_sub(1, std::memory_order_acq_rel)) <= 1) { ::munmap(ii->mem_, ii->size_); if (!ii->name_.empty()) { - ::shm_unlink(ii->name_.c_str()); + int unlink_ret = ::shm_unlink(ii->name_.c_str()); + if (unlink_ret == -1) { + ipc::error("fail shm_unlink[%d]: %s\n", errno, ii->name_.c_str()); + } } } else ::munmap(ii->mem_, ii->size_); @@ -190,7 +193,10 @@ void remove(id_t id) noexcept { auto name = std::move(ii->name_); release(id); if (!name.empty()) { - ::shm_unlink(name.c_str()); + int unlink_ret = ::shm_unlink(name.c_str()); + if (unlink_ret == -1) { + ipc::error("fail shm_unlink[%d]: %s\n", errno, name.c_str()); + } } } @@ -199,7 +205,12 @@ void remove(char const * name) noexcept { ipc::error("fail remove: name is empty\n"); return; } - ::shm_unlink(name); + // For portable use, a shared memory object should be identified by name of the form /somename. + ipc::string op_name = ipc::string{"/"} + name; + int unlink_ret = ::shm_unlink(op_name.c_str()); + if (unlink_ret == -1) { + ipc::error("fail shm_unlink[%d]: %s\n", errno, op_name.c_str()); + } } } // namespace shm