Skip to content

Complete only the pidfd whose monitor failed #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xalestar wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −18
Open

Complete only the pidfd whose monitor failed #399

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 59 additions & 18 deletions src/syscall/proc-pidfd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@ typedef struct {
int guest_fd;
int64_t guest_pid;
int write_end;
uint64_t gen; /* names this open across guest fd number reuse */

@jserv jserv Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pidfd_cleanup still finds its entry by guest_fd alone, so the reuse race this gen closes for the monitor is still open on the close path. fd_snapshot_and_close marks the slot closed under fd_lock, and then fd_cleanup_entry runs pidfd_cleanup with no lock held. A sibling thread's pidfd_open can get the same fd number in between, and if pidfd_find_free_entry places the new entry at a lower index, the old fd's cleanup tears down the new pidfd and leaves the stale entry active, so pidfd_send_signal on the new fd signals the old target. A follow-up could store the gen in the fd table entry (or pass it through the snapshot) and have cleanup match (guest_fd, gen).

@xalestar xalestar Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: fd_snapshot_and_close releases fd_lock before fd_cleanup_entry calls pidfd_cleanup, and the lookup by guest_fd alone then picks the lower-index entry. Since the cleanup callback only receives the fd number, carrying gen there means storing it in fd_entry_t or passing the snapshot through, which reaches beyond this file. Filed as #403.

} pidfd_entry_t;

static pidfd_entry_t pidfd_table[PIDFD_TABLE_SIZE];
static uint64_t pidfd_next_gen;
static pthread_mutex_t pidfd_lock = PTHREAD_MUTEX_INITIALIZER;

static pidfd_entry_t *pidfd_find_free_entry(void)
Expand All @@ -53,6 +55,30 @@ static pidfd_entry_t *pidfd_find_guest_fd_entry(int guest_fd)

static void pidfd_cleanup(int guest_fd);

/* Caller holds pidfd_lock. */
static void pidfd_complete_entry(pidfd_entry_t *entry)
{
if (entry->write_end < 0)
return;
uint8_t byte = 0;
(void) write(entry->write_end, &byte, 1);
close(entry->write_end);
entry->write_end = -1;
}

/* Complete one pidfd without touching others that watch the same target. */
static void pidfd_complete_one(uint64_t gen)
{
pthread_mutex_lock(&pidfd_lock);
for (int i = 0; i < PIDFD_TABLE_SIZE; i++) {
if (pidfd_table[i].active && pidfd_table[i].gen == gen) {
pidfd_complete_entry(&pidfd_table[i]);
break;
}
}
pthread_mutex_unlock(&pidfd_lock);
}

void pidfd_init(void)
{
fd_register_cleanup(FD_PIDFD, pidfd_cleanup);
Expand All @@ -73,9 +99,10 @@ static void pidfd_cleanup(int guest_fd)

static void *pidfd_monitor_thread(void *arg)
{
int64_t *pids = (int64_t *) arg;
int64_t gpid = pids[0];
pid_t hpid = (pid_t) pids[1];
int64_t *ctx = (int64_t *) arg;
int64_t gpid = ctx[0];
pid_t hpid = (pid_t) ctx[1];
uint64_t gen = (uint64_t) ctx[2];
free(arg);

if (kill(hpid, 0) < 0 && errno == ESRCH) {
Expand All @@ -84,23 +111,38 @@ static void *pidfd_monitor_thread(void *arg)
}

int kq = kqueue();
if (kq < 0)
if (kq < 0) {

@jserv jserv Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says a guest cannot make kqueue or pthread_create fail, but closing a pidfd never stops its detached monitor. A guest that repeatedly opens and closes a pidfd on a long-lived child leaves one thread blocked in kevent plus one kqueue fd behind per open. The table holds only 32 active entries, but closing frees a slot, so these leaks have no limit. That makes these failure paths reachable from the guest, and that is the case that deserves a test. A follow-up could give each monitor a cancel path from pidfd_cleanup (for example an EVFILT_USER event on its kqueue that cleanup triggers) so a close also tears down the monitor.

@xalestar xalestar Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the description was wrong on that point; the monitor leak in #400 makes these paths reachable, and I have corrected it. A test that drives the leak until kqueue() fails stops being possible once #400 is fixed, so I would pair the test with that fix: open and close pidfds on a live child in a loop and check the host thread count stays flat.

pidfd_complete_one(gen);
return NULL;
}

struct kevent ev;
EV_SET(&ev, hpid, EVFILT_PROC, EV_ADD | EV_ONESHOT, NOTE_EXIT, 0, NULL);
if (kevent(kq, &ev, 1, NULL, 0, NULL) < 0) {
/* ESRCH means the target exited before registration, which every pidfd
* on it should see. Any other failure says nothing about the target, so
* only this fd is completed.
*/
bool gone = errno == ESRCH;

@cubic-dev-ai cubic-dev-ai Bot Oct 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The EV_ADD registration is not retried on EINTR, but the blocking wait below it is. macOS kevent(2) documents that when a change-list call fails with EINTR, all changes have already been applied — so an EINTR here means the EVFILT_PROC filter is installed, yet this path closes kq, discards the working monitor, and completes only this pidfd while the target may still be alive. Retry the registration on EINTR (EV_ADD is idempotent) like the wait loop does; nothing else in this block changes.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/syscall/proc-pidfd.c, line 126:

<comment>The `EV_ADD` registration is not retried on `EINTR`, but the blocking wait below it is. macOS kevent(2) documents that when a change-list call fails with `EINTR`, all changes have already been applied — so an `EINTR` here means the `EVFILT_PROC` filter is installed, yet this path closes `kq`, discards the working monitor, and completes only this pidfd while the target may still be alive. Retry the registration on `EINTR` (EV_ADD is idempotent) like the wait loop does; nothing else in this block changes.</comment>

<file context>
@@ -84,23 +111,38 @@ static void *pidfd_monitor_thread(void *arg)
+         * on it should see. Any other failure says nothing about the target, so
+         * only this fd is completed.
+         */
+        bool gone = errno == ESRCH;
         close(kq);
-        proc_pidfd_notify_exit(gpid);
</file context>

@xalestar xalestar Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence about changes being applied on EINTR is from the FreeBSD page; the macOS kevent(2) page does not say it. This call passes nevents = 0, so it returns without waiting and has nothing for a signal to interrupt, and elfuse blocks its async signals on all threads except the sigwait thread. The final wait is retried because it does block.

close(kq);
proc_pidfd_notify_exit(gpid);
if (gone)
proc_pidfd_notify_exit(gpid);
else
pidfd_complete_one(gen);
return NULL;
}

struct kevent out;
int n = kevent(kq, NULL, 0, &out, 1, NULL);
int n;
do {
n = kevent(kq, NULL, 0, &out, 1, NULL);
} while (n < 0 && errno == EINTR);
close(kq);

if (n > 0 && out.filter == EVFILT_PROC)
proc_pidfd_notify_exit(gpid);
else
pidfd_complete_one(gen);

return NULL;
}
Expand Down Expand Up @@ -139,6 +181,8 @@ int pidfd_create(guest_t *g, int64_t target_pid, pid_t host_pid)
entry->guest_fd = gfd;
entry->guest_pid = target_pid;
entry->write_end = pfd[1];
entry->gen = ++pidfd_next_gen;
uint64_t gen = entry->gen;
pthread_mutex_unlock(&pidfd_lock);

/* host_pid <= 0 means the target lives inside this host process -- the
Expand All @@ -151,10 +195,11 @@ int pidfd_create(guest_t *g, int64_t target_pid, pid_t host_pid)

bool monitor_ok = false;
{
int64_t *ctx = malloc(2 * sizeof(int64_t));
int64_t *ctx = malloc(3 * sizeof(int64_t));
if (ctx) {
ctx[0] = target_pid;
ctx[1] = (int64_t) host_pid;
ctx[2] = (int64_t) gen;
pthread_t thr;
pthread_attr_t attr;
if (pthread_attr_init(&attr) == 0) {
Expand All @@ -174,12 +219,13 @@ int pidfd_create(guest_t *g, int64_t target_pid, pid_t host_pid)
}

/* Nothing will ever mark this fd readable without a monitor behind it, so
* complete it rather than leave the guest polling forever. A target that
* has already exited needs no special case: the monitor thread finds it
* gone and completes the fd the same way.
* complete it rather than leave the guest polling forever. Only this fd:
* other pidfds on the same live target keep their own monitors. A target
* that has already exited needs no special case: the monitor thread finds
* it gone and completes the fd the same way.
*/
if (!monitor_ok)
proc_pidfd_notify_exit(target_pid);
pidfd_complete_one(gen);

return gfd;
}
Expand All @@ -188,13 +234,8 @@ void proc_pidfd_notify_exit(int64_t exited_pid)
{
pthread_mutex_lock(&pidfd_lock);
for (int i = 0; i < PIDFD_TABLE_SIZE; i++) {
if (pidfd_table[i].active && pidfd_table[i].guest_pid == exited_pid &&
pidfd_table[i].write_end >= 0) {
uint8_t byte = 0;
(void) write(pidfd_table[i].write_end, &byte, 1);
close(pidfd_table[i].write_end);
pidfd_table[i].write_end = -1;
}
if (pidfd_table[i].active && pidfd_table[i].guest_pid == exited_pid)
pidfd_complete_entry(&pidfd_table[i]);
}
pthread_mutex_unlock(&pidfd_lock);
}
Expand Down
Loading