Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
#include "core/sysroot.h"

#include "runtime/forkipc.h"
#include "runtime/futex.h" /* futex_interrupt_request */
#include "runtime/futex.h" /* futex_interrupt_request */
#include "runtime/procemu.h" /* proc_pty_release_process_slaves */
#include "runtime/proctitle.h"
#include "runtime/thread.h"

Expand Down Expand Up @@ -762,6 +763,14 @@ int main(int argc, char **argv)
* temp ELF, which the post-prepare error paths and a Rosetta guest's
* teardown previously leaked.
*/
/* Give back any pty slaves this process still holds before the fd table
* goes away. The guest's stdio slaves are closed by the kernel, not by the
* guest, so they never pass through the per-fd close hook; a master in
* another process would otherwise wait forever for a hangup that this exit
* should have produced.
*/
proc_pty_release_process_slaves();

cleanup_main_resources(&g, guest_initialized, &sysroot_mount,
have_host_cwd ? host_cwd : NULL, guest_argv,
guest_argc, elf_path, sysroot_path);
Expand Down
52 changes: 52 additions & 0 deletions src/runtime/forkipc.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "runtime/forkipc.h"
#include "runtime/fork-state.h"
#include "runtime/futex.h"
#include "runtime/procemu.h"

#include "syscall/linux-wire.h"
#include "syscall/chown-overlay.h"
Expand Down Expand Up @@ -294,15 +295,34 @@ int fork_child_main(int ipc_fd,
return 1;
}

/* Both the fd table and the keepalives are in place, which is what this
* needs to recognize the slave fds inherited from the parent and put them
* back on the books. Without it the parent's close of its own copy makes
* the pty look hung up while this child still holds a live slave.
*/
proc_pty_adopt_inherited_slaves();
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

signal_state_t sig;
if (fork_ipc_recv_process_state(ipc_fd, &g, &sig) < 0) {
log_error("fork-child: failed to receive process state");
/* Give back the credit the parent added for this child before
* bailing: nothing here reaches the teardown that normally
* returns it, and a child that never runs must not leave the
* pty looking busy to the master the parent still holds.
*/
proc_pty_release_process_slaves();
guest_destroy(&g);
return 1;
}

if (chown_overlay_recv(ipc_fd) < 0) {
log_error("fork-child: failed to receive chown overlay");
/* Give back the credit the parent added for this child before
* bailing: nothing here reaches the teardown that normally
* returns it, and a child that never runs must not leave the
* pty looking busy to the master the parent still holds.
*/
proc_pty_release_process_slaves();
guest_destroy(&g);
return 1;
}
Expand All @@ -317,6 +337,12 @@ int fork_child_main(int ipc_fd,
0 ||
admission_ready != 1) {
log_error("fork-child: parent did not commit child admission");
/* Give back the credit the parent added for this child before
* bailing: nothing here reaches the teardown that normally
* returns it, and a child that never runs must not leave the
* pty looking busy to the master the parent still holds.
*/
proc_pty_release_process_slaves();
guest_destroy(&g);
return 1;
}
Expand Down Expand Up @@ -371,6 +397,12 @@ int fork_child_main(int ipc_fd,
* the single-threaded child at this point).
*/
if (shim_globals_install_per_vcpu(vcpu, &g, hdr.child_pid) < 0) {
/* Give back the credit the parent added for this child before
* bailing: nothing here reaches the teardown that normally
* returns it, and a child that never runs must not leave the
* pty looking busy to the master the parent still holds.
*/
proc_pty_release_process_slaves();
guest_destroy(&g);
return 1;
}
Expand Down Expand Up @@ -498,6 +530,14 @@ int fork_child_main(int ipc_fd,
vcpu_run_loop(vcpu, vexit, &g, verbose, timeout_sec, &wait_status);

proc_process_exit(wait_status);

/* Same reason as the main-process path: this child is where a forked shell
* runs, and its stdio slaves are closed by the kernel rather than by the
* guest, so they never reach the per-fd close hook. The parent still holds
* the master and is waiting for exactly this hangup.
*/
proc_pty_release_process_slaves();

guest_destroy(&g);
return exit_code;
}
Expand Down Expand Up @@ -1911,6 +1951,18 @@ int64_t sys_clone(hv_vcpu_t vcpu,
goto fail_snapshot;
}

/* The child's inherited slave copies are live from the moment fork returns,
* so they go on the shared count here rather than in the child's own init,
* which the parent's close of its copy can beat.
*
* Deliberately after the last failure exit: every goto above unwinds a
* child that will never run, and a credit added before them would never be
* given back -- the pty would then look permanently busy and its master
* 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();
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>


/* The process-state payload includes the SCM_RIGHTS handoff for region
* backing fds. Keep siblings quiesced until that send completes so a
* concurrent munmap/remap cannot close or recycle the captured fd numbers.
Expand Down
Loading
Loading