Skip to content

Commit b5f2508

Browse files
SunrisepeakSunrisepeak
committed
fix: seal child-process stdin on Windows (first-run hang)
mcpp's first-run flow on Windows was hanging at xlings / xim / curl / git grandchildren that block on terminal stdin, forcing users to press Enter repeatedly to advance bootstrap and toolchain install. Root cause: process::seal_stdin was a no-op on Windows, and install_with_progress's direct-install path deliberately bypassed it. The POSIX side has had </dev/null sealing since PR #55 (macOS xcrun hang fix); Windows never received the equivalent fix. PR #57 only suppressed stdout/stderr noise (>/dev/null 2>&1) and did not touch stdin. Changes: - process.cppm: seal_stdin now appends "<NUL" on Windows (matches POSIX behavior). All capture / run_silent / run_streaming / run_passthrough callers gain the protection automatically. - xlings.cppm: install_with_progress's direct path explicitly appends "<NUL" on Windows. POSIX keeps the original behavior conservatively. - shell.cppm: silent_redirect docstring corrected — it never touched stdin, that's seal_stdin's job. Implementation unchanged. Regression coverage: - tests/unit/test_process_seal_stdin.cpp — deterministic reproduction test. Rebinds the test process's own stdin to an open, empty, never-closing pipe, then calls run_silent / capture / run_streaming with a child that reads stdin (more on Windows, cat on POSIX). Without the fix the child would block forever waiting on our pipe; with the fix it reads NUL / /dev/null and exits immediately. 5-second upper bound (real runs complete in <100ms). - ci-windows.yml — adds a step that launches mcpp via System.Diagnostics.Process with RedirectStandardInput=$true (parent holds the child's stdin open but never writes). Runs mcpp --version, mcpp build, mcpp run. Without the fix, any grandchild reading stdin blocks → step times out → CI fails. With the fix → all complete.
1 parent 328e46f commit b5f2508

5 files changed

Lines changed: 270 additions & 12 deletions

File tree

‎.github/workflows/ci-windows.yml‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,82 @@ jobs:
9090
export MCPP_VENDORED_XLINGS=$(cygpath -w "$USERPROFILE/.xlings/subos/default/bin/xlings.exe")
9191
"$MCPP_SELF" test
9292
93+
# Regression test for the Windows first-run "press Enter to advance" hang.
94+
# Launches mcpp with an OPEN, EMPTY, never-closing stdin pipe. Without
95+
# seal_stdin's Windows fix, any grandchild that reads stdin would inherit
96+
# our pipe and block forever — caught by the timeout below. With the fix,
97+
# every subprocess stdin is redirected from NUL → no possibility of hang.
98+
- name: "Regression: mcpp survives open-empty-stdin (Windows hang fix)"
99+
shell: pwsh
100+
timeout-minutes: 15
101+
env:
102+
MCPP_VENDORED_XLINGS: ${{ env.XLINGS_BIN }}
103+
run: |
104+
$ErrorActionPreference = 'Stop'
105+
106+
$tmp = Join-Path $env:RUNNER_TEMP ("stdin-hang-test-" + [guid]::NewGuid().ToString('N'))
107+
New-Item -ItemType Directory -Path $tmp | Out-Null
108+
Set-Location $tmp
109+
& $env:MCPP_SELF new hello_stdin
110+
Set-Location hello_stdin
111+
112+
function Invoke-McppWithOpenStdin {
113+
param([string]$Args, [int]$TimeoutSeconds = 300)
114+
115+
$psi = [System.Diagnostics.ProcessStartInfo]::new()
116+
$psi.FileName = $env:MCPP_SELF
117+
$psi.Arguments = $Args
118+
$psi.WorkingDirectory = (Get-Location).Path
119+
$psi.UseShellExecute = $false
120+
$psi.RedirectStandardInput = $true # parent holds child's stdin
121+
$psi.RedirectStandardOutput = $true
122+
$psi.RedirectStandardError = $true
123+
# By default the child inherits the parent's env (we did not
124+
# touch $psi.Environment) so MCPP_VENDORED_XLINGS / PATH / etc.
125+
# propagate.
126+
127+
$p = [System.Diagnostics.Process]::Start($psi)
128+
129+
# Async-drain stdout/stderr so a full output buffer doesn't
130+
# itself deadlock the child (separate failure mode from the
131+
# stdin hang we're testing).
132+
$stdoutTask = $p.StandardOutput.ReadToEndAsync()
133+
$stderrTask = $p.StandardError.ReadToEndAsync()
134+
135+
# NEVER write or close $p.StandardInput — the pipe stays open
136+
# and empty for the lifetime of the child. Any grandchild that
137+
# reads stdin will block on this pipe → caught by WaitForExit.
138+
139+
if (-not $p.WaitForExit($TimeoutSeconds * 1000)) {
140+
try { $p.Kill($true) } catch {}
141+
Write-Host "----- captured stdout -----"
142+
Write-Host $stdoutTask.Result
143+
Write-Host "----- captured stderr -----"
144+
Write-Host $stderrTask.Result
145+
throw "REGRESSION: 'mcpp $Args' HUNG with open-empty stdin after ${TimeoutSeconds}s. The Windows seal_stdin fix is not effective."
146+
}
147+
148+
Write-Host "----- stdout -----"
149+
Write-Host $stdoutTask.Result
150+
Write-Host "----- stderr -----"
151+
Write-Host $stderrTask.Result
152+
153+
if ($p.ExitCode -ne 0) {
154+
throw "'mcpp $Args' exited with code $($p.ExitCode) (no hang, but failed)."
155+
}
156+
}
157+
158+
Write-Host '=== T1: mcpp --version (sanity, fast path) ==='
159+
Invoke-McppWithOpenStdin -Args '--version' -TimeoutSeconds 30
160+
161+
Write-Host '=== T2: mcpp build (full bootstrap + toolchain + dep resolve + compile) ==='
162+
Invoke-McppWithOpenStdin -Args 'build' -TimeoutSeconds 600
163+
164+
Write-Host '=== T3: mcpp run (post-build run path) ==='
165+
Invoke-McppWithOpenStdin -Args 'run' -TimeoutSeconds 120
166+
167+
Write-Host 'SUCCESS: mcpp completes with open-empty stdin → Windows seal_stdin fix verified.'
168+
93169
- name: E2E suite
94170
shell: bash
95171
run: |

‎src/platform/process.cppm‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
// mcpp.platform.process — platform-aware process runner.
22
//
33
// Centralises all popen/system usage so callers do not scatter #if _WIN32
4-
// guards or duplicate the popen-read loop. On POSIX, all functions
5-
// automatically redirect stdin from /dev/null to prevent interactive
6-
// prompts from child processes (fixes macOS first-run hangs where xcrun
7-
// or xcode-select would block waiting for user input).
4+
// guards or duplicate the popen-read loop. All functions automatically
5+
// seal stdin (redirect from /dev/null on POSIX, from NUL on Windows) to
6+
// prevent interactive prompts from child processes:
7+
// - POSIX: fixes macOS first-run hangs where xcrun / xcode-select would
8+
// block waiting for user input.
9+
// - Windows: fixes first-run hangs where xlings / xim / curl / git child
10+
// processes would block on terminal stdin, forcing the user to press
11+
// Enter repeatedly to advance bootstrap / toolchain install.
812
//
913
// Entry points:
1014
// capture — run a command, capture stdout
@@ -73,12 +77,16 @@ namespace mcpp::platform::process {
7377

7478
namespace {
7579

76-
// On POSIX, append "< /dev/null" to prevent child processes from reading
77-
// stdin. This fixes macOS first-run hangs where tools like xcrun or
78-
// xcode-select block waiting for user input.
80+
// Append a non-interactive stdin redirect to prevent child processes from
81+
// blocking on terminal input.
82+
// - POSIX: "< /dev/null" — fixes macOS xcrun / xcode-select hangs.
83+
// - Windows: "<NUL" — fixes xlings / xim / curl / git hangs on
84+
// first-run toolchain install (user otherwise
85+
// had to press Enter repeatedly to advance).
86+
// `cmd.exe` accepts `<NUL` as a redirect for an immediately-EOF stdin.
7987
std::string seal_stdin(std::string_view cmd) {
8088
#if defined(_WIN32)
81-
return std::string(cmd);
89+
return std::string(cmd) + " <NUL";
8290
#else
8391
return std::string(cmd) + " </dev/null";
8492
#endif

‎src/platform/shell.cppm‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ export namespace mcpp::platform::shell {
2020
// Platform-aware shell argument quoting.
2121
std::string quote(std::string_view s);
2222

23-
// Full silent redirect (stdin + stdout + stderr → /dev/null).
23+
// Silent redirect — stdout + stderr → /dev/null (or NUL on Windows).
24+
// stdin is NOT touched here; that's the responsibility of
25+
// mcpp::platform::process::seal_stdin, which is auto-applied by capture /
26+
// run_silent / run_streaming on all platforms.
2427
#if defined(_WIN32)
2528
constexpr std::string_view silent_redirect = ">nul 2>&1";
2629
#else

‎src/xlings.cppm‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -681,9 +681,16 @@ int install_with_progress(const Env& env, std::string_view target,
681681
{
682682
auto directCmd = build_command_prefix(env) +
683683
std::format(" install {} -y {}", target, mcpp::platform::shell::silent_redirect);
684-
// Use std::system() directly — do NOT redirect stdin via </dev/null
685-
// because xlings may need stdin for subprocess coordination during
686-
// large package extraction.
684+
// Windows: explicitly seal stdin (<NUL) so xlings and any grandchildren
685+
// (curl, git, 7z, etc.) cannot block on a terminal read. The earlier
686+
// comment claimed POSIX must keep stdin open for "subprocess
687+
// coordination" — that's never been observed in practice on Linux/macOS,
688+
// and on Windows it caused users to press Enter repeatedly during first-
689+
// run toolchain install. POSIX keeps the original behavior to stay
690+
// conservative.
691+
if constexpr (mcpp::platform::is_windows) {
692+
directCmd += " <NUL";
693+
}
687694
int directRc = mcpp::platform::process::extract_exit_code(
688695
std::system(directCmd.c_str()));
689696
if (directRc == 0) return 0;

‎tests/unit/test_process_seal_stdin.cpp‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
// Regression test for the Windows first-run hang where xlings / xim / curl /
2+
// git child processes blocked on terminal stdin, forcing the user to press
3+
// Enter repeatedly to advance bootstrap / toolchain install.
4+
//
5+
// `mcpp::platform::process::{capture, run_silent, run_streaming}` MUST seal
6+
// stdin so any child reading stdin gets immediate EOF, not a blocking read.
7+
// POSIX: appends "</dev/null"
8+
// Windows: appends "<NUL"
9+
//
10+
// Test strategy: rebind this test process's own stdin to an open, empty,
11+
// never-closing pipe. Then invoke run_silent / capture with a child that
12+
// reads stdin. Without seal_stdin, the child would inherit our pipe stdin
13+
// and block forever; the gtest runner would then hang until CI timeout.
14+
// With the fix, the child reads from NUL / /dev/null and exits immediately.
15+
16+
#include <gtest/gtest.h>
17+
#include <chrono>
18+
19+
#if defined(_WIN32)
20+
#include <windows.h>
21+
#include <io.h>
22+
#include <fcntl.h>
23+
#else
24+
#include <fcntl.h>
25+
#include <unistd.h>
26+
#endif
27+
28+
import std;
29+
import mcpp.platform.process;
30+
31+
namespace {
32+
33+
// Maximum seconds a sealed-stdin command may take before we declare it
34+
// "hung". Real child runs (cat / more reading from EOF stdin) complete in
35+
// well under 100ms on any modern machine, so 5s is a very generous bound.
36+
constexpr int kMaxSealedSeconds = 5;
37+
38+
// RAII: rebind STDIN to an open, empty, never-closing pipe for the duration
39+
// of one test. Restores the original stdin on destruction.
40+
class OpenEmptyStdinScope {
41+
public:
42+
OpenEmptyStdinScope() {
43+
#if defined(_WIN32)
44+
SECURITY_ATTRIBUTES sa{};
45+
sa.nLength = sizeof(sa);
46+
sa.bInheritHandle = TRUE;
47+
if (!CreatePipe(&hRead_, &hWrite_, &sa, 0)) {
48+
std::abort();
49+
}
50+
// Make the read end inheritable (already is via sa, but be explicit).
51+
SetHandleInformation(hRead_, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT);
52+
53+
// Save the original stdin (both Win32 handle + CRT fd) so we can
54+
// restore in the destructor.
55+
origStdinHandle_ = GetStdHandle(STD_INPUT_HANDLE);
56+
origStdinFd_ = _dup(0);
57+
58+
// Bind the pipe-read-end as our process's stdin at both layers.
59+
SetStdHandle(STD_INPUT_HANDLE, hRead_);
60+
int newFd = _open_osfhandle(reinterpret_cast<intptr_t>(hRead_),
61+
_O_RDONLY | _O_BINARY);
62+
if (newFd >= 0) {
63+
_dup2(newFd, 0);
64+
_close(newFd); // _dup2 keeps a reference; we're done with newFd
65+
}
66+
#else
67+
if (::pipe(pipeFds_) != 0) std::abort();
68+
origStdinFd_ = ::dup(0);
69+
::dup2(pipeFds_[0], 0);
70+
#endif
71+
}
72+
73+
~OpenEmptyStdinScope() {
74+
#if defined(_WIN32)
75+
// Restore original stdin.
76+
if (origStdinFd_ >= 0) {
77+
_dup2(origStdinFd_, 0);
78+
_close(origStdinFd_);
79+
}
80+
SetStdHandle(STD_INPUT_HANDLE, origStdinHandle_);
81+
if (hWrite_) CloseHandle(hWrite_);
82+
if (hRead_) CloseHandle(hRead_);
83+
#else
84+
if (origStdinFd_ >= 0) {
85+
::dup2(origStdinFd_, 0);
86+
::close(origStdinFd_);
87+
}
88+
::close(pipeFds_[0]);
89+
::close(pipeFds_[1]);
90+
#endif
91+
}
92+
93+
OpenEmptyStdinScope(const OpenEmptyStdinScope&) = delete;
94+
OpenEmptyStdinScope& operator=(const OpenEmptyStdinScope&) = delete;
95+
96+
private:
97+
#if defined(_WIN32)
98+
HANDLE hRead_ = nullptr;
99+
HANDLE hWrite_ = nullptr; // intentionally never written → reader blocks
100+
HANDLE origStdinHandle_ = nullptr;
101+
int origStdinFd_ = -1;
102+
#else
103+
int pipeFds_[2] = {-1, -1}; // intentionally never written → reader blocks
104+
int origStdinFd_ = -1;
105+
#endif
106+
};
107+
108+
// A child command that reads stdin to EOF and exits.
109+
// With seal_stdin in effect → stdin is NUL / /dev/null → child exits immediately.
110+
// Without seal_stdin AND with an open-empty parent stdin → child blocks forever.
111+
constexpr std::string_view kStdinReaderCmd =
112+
#if defined(_WIN32)
113+
"more >nul 2>&1"
114+
#else
115+
"cat >/dev/null"
116+
#endif
117+
;
118+
119+
template <class F>
120+
double time_seconds(F&& fn) {
121+
auto t0 = std::chrono::steady_clock::now();
122+
fn();
123+
auto t1 = std::chrono::steady_clock::now();
124+
return std::chrono::duration<double>(t1 - t0).count();
125+
}
126+
127+
} // namespace
128+
129+
// run_silent: must seal stdin so the child does not inherit our pipe stdin
130+
// and block forever.
131+
TEST(ProcessSealStdin, RunSilentDoesNotHangWhenParentStdinIsOpenPipe) {
132+
OpenEmptyStdinScope scope;
133+
double elapsed = time_seconds([] {
134+
(void)mcpp::platform::process::run_silent(kStdinReaderCmd);
135+
});
136+
EXPECT_LT(elapsed, static_cast<double>(kMaxSealedSeconds))
137+
<< "run_silent took " << elapsed
138+
<< "s — child blocked on stdin → seal_stdin is broken or not applied";
139+
}
140+
141+
// capture: must also seal stdin (it shares seal_stdin with run_silent).
142+
TEST(ProcessSealStdin, CaptureDoesNotHangWhenParentStdinIsOpenPipe) {
143+
OpenEmptyStdinScope scope;
144+
double elapsed = time_seconds([] {
145+
(void)mcpp::platform::process::capture(kStdinReaderCmd);
146+
});
147+
EXPECT_LT(elapsed, static_cast<double>(kMaxSealedSeconds))
148+
<< "capture took " << elapsed
149+
<< "s — child blocked on stdin → seal_stdin is broken or not applied";
150+
}
151+
152+
// run_streaming: same property — children spawned via popen-streaming must
153+
// not inherit a live stdin.
154+
TEST(ProcessSealStdin, RunStreamingDoesNotHangWhenParentStdinIsOpenPipe) {
155+
OpenEmptyStdinScope scope;
156+
double elapsed = time_seconds([] {
157+
(void)mcpp::platform::process::run_streaming(
158+
kStdinReaderCmd,
159+
[](std::string_view) {});
160+
});
161+
EXPECT_LT(elapsed, static_cast<double>(kMaxSealedSeconds))
162+
<< "run_streaming took " << elapsed
163+
<< "s — child blocked on stdin → seal_stdin is broken or not applied";
164+
}

0 commit comments

Comments
 (0)