Skip to content

Commit 543672b

Browse files
committed
Fix FreeBSD-specific issues in POSIX implementation
This commit addresses the test failures reported in issue #156 on FreeBSD platform. 1. Fix POSIX semaphore naming for FreeBSD compatibility - FreeBSD requires semaphore names to start with "/" - Add "_sem" suffix to avoid namespace conflicts with shm - Store semaphore name separately for proper cleanup - This fixes all 24 semaphore test failures 2. Fix robust mutex EOWNERDEAD handling - When pthread_mutex_lock returns EOWNERDEAD, the lock is already acquired by the calling thread - After calling pthread_mutex_consistent(), we should return success immediately, not unlock and retry - Previous behavior caused issues with FreeBSD's libthr robust mutex list management, leading to fatal errors - This fixes the random crashes in MutexTest Technical details: - EOWNERDEAD indicates the previous lock owner died while holding the lock, but the current thread has successfully acquired it - pthread_mutex_consistent() restores the mutex to a consistent state - The Linux implementation worked differently, but the new approach is more correct according to POSIX semantics and works on both Linux and FreeBSD Fixes #156
1 parent 006a46e commit 543672b

File tree

2 files changed

+35
-23
lines changed

2 files changed

+35
-23
lines changed

src/libipc/platform/posix/mutex.h

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -214,13 +214,13 @@ class mutex {
214214
ipc::error("fail pthread_mutex_lock[%d], pthread_mutex_consistent[%d]\n", eno, eno2);
215215
return false;
216216
}
217-
int eno3 = ::pthread_mutex_unlock(mutex_);
218-
if (eno3 != 0) {
219-
ipc::error("fail pthread_mutex_lock[%d], pthread_mutex_unlock[%d]\n", eno, eno3);
220-
return false;
221-
}
217+
// EOWNERDEAD means we have successfully acquired the lock,
218+
// but the previous owner died. After calling pthread_mutex_consistent(),
219+
// the mutex is now in a consistent state and we hold the lock.
220+
// We should return success here, not unlock and retry.
221+
// This avoids issues with FreeBSD's robust mutex list management.
222+
return true;
222223
}
223-
break; // loop again
224224
default:
225225
ipc::error("fail pthread_mutex_lock[%d]\n", eno);
226226
return false;
@@ -244,15 +244,11 @@ class mutex {
244244
int eno2 = ::pthread_mutex_consistent(mutex_);
245245
if (eno2 != 0) {
246246
ipc::error("fail pthread_mutex_timedlock[%d], pthread_mutex_consistent[%d]\n", eno, eno2);
247-
break;
248-
}
249-
int eno3 = ::pthread_mutex_unlock(mutex_);
250-
if (eno3 != 0) {
251-
ipc::error("fail pthread_mutex_timedlock[%d], pthread_mutex_unlock[%d]\n", eno, eno3);
252-
break;
247+
throw std::system_error{eno2, std::system_category()};
253248
}
249+
// Successfully acquired the lock after making it consistent
250+
return true;
254251
}
255-
break;
256252
default:
257253
ipc::error("fail pthread_mutex_timedlock[%d]\n", eno);
258254
break;

src/libipc/platform/posix/semaphore_impl.h

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ namespace sync {
1919
class semaphore {
2020
ipc::shm::handle shm_;
2121
sem_t *h_ = SEM_FAILED;
22+
std::string sem_name_; // Store the actual semaphore name used
2223

2324
public:
2425
semaphore() = default;
@@ -38,9 +39,16 @@ class semaphore {
3839
ipc::error("[open_semaphore] fail shm.acquire: %s\n", name);
3940
return false;
4041
}
41-
h_ = ::sem_open(name, O_CREAT, 0666, static_cast<unsigned>(count));
42+
// POSIX semaphore names must start with "/" on some platforms (e.g., FreeBSD)
43+
// Use a separate namespace for semaphores to avoid conflicts with shm
44+
if (name[0] == '/') {
45+
sem_name_ = std::string(name) + "_sem";
46+
} else {
47+
sem_name_ = std::string("/") + name + "_sem";
48+
}
49+
h_ = ::sem_open(sem_name_.c_str(), O_CREAT, 0666, static_cast<unsigned>(count));
4250
if (h_ == SEM_FAILED) {
43-
ipc::error("fail sem_open[%d]: %s\n", errno, name);
51+
ipc::error("fail sem_open[%d]: %s\n", errno, sem_name_.c_str());
4452
return false;
4553
}
4654
return true;
@@ -52,14 +60,14 @@ class semaphore {
5260
ipc::error("fail sem_close[%d]: %s\n", errno);
5361
}
5462
h_ = SEM_FAILED;
55-
if (shm_.name() != nullptr) {
56-
std::string name = shm_.name();
63+
if (!sem_name_.empty() && shm_.name() != nullptr) {
5764
if (shm_.release() <= 1) {
58-
if (::sem_unlink(name.c_str()) != 0) {
59-
ipc::error("fail sem_unlink[%d]: %s, name: %s\n", errno, name.c_str());
65+
if (::sem_unlink(sem_name_.c_str()) != 0) {
66+
ipc::error("fail sem_unlink[%d]: %s, name: %s\n", errno, sem_name_.c_str());
6067
}
6168
}
6269
}
70+
sem_name_.clear();
6371
}
6472

6573
void clear() noexcept {
@@ -69,14 +77,22 @@ class semaphore {
6977
}
7078
h_ = SEM_FAILED;
7179
}
72-
char const *name = shm_.name();
73-
if (name == nullptr) return;
74-
::sem_unlink(name);
80+
if (!sem_name_.empty()) {
81+
::sem_unlink(sem_name_.c_str());
82+
sem_name_.clear();
83+
}
7584
shm_.clear(); // Make sure the storage is cleaned up.
7685
}
7786

7887
static void clear_storage(char const *name) noexcept {
79-
::sem_unlink(name);
88+
// Construct the semaphore name same way as open() does
89+
std::string sem_name;
90+
if (name[0] == '/') {
91+
sem_name = std::string(name) + "_sem";
92+
} else {
93+
sem_name = std::string("/") + name + "_sem";
94+
}
95+
::sem_unlink(sem_name.c_str());
8096
ipc::shm::handle::clear_storage(name);
8197
}
8298

0 commit comments

Comments
 (0)