Report a pty hangup on every path that can observe one - #278
Conversation
9c25676 to
f7bc240
Compare
26891a2 to
3e8dfce
Compare
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/runtime/procemu.c">
<violation number="1" location="src/runtime/procemu.c:2462">
P1: A forked child killed by `SIGKILL` can leave the parent master waiting forever for `POLLHUP`: the host closes the child's slave, but no cleanup runs and `shared->slave_count` stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.</violation>
</file>
<file name="src/runtime/forkipc.c">
<violation number="1" location="src/runtime/forkipc.c:1964">
P2: A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| pty_guest_slave_record_locked(dst_slave_host_fd, pts_num, true); | ||
| } | ||
|
|
||
| void proc_pty_release_process_slaves(void) |
There was a problem hiding this comment.
P1: A forked child killed by SIGKILL can leave the parent master waiting forever for POLLHUP: the host closes the child's slave, but no cleanup runs and shared->slave_count stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/runtime/procemu.c, line 2462:
<comment>A forked child killed by `SIGKILL` can leave the parent master waiting forever for `POLLHUP`: the host closes the child's slave, but no cleanup runs and `shared->slave_count` stays positive. The accounting needs a death-tolerant reconciliation mechanism instead of relying solely on process-exit cleanup and pty-path reuse.</comment>
<file context>
@@ -2078,25 +2388,184 @@ void proc_pty_note_guest_slave(int slave_host_fd, uint32_t linux_pts_num)
+ pty_guest_slave_record_locked(dst_slave_host_fd, pts_num, true);
+}
+
+void proc_pty_release_process_slaves(void)
+{
+ /* Hand back every slave this process still holds, at process teardown.
</file context>
| * would stop reporting hangups for good. Still before siblings resume, so | ||
| * no guest code can close a slave in between. | ||
| */ | ||
| proc_pty_fork_parent_note_inherited(); |
There was a problem hiding this comment.
P2: A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/runtime/forkipc.c, line 1964:
<comment>A forked child that dies before it runs proc_pty_adopt_inherited_slaves() leaks the parent's per-slave credit in the shared segment, so that pty's master stops reporting hangups permanently. The parent adds one to shared->slave_count per inherited slave in proc_pty_fork_parent_note_inherited() right after it commits the child, and that credit is only returned when the child's local count (set by adopt) is subtracted in proc_pty_release_process_slaves(). The one bail path that runs before adopt -- the fork_ipc_recv_pty_keepalives failure at the top of fork_child_main -- both skips the release call and would not repay the credit even if it called it, because the local count is still 0. The other four bail paths added in this diff all call proc_pty_release_process_slaves(), so this path is an inconsistency and a real leak. Suggest having the child also give back the parent's pre-credit when it bails before adopt (or deferring the parent's credit until the child has adopted), so an aborted fork cannot wedge a master's hangup indefinitely.</comment>
<file context>
@@ -1911,6 +1951,18 @@ int64_t sys_clone(hv_vcpu_t vcpu,
+ * would stop reporting hangups for good. Still before siblings resume, so
+ * no guest code can close a slave in between.
+ */
+ proc_pty_fork_parent_note_inherited();
+
/* The process-state payload includes the SCM_RIGHTS handoff for region
</file context>
142ce7f to
aaea84c
Compare
aaea84c to
f40df62
Compare
jserv
left a comment
There was a problem hiding this comment.
Rebase latest main branch and resolve conflicts.
epoll_wait never reported EPOLLHUP for a pty master whose slaves had all closed, and slaves obtained through TIOCGPTPEER were never registered, so the master's hangup accounting missed them. Wire both, and re-verify the fd generation a registration pinned at ADD/MOD so a close-and-reuse in the lookup window cannot charge the hangup to an unrelated file. Drop proc_pty_any_master_hung_up(). It was meant as a cheap gate that let epoll skip the registration sweep, but it latches: a master that hangs up while the guest still holds its fd keeps the gate true for the process's lifetime, and every epoll_pwait then paid for a full FD_TABLE_SIZE sweep. Track per-instance active and pty-master counts instead, so the scan is bounded by what the instance actually registered and skipped entirely when it holds no pty master. Delete kqueue events whose guest fd is no longer registered. A level-triggered event for a deregistered fd is re-reported on every call and translates to zero epoll events, which spins the caller. Keep an epoll registration alive while another fd still refers to the same open file description, matching Linux, and identify that description by ofd_id at the close chokepoint. Return ready events in preference to EINTR. Linux ep_poll() checks ep_events_available() and jumps to send_events before it consults signal_pending, so EINTR is only the answer for a wait that produced nothing. Discarding a ready fd to report EINTR loses it in practice: kqueue re-reports it, but the same pending signal is still there, so the caller is handed EINTR forever and never drains the fd. A terminal with an unhandled SIGCHLD spun at 100% CPU this way without ever drawing. Fix sysprog21#274
f40df62 to
26de80c
Compare
|
Updated |
Report a pty hangup on every path that can observe one
epoll_wait never reported EPOLLHUP for a pty master whose slaves had all
closed, and slaves obtained through TIOCGPTPEER were never registered, so
the master's hangup accounting missed them. Wire both, and re-verify the
fd generation a registration pinned at ADD/MOD so a close-and-reuse in the
lookup window cannot charge the hangup to an unrelated file.
Drop proc_pty_any_master_hung_up(). It was meant as a cheap gate that let
epoll skip the registration sweep, but it latches: a master that hangs up
while the guest still holds its fd keeps the gate true for the process's
lifetime, and every epoll_pwait then paid for a full FD_TABLE_SIZE sweep.
Track per-instance active and pty-master counts instead, so the scan is
bounded by what the instance actually registered and skipped entirely when
it holds no pty master.
Delete kqueue events whose guest fd is no longer registered. A
level-triggered event for a deregistered fd is re-reported on every call
and translates to zero epoll events, which spins the caller.
Keep an epoll registration alive while another fd still refers to the same
open file description, matching Linux, and identify that description by
ofd_id at the close chokepoint.
Return ready events in preference to EINTR. Linux ep_poll() checks
ep_events_available() and jumps to send_events before it consults
signal_pending, so EINTR is only the answer for a wait that produced
nothing. Discarding a ready fd to report EINTR loses it in practice: kqueue
re-reports it, but the same pending signal is still there, so the caller is
handed EINTR forever and never drains the fd. A terminal with an unhandled
SIGCHLD spun at 100% CPU this way without ever drawing.
Fix #274
Summary by cubic
Make pty hangups visible on every path and across processes. poll, epoll, read, and readv now report hangups reliably; epoll stamps EPOLLHUP immediately and reads return EIO after draining pending data.
fd_current_generation; resolve host fd and generation atomically.Written for commit 26de80c. Summary will update on new commits.