Fix ptrace stop misclassification and ownership-gated draining in CheckChildren

[adamsitnik]

Summary

This builds on the macOS deadlock fix from #131944 / #133930 (cherry-picked as the first commit of this PR) and addresses a reviewer concern raised on #133930 (https://github.com/dotnet/runtime/pull/133930/changes#r4013927001): a non-exit wait notification observed for a pid could belong to something else entirely (a foreign, non-Process-owned pid, or an external tracer such as ClrMD attached to a pid this runtime also happens to be tracking), so it must never be blindly consumed.

It also fixes #133736, where a waitpid(pid, WNOHANG) (no WUNTRACED) call on a pid that this process is ptrace-tracing observes the tracee's stop/continue transitions (always visible to a tracer), which was misclassified as a normal exit with code 0.

Changes

• SystemNative_WaitIdAnyExitedNoHangNoWait now only peeks (WNOWAIT) and classifies the pending notification via si_code (CLD_EXITED/CLD_KILLED/CLD_DUMPED vs. others); it never consumes/drains anything.
• CheckChildren's peek loop only reaps a pid directly when the notification is an actual exit (isExited) and the pid is a known/owned child. In every other case — a non-exit notification, or a pid we don't recognize — it leaves the notification completely untouched and falls back to checkAll, which performs a full scan of all known children using each child's own targeted, per-pid waitpid() (TryReapChild). This makes progress without spinning, without ever consuming or otherwise interfering with a notification that might not belong to us (e.g. a CLD_TRAPPED stop that an external tracer like ClrMD needs to observe), and without relying on cache membership as a proxy for pid ownership.
• Fixes SystemNative_WaitPidExitedNoHang to not misreport a WIFSTOPPED/WIFCONTINUED status as a normal exit with code 0.
• Adds a regression test for Linux: Process reports its ptrace-stopped child as exited when the parent is the tracer #133736. The test spawns short-lived "trigger" children while the target is ptrace-stopped (SIGCHLD is registered with SA_NOCLDSTOP, so the stop by itself does not trigger a check; unrelated child activity is needed to exercise the bug). Verified this test fails without the fix (reproducing the reported HasExited == true misclassification) and passes with it.
• Strengthens the existing Kill_EntireProcessTree_Concurrent_DoesNotHang test to also track and assert on grandchild processes exiting, not just the roots, so a regression that kills only the roots (but not the whole tree) would be caught.

An earlier iteration of this PR introduced a separate SystemNative_WaitIdDrainNonExited(pid) entrypoint that would consume a non-exit notification once the managed layer confirmed (via s_childProcessWaitStates) that it owned the pid's reaping responsibility. That approach was replaced by the checkAll fallback described above, because pid membership in s_childProcessWaitStates doesn't reliably prove current ownership after pid reuse, and because draining could still steal a notification (e.g. a ptrace stop) that an external tracer attached to the same pid needed to see.

Testing

• System.Diagnostics.Process.Tests (including outerloop): 689 total, 0 failed, 8 skipped.
• New regression test manually verified to fail against the pre-fix code and pass against the fixed code.

Fixes #133736

Note

This PR description and the accompanying implementation were prepared with GitHub Copilot CLI assistance.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

[Copilot]

🟡 Changes recommended

Critical ptrace ownership and WASI build issues, along with additional child-scanning and test reliability concerns, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes Unix child-wait classification for ptraced processes and adds ownership-gated notification draining.

Changes:

• Classifies waitid notifications and adds targeted draining.
• Updates managed child checking and waitpid handling.
• Adds regression tests and interop updates.

File summaries

File	Summary
src/native/libs/System.Native/pal_process.h	Updates wait APIs; WASI declarations need matching implementations.
src/native/libs/System.Native/pal_process.c	Implements notification classification and draining.
src/native/libs/System.Native/entrypoints.c	Exports the drain function; WASI binding remains unresolved.
src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs	Adds regression coverage; test readiness, cleanup, and runtime cost need improvement.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs	Applies ownership-aware handling; ptrace ownership, PID reuse, and pending-exit scanning issues remain.
src/libraries/Common/src/Interop/Unix/System.Native/Interop.WaitPid.cs	Updates wait behavior interop documentation.
src/libraries/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs	Adds waitid interop declarations.

Review details

Suppressed comments (2)

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1201

• A fixed 500 ms delay does not establish that all /bin/sleep grandchildren have started before the roots are stopped. On a slow runner, tree enumeration can miss a grandchild, so this test may pass without exercising recursive termination and leave an untracked sleep process running; use a readiness handshake or otherwise record each child before launching the kill tasks.

                // Give the grandchildren time to start so the trees are fully formed.
                Thread.Sleep(500);

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1181

• This is a normal test case but runs 30 iterations of 8 two-process trees, creating roughly 480 child processes and adding a 500 ms delay per iteration before the actual assertions. That is a substantial cost for every standard Process test run; the neighboring process-heavy/long-running tests use [OuterLoop]. Mark this test as outer-loop or reduce the workload so the regression coverage does not slow the regular suite.

            const int TreeCount = 8;
            const int Iterations = 30;

• Files reviewed: 7/7 changed files
• Comments generated: 6
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in ownership-gated draining and all-child reaping.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs:639

• The PR summary says this path calls a new SystemNative_WaitIdDrainNonExited(pid) after confirming ownership, but no such entry point is declared or implemented in this change. The actual fallback scans every tracked child and calls TryReapChild, so the implementation and the stated targeted-drain ownership design no longer match. Please update the description or narrow the implementation to the intended targeted operation so reviewers can verify the native-wait contract.

                            // Either this pid is not one we're responsible for reaping, or (on some
                            // platforms, e.g. macOS, or for a ptrace-traced child on Linux) the
                            // notification isn't actually an exit even though only exit notifications
                            // (WEXITED) were requested. In both cases we must not consume/act on this
                            // specific notification: it may belong to something else in this process

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1180

• This test creates 8 roots and 8 grandchildren for 30 iterations (480 process launches), but it is not marked [OuterLoop]. That makes the normal Process test pass pay a substantial process-creation/teardown cost and increases CI/process-limit pressure; the existing expensive process-launch tests in this file are isolated similarly. Mark this regression test as outer-loop, or reduce the workload after preserving the race coverage.

        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
        [PlatformSpecific(TestPlatforms.OSX)]
        public void Kill_EntireProcessTree_Concurrent_DoesNotHang()
        {
            const int TreeCount = 8;

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1261

• This regression test never acts as a native waiter for the ptrace stop; it only detaches after the managed reaper has had a chance to run. It can therefore pass even when CheckChildren consumes the stop notification, so it does not verify the ownership/interference contract described by the PR. Add an isolated native waitpid(..., WUNTRACED) assertion or an equivalent unmanaged-tracer scenario.

                    // SIGCHLD is installed with SA_NOCLDSTOP, so the ptrace stop above does not by itself
                    // wake up CheckChildren. Spawn unrelated short-lived children concurrently: each real
                    // exit delivers a SIGCHLD that runs CheckChildren, which (via waitid(P_ALL, ...)) will
                    // also observe -- and, without the fix, misclassify -- the stopped tracee.

src/native/libs/System.Native/pal_process.c:1258

• When reapAll is true, CheckChildren calls WaitPidExitedNoHang(-1) and treats a zero return as “no more children” (the caller breaks on pid <= 0). If waitpid(-1, WNOHANG) selects a ptrace stop/continue before another child’s exit, this branch consumes the non-exit status and converts the result to 0, so the loop stops without examining the pending exit. Since SIGCHLD delivery can be coalesced, no later callback is guaranteed and a tracked child can remain unmarked as exited. Keep the all-child path able to distinguish a consumed non-exit from no waitable child and continue or scan accordingly.

        else if (WIFSTOPPED(status) || WIFCONTINUED(status))
        {
            // The child has not exited -- it was merely stopped or continued. This can happen even
            // without WUNTRACED/WCONTINUED being requested when this process is the child's ptrace
            // tracer (e.g. via PTRACE_ATTACH): stop/continue transitions of a tracee are always
            // visible to its tracer's wait calls. Do not misreport this as an exit.
            result = 0;
        }

src/native/libs/System.Native/pal_process.c:1208

• Unlike the PR summary, this diff contains no SystemNative_WaitIdDrainNonExited declaration, export, implementation, or managed call; non-exit notifications are only classified and left pending. That is a substantive design mismatch, because the macOS fix from #133930 relied on draining to expose exits hidden behind a stopped child. Either add the ownership-gated drain (with matching platform stubs/exports) or update the PR contract and demonstrate that the fallback is sufficient.

        // classes. This function only peeks (WNOWAIT) and classifies the notification -- it never
        // consumes it, so callers can safely fall back to checking their own known children
        // directly without risking interference with an unrelated waiter for this pid.

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved issues remain around notification preservation, test cleanup, and 32-bit ptrace ABI coverage.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1290

• The ptrace request parameter is a 4-byte enum/int in the Unix ABI, but this declaration uses an 8-byte managed long. On 32-bit Unix, that shifts the variadic pid argument, so PTRACE_ATTACH/PTRACE_DETACH receive the wrong pid and this regression test cannot run on supported linux-arm/linux-x86 targets.

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs:678

• Skipping only the one PID returned by the waitid peek does not protect other pending non-exit notifications. If two tracked children are ptrace-stopped, the fallback reaches the second child here; TryReapChild calls waitpid, and the new native loop consumes that child's WIFSTOPPED/WIFCONTINUED status, so an external tracer can miss it. Track all candidates known to have non-exit notifications or classify each PID without a consuming wait before reaping.

                        if (kv.Key == pidToSkip)
                        {
                            continue;
                        }

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1225

• If any assertion in this loop fails (including the 30-second grandchild wait), execution skips these Dispose calls. ProcessTestBase only tracks the roots, and its cleanup kills roots without entireProcessTree, so the /bin/sleep 1000 grandchildren can be orphaned and left running after a failed test. Put grandchild cleanup in a finally (or otherwise register a cleanup that also terminates them).

                for (int i = 0; i < TreeCount; i++)
                {
                    Assert.True(roots[i].WaitForExit(WaitInMS));
                    Assert.True(grandChildren[i].WaitForExit(WaitInMS), $"Grandchild {grandChildren[i].Id} was not killed on iteration {iteration}.");
                    grandChildren[i].Dispose();

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:1271

• These assertions only prove that managed state remains false; an incorrect fallback could still consume the child's CLD_TRAPPED status via waitpid, then return 0 and pass both assertions. Add coverage that the stop notification remains observable to the tracer (for example, a non-consuming waitid check or a second traced child whose wait status is verified), since preserving that notification is a stated part of this change.

                    Assert.False(child.HasExited);
                    Assert.False(child.WaitForExit(0));

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

Stale. Missed copilot feedback :(