Skip to content

Commit c80a256

Browse files
DeusDataDeusData
authored
Merge pull request DeusData#2207 from AmirF194/fix/lifetime-lock-probe-drops-held-lock
fix(daemon): stop the lifetime-lock probe from dropping a held lock
2 parents 7a9b1cb + 7e3024d commit c80a256

2 files changed

Lines changed: 241 additions & 9 deletions

File tree

‎src/daemon/ipc.c‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1263,6 +1263,13 @@ static int posix_named_shared_lock_try_acquire(const cbm_daemon_ipc_endpoint_t *
12631263
return posix_named_lock_try_acquire_mode(endpoint, lock_name, true, fd_out, process_entry_out);
12641264
}
12651265

1266+
/* Never open and close the lifetime lock file in a process that may already
1267+
* hold it: fcntl(2) record locks are scoped to (process, inode), so that
1268+
* close would silently release every lock this process holds on the file,
1269+
* regardless of which fd took it. This lock is fcntl rather than flock for
1270+
* exactly this reason in reverse: flock locks survive a fork, fcntl locks
1271+
* do not, and a forked child must never appear to hold its parent's
1272+
* reservation. */
12661273
static int posix_record_lock_set(int fd, short lock_type) {
12671274
struct flock record_lock = {
12681275
.l_type = lock_type,
@@ -1286,6 +1293,15 @@ static int posix_lifetime_lock_probe(const cbm_daemon_ipc_endpoint_t *endpoint,
12861293
if (process_claimed < 0) {
12871294
return -1;
12881295
}
1296+
if (process_claimed == 1) {
1297+
/* Already held by this process per the in-process registry: trust
1298+
* that alone. Opening a throwaway fd on the lock file here and then
1299+
* closing it would release the real fcntl(2) record lock, which is
1300+
* scoped to (process, inode) rather than (fd, inode): closing any
1301+
* fd on this inode drops every lock this process holds on it, even
1302+
* one held via a different, still-open fd. */
1303+
return endpoint_runtime_still_valid(endpoint) ? 1 : -1;
1304+
}
12891305

12901306
int fd = openat(endpoint->dir_fd, lock_name, O_RDWR | O_CLOEXEC | O_NOFOLLOW);
12911307
if (fd < 0) {
@@ -1297,10 +1313,6 @@ static int posix_lifetime_lock_probe(const cbm_daemon_ipc_endpoint_t *endpoint,
12971313
(void)close(fd);
12981314
return -1;
12991315
}
1300-
if (process_claimed == 1) {
1301-
bool still_private = endpoint_runtime_still_valid(endpoint);
1302-
return close(fd) == 0 && still_private ? 1 : -1;
1303-
}
13041316
struct flock record_lock = {
13051317
.l_type = F_WRLCK,
13061318
.l_whence = SEEK_SET,
@@ -1338,11 +1350,13 @@ static int posix_lifetime_lock_try_acquire(const cbm_daemon_ipc_endpoint_t *endp
13381350
process_lock_entry_t *process_entry = NULL;
13391351
int process_result = process_lock_claim(endpoint, lock_name, &process_entry);
13401352
if (process_result != 1) {
1341-
return process_result == 0 &&
1342-
private_regular_file_at_is_safe(endpoint->dir_fd, lock_name, 1) &&
1343-
endpoint_runtime_still_valid(endpoint)
1344-
? 0
1345-
: -1;
1353+
/* Already held by this process per the in-process registry: trust
1354+
* that alone, the same as the probe's early return above. Calling
1355+
* private_regular_file_at_is_safe() here would open a throwaway fd
1356+
* on the lock file and close it, releasing the real fcntl(2) record
1357+
* lock this process holds, because that lock is scoped to (process,
1358+
* inode) rather than (fd, inode). */
1359+
return process_result == 0 && endpoint_runtime_still_valid(endpoint) ? 0 : -1;
13461360
}
13471361
int fd = openat(endpoint->dir_fd, lock_name, O_RDWR | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0600);
13481362
if (fd < 0 || !fd_set_cloexec(fd)) {

‎tests/test_daemon_ipc.c‎

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3581,6 +3581,222 @@ TEST(daemon_ipc_posix_lifetime_reservation_rejects_fork_inheritance) {
35813581
PASS();
35823582
}
35833583

3584+
static bool ipc_test_lifetime_lock_path(char out[TEST_PATH_CAP], const char *runtime_dir,
3585+
const char *key) {
3586+
int written = runtime_dir && key
3587+
? snprintf(out, TEST_PATH_CAP, "%s/cbm-%s.lifetime.lock", runtime_dir, key)
3588+
: -1;
3589+
return written > 0 && written < TEST_PATH_CAP;
3590+
}
3591+
3592+
/* Child-process verdict on whether the lifetime lock at lock_path is held by
3593+
* someone else, going straight through open()/fcntl() rather than any of
3594+
* this codebase's APIs, so a genuinely separate process cannot inherit the
3595+
* parent's in-process claim registry. Distinguishes three outcomes instead
3596+
* of collapsing "held" and "could not even check" into the same value:
3597+
* IPC_TEST_LOCK_HELD only when open() succeeded and F_SETLK failed with
3598+
* EACCES or EAGAIN (the lock is held by another process, as expected);
3599+
* IPC_TEST_LOCK_TAKEN when F_SETLK succeeded (the lock was free, a bug);
3600+
* IPC_TEST_LOCK_ERROR for anything else (open() failed, e.g. a wrong path,
3601+
* or F_SETLK failed with an errno that says nothing about the lock's
3602+
* state), which must never be mistaken for IPC_TEST_LOCK_HELD. */
3603+
enum {
3604+
IPC_TEST_LOCK_HELD = 0,
3605+
IPC_TEST_LOCK_TAKEN = 1,
3606+
IPC_TEST_LOCK_ERROR = 3,
3607+
};
3608+
3609+
static uint8_t ipc_test_lock_child_verdict(const char *lock_path) {
3610+
int fd = open(lock_path, O_RDWR);
3611+
if (fd < 0) {
3612+
return IPC_TEST_LOCK_ERROR;
3613+
}
3614+
struct flock record_lock = {
3615+
.l_type = F_WRLCK,
3616+
.l_whence = SEEK_SET,
3617+
.l_start = 0,
3618+
.l_len = 0,
3619+
};
3620+
int lock_result;
3621+
do {
3622+
lock_result = fcntl(fd, F_SETLK, &record_lock);
3623+
} while (lock_result != 0 && errno == EINTR);
3624+
int lock_errno = errno;
3625+
(void)close(fd);
3626+
if (lock_result == 0) {
3627+
return IPC_TEST_LOCK_TAKEN;
3628+
}
3629+
return lock_errno == EACCES || lock_errno == EAGAIN ? IPC_TEST_LOCK_HELD : IPC_TEST_LOCK_ERROR;
3630+
}
3631+
3632+
TEST(daemon_ipc_posix_lifetime_reservation_probe_does_not_drop_held_lock) {
3633+
static const char key[] = "c1c1d2d2e3e3f4f4";
3634+
char parent[TEST_PATH_CAP] = {0};
3635+
char runtime_dir[TEST_PATH_CAP] = {0};
3636+
char lock_path[TEST_PATH_CAP] = {0};
3637+
cbm_daemon_ipc_endpoint_t *endpoint = NULL;
3638+
cbm_daemon_ipc_lifetime_reservation_t *reservation = NULL;
3639+
int result_pipe[2] = {-1, -1};
3640+
pid_t child = -1;
3641+
uint8_t child_result = 0;
3642+
int child_status = -1;
3643+
int acquired = -1;
3644+
int held_after_first_probe = -1;
3645+
int held_after_second_probe = -1;
3646+
int free_after_release = -1;
3647+
bool lock_path_ok = false;
3648+
3649+
if (ipc_test_parent_new(parent, "probe-holds-lock")) {
3650+
endpoint = cbm_daemon_ipc_endpoint_new(key, parent);
3651+
}
3652+
if (endpoint) {
3653+
ipc_test_copy_path(runtime_dir, cbm_daemon_ipc_endpoint_runtime_dir(endpoint));
3654+
lock_path_ok = ipc_test_lifetime_lock_path(lock_path, runtime_dir, key);
3655+
acquired = cbm_daemon_ipc_lifetime_reservation_try_acquire(endpoint, &reservation);
3656+
}
3657+
if (reservation) {
3658+
/* Two probes, matching how a real daemon polls its own reservation
3659+
* repeatedly while running. Each probe of an already-held lock used
3660+
* to open a throwaway fd on the lock file and close it. Since
3661+
* fcntl(2) record locks are scoped to (process, inode), that close
3662+
* silently released the real lock every time, regardless of the
3663+
* still-open fd the reservation itself is holding. */
3664+
held_after_first_probe = cbm_daemon_ipc_lifetime_reservation_probe(endpoint);
3665+
held_after_second_probe = cbm_daemon_ipc_lifetime_reservation_probe(endpoint);
3666+
}
3667+
if (reservation && lock_path_ok && pipe(result_pipe) == 0) {
3668+
child = fork();
3669+
}
3670+
if (child == 0) {
3671+
/* A genuinely independent process, not going through any of this
3672+
* codebase's APIs so it cannot inherit the parent's in-process
3673+
* claim registry: try to take the real OS-level lock directly. If
3674+
* the parent's fcntl lock survived the probes above, this fails. */
3675+
(void)close(result_pipe[0]);
3676+
child_result = ipc_test_lock_child_verdict(lock_path);
3677+
bool reported = ipc_test_fd_write_all(result_pipe[1], &child_result, sizeof(child_result));
3678+
(void)close(result_pipe[1]);
3679+
_exit(reported ? 0 : 1);
3680+
}
3681+
if (child > 0) {
3682+
(void)close(result_pipe[1]);
3683+
result_pipe[1] = -1;
3684+
bool received = ipc_test_fd_read_all(result_pipe[0], &child_result, sizeof(child_result));
3685+
(void)close(result_pipe[0]);
3686+
result_pipe[0] = -1;
3687+
if (!received) {
3688+
child_result = 2;
3689+
}
3690+
while (waitpid(child, &child_status, 0) < 0 && errno == EINTR) {}
3691+
}
3692+
cbm_daemon_ipc_lifetime_reservation_release(reservation);
3693+
if (endpoint) {
3694+
free_after_release = cbm_daemon_ipc_lifetime_reservation_probe(endpoint);
3695+
}
3696+
for (size_t index = 0; index < 2; index++) {
3697+
if (result_pipe[index] >= 0) {
3698+
(void)close(result_pipe[index]);
3699+
}
3700+
}
3701+
cbm_daemon_ipc_endpoint_free(endpoint);
3702+
ipc_test_remove_tree(runtime_dir, parent);
3703+
3704+
ASSERT_EQ(acquired, 1);
3705+
ASSERT_TRUE(lock_path_ok);
3706+
ASSERT_EQ(held_after_first_probe, 1);
3707+
ASSERT_EQ(held_after_second_probe, 1);
3708+
ASSERT_GT(child, 0);
3709+
ASSERT_TRUE(WIFEXITED(child_status));
3710+
ASSERT_EQ(WEXITSTATUS(child_status), 0);
3711+
ASSERT_EQ(child_result, IPC_TEST_LOCK_HELD);
3712+
ASSERT_EQ(free_after_release, 0);
3713+
PASS();
3714+
}
3715+
3716+
TEST(daemon_ipc_posix_lifetime_reservation_try_acquire_again_does_not_drop_held_lock) {
3717+
static const char key[] = "a1a1b2b2c3c3d4d4";
3718+
char parent[TEST_PATH_CAP] = {0};
3719+
char runtime_dir[TEST_PATH_CAP] = {0};
3720+
char lock_path[TEST_PATH_CAP] = {0};
3721+
cbm_daemon_ipc_endpoint_t *endpoint = NULL;
3722+
cbm_daemon_ipc_lifetime_reservation_t *reservation = NULL;
3723+
cbm_daemon_ipc_lifetime_reservation_t *second_reservation =
3724+
(cbm_daemon_ipc_lifetime_reservation_t *)(uintptr_t)1;
3725+
int result_pipe[2] = {-1, -1};
3726+
pid_t child = -1;
3727+
uint8_t child_result = 0;
3728+
int child_status = -1;
3729+
int acquired = -1;
3730+
int reacquired = -1;
3731+
int free_after_release = -1;
3732+
bool lock_path_ok = false;
3733+
3734+
if (ipc_test_parent_new(parent, "try-acquire-again-holds-lock")) {
3735+
endpoint = cbm_daemon_ipc_endpoint_new(key, parent);
3736+
}
3737+
if (endpoint) {
3738+
ipc_test_copy_path(runtime_dir, cbm_daemon_ipc_endpoint_runtime_dir(endpoint));
3739+
lock_path_ok = ipc_test_lifetime_lock_path(lock_path, runtime_dir, key);
3740+
acquired = cbm_daemon_ipc_lifetime_reservation_try_acquire(endpoint, &reservation);
3741+
}
3742+
if (reservation) {
3743+
/* This process already holds the lifetime lock. Calling try_acquire
3744+
* again, still in this process, must trust the in-process registry
3745+
* alone and return 0 (already held, nothing new to hand back)
3746+
* without touching the lock file: opening and closing a second fd
3747+
* on it would drop the fcntl(2) record lock this process already
3748+
* holds, exactly the way the probe's own fast path used to. */
3749+
reacquired = cbm_daemon_ipc_lifetime_reservation_try_acquire(endpoint, &second_reservation);
3750+
}
3751+
if (reservation && lock_path_ok && pipe(result_pipe) == 0) {
3752+
child = fork();
3753+
}
3754+
if (child == 0) {
3755+
/* A genuinely independent process, not going through any of this
3756+
* codebase's APIs so it cannot inherit the parent's in-process
3757+
* claim registry: try to take the real OS-level lock directly. If
3758+
* the repeated try_acquire above survived, this fails. */
3759+
(void)close(result_pipe[0]);
3760+
child_result = ipc_test_lock_child_verdict(lock_path);
3761+
bool reported = ipc_test_fd_write_all(result_pipe[1], &child_result, sizeof(child_result));
3762+
(void)close(result_pipe[1]);
3763+
_exit(reported ? 0 : 1);
3764+
}
3765+
if (child > 0) {
3766+
(void)close(result_pipe[1]);
3767+
result_pipe[1] = -1;
3768+
bool received = ipc_test_fd_read_all(result_pipe[0], &child_result, sizeof(child_result));
3769+
(void)close(result_pipe[0]);
3770+
result_pipe[0] = -1;
3771+
if (!received) {
3772+
child_result = 2;
3773+
}
3774+
while (waitpid(child, &child_status, 0) < 0 && errno == EINTR) {}
3775+
}
3776+
cbm_daemon_ipc_lifetime_reservation_release(reservation);
3777+
if (endpoint) {
3778+
free_after_release = cbm_daemon_ipc_lifetime_reservation_probe(endpoint);
3779+
}
3780+
for (size_t index = 0; index < 2; index++) {
3781+
if (result_pipe[index] >= 0) {
3782+
(void)close(result_pipe[index]);
3783+
}
3784+
}
3785+
cbm_daemon_ipc_endpoint_free(endpoint);
3786+
ipc_test_remove_tree(runtime_dir, parent);
3787+
3788+
ASSERT_EQ(acquired, 1);
3789+
ASSERT_TRUE(lock_path_ok);
3790+
ASSERT_EQ(reacquired, 0);
3791+
ASSERT_TRUE(second_reservation == NULL);
3792+
ASSERT_GT(child, 0);
3793+
ASSERT_TRUE(WIFEXITED(child_status));
3794+
ASSERT_EQ(WEXITSTATUS(child_status), 0);
3795+
ASSERT_EQ(child_result, IPC_TEST_LOCK_HELD);
3796+
ASSERT_EQ(free_after_release, 0);
3797+
PASS();
3798+
}
3799+
35843800
TEST(daemon_ipc_posix_child_participant_handoff_retains_legacy_bridge) {
35853801
#ifdef _WIN32
35863802
PASS();
@@ -5999,6 +6215,8 @@ SUITE(daemon_ipc) {
59996215
#endif
60006216
RUN_TEST(daemon_ipc_posix_startup_lock_is_cross_process);
60016217
RUN_TEST(daemon_ipc_posix_lifetime_reservation_rejects_fork_inheritance);
6218+
RUN_TEST(daemon_ipc_posix_lifetime_reservation_probe_does_not_drop_held_lock);
6219+
RUN_TEST(daemon_ipc_posix_lifetime_reservation_try_acquire_again_does_not_drop_held_lock);
60026220
RUN_TEST(daemon_ipc_posix_child_participant_handoff_retains_legacy_bridge);
60036221
RUN_TEST(daemon_ipc_posix_publication_boundaries_recover_from_crash);
60046222
RUN_TEST(daemon_ipc_posix_record_publication_windows_recover_from_crash);

0 commit comments

Comments
 (0)