Skip to content

Commit 115800a

Browse files
committed
fix(process): shell env-prefix launcher (no spawn) — fixes Windows/macOS
Both spawn-based attempts regressed non-Linux: posix_spawn returned an error on the macOS runner (run_exec rc!=0), and _spawnvpe segfaulted mcpp run on Windows (0xC0000005). The leak is Linux-only (macOS injects no runtime env; Windows has no glibc symbol versioning), so changing the launch primitive on every platform was the wrong call. Reimplement run_exec/capture_exec on top of the existing shell helpers: - run_exec: std::system with the env as a prefix via build_env_prefix (POSIX) / _putenv_s (Windows). The shell std::system spawns starts clean, so a bundled-glibc LD_LIBRARY_PATH reaches only the target child, never /bin/sh — which fixes the original crash. - capture_exec: command_from_argv + ' 2>&1' through the existing capture_with_env (same prefix semantics). No spawn primitives, no per-platform exec code. Linux: unit 19/19, e2e 74, hostile-LD_LIBRARY_PATH run, and fast-path rebuild all green.
1 parent d130f0e commit 115800a

2 files changed

Lines changed: 31 additions & 106 deletions

File tree

src/platform/process.cppm

Lines changed: 28 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,9 @@ module;
2222
#include <cstdio>
2323
#include <cstdlib>
2424
#if defined(_WIN32)
25-
#include <stdlib.h> // _putenv_s, _spawnvpe, _environ
26-
#include <process.h> // _spawnvpe, _P_WAIT
25+
#include <stdlib.h> // _putenv_s
2726
#define popen _popen
2827
#define pclose _pclose
29-
#else
30-
#include <unistd.h> // pipe, dup2, close, read, environ
31-
#include <sys/wait.h> // waitpid
32-
#include <spawn.h> // posix_spawnp, posix_spawn_file_actions_*
33-
extern "C" char **environ;
3428
#endif
3529

3630
export module mcpp.platform.process;
@@ -131,26 +125,23 @@ int normalize_exit_code(int rc) {
131125
#endif
132126
}
133127

134-
#if !defined(_WIN32)
135-
// Build a child environment block = the current environ with `extra` overrides
136-
// applied. Returned vector owns the strings; the caller derives a NUL-terminated
137-
// char* array from it. Built in the PARENT so the child env never requires a
138-
// post-fork setenv (async-signal-unsafe on macOS) and the parent is untouched.
139-
std::vector<std::string> merged_environ(
140-
const std::vector<std::pair<std::string, std::string>>& extra)
141-
{
142-
std::vector<std::string> out;
143-
std::set<std::string> overridden;
144-
for (auto& [k, v] : extra) { out.push_back(k + "=" + v); overridden.insert(k); }
145-
for (char** e = environ; e && *e; ++e) {
146-
std::string_view entry(*e);
147-
auto eq = entry.find('=');
148-
std::string key(eq == std::string_view::npos ? entry : entry.substr(0, eq));
149-
if (!overridden.contains(key)) out.emplace_back(entry);
128+
// Build a shell command line from an argv vector. The first token (program)
129+
// is kept RAW on Windows — quoting it would make cmd.exe's `/c "..."` strip the
130+
// outer quotes and mangle the path (see platform.shell) — and shell-quoted on
131+
// POSIX. Remaining args are always shell-quoted.
132+
std::string command_from_argv(const std::vector<std::string>& argv) {
133+
if (argv.empty()) return "";
134+
#if defined(_WIN32)
135+
std::string cmd = argv[0];
136+
#else
137+
std::string cmd = mcpp::platform::shell::quote(argv[0]);
138+
#endif
139+
for (std::size_t i = 1; i < argv.size(); ++i) {
140+
cmd += ' ';
141+
cmd += mcpp::platform::shell::quote(argv[i]);
150142
}
151-
return out;
143+
return cmd;
152144
}
153-
#endif
154145

155146
} // namespace
156147

@@ -264,39 +255,14 @@ int run_exec(const std::vector<std::string>& argv,
264255
const std::vector<std::pair<std::string, std::string>>& extraEnv)
265256
{
266257
if (argv.empty()) return 127;
267-
#if defined(_WIN32)
268-
// Build a child env block (parent env + overrides); no cmd.exe involved.
269-
std::vector<std::string> envStrings;
270-
for (char** e = _environ; e && *e; ++e) envStrings.emplace_back(*e);
271-
for (auto& [k, v] : extraEnv) envStrings.push_back(k + "=" + v);
272-
std::vector<const char*> envp;
273-
for (auto& s : envStrings) envp.push_back(s.c_str());
274-
envp.push_back(nullptr);
275-
276-
std::vector<const char*> cargv;
277-
for (auto& a : argv) cargv.push_back(a.c_str());
278-
cargv.push_back(nullptr);
279-
280-
intptr_t rc = _spawnvpe(_P_WAIT, cargv[0], cargv.data(), envp.data());
281-
return rc < 0 ? 127 : static_cast<int>(rc);
282-
#else
283-
// posix_spawnp: direct exec (PATH-searched), child env built in the parent.
284-
// No post-fork setenv (async-signal-unsafe on macOS); parent env untouched.
285-
auto envStore = merged_environ(extraEnv);
286-
std::vector<char*> envp;
287-
for (auto& s : envStore) envp.push_back(s.data());
288-
envp.push_back(nullptr);
289-
std::vector<char*> cargv;
290-
for (auto& a : argv) cargv.push_back(const_cast<char*>(a.c_str()));
291-
cargv.push_back(nullptr);
292-
293-
pid_t pid = 0;
294-
if (::posix_spawnp(&pid, cargv[0], nullptr, nullptr, cargv.data(), envp.data()) != 0)
295-
return 127; // exec/spawn failed (e.g. program not found)
296-
int status = 0;
297-
while (::waitpid(pid, &status, 0) < 0) { /* EINTR retry */ }
298-
return normalize_exit_code(status);
299-
#endif
258+
// Shell launch via std::system, with the runtime env applied as a COMMAND
259+
// PREFIX on POSIX (`KEY='val' cmd`). The shell std::system spawns starts
260+
// with a clean environment — a bundled-glibc LD_LIBRARY_PATH is set only
261+
// for the target child, never for /bin/sh itself (the newer-glibc `sh:`
262+
// crash class). On Windows build_env_prefix does _putenv_s and returns "".
263+
std::string prefix = mcpp::platform::env::build_env_prefix(extraEnv);
264+
std::string cmd = prefix + command_from_argv(argv);
265+
return normalize_exit_code(std::system(cmd.c_str()));
300266
}
301267

302268
RunResult capture_exec(
@@ -305,53 +271,11 @@ RunResult capture_exec(
305271
{
306272
RunResult result;
307273
if (argv.empty()) { result.exit_code = 127; return result; }
308-
#if defined(_WIN32)
309-
// Windows is unaffected by the glibc leak; reuse the shell capture path.
310-
// Keep argv[0] RAW (never quote the first token under cmd.exe `/c "..."`,
311-
// or it mangles the path — see platform.shell); quote later args only.
312-
std::string cmd = argv[0];
313-
for (std::size_t i = 1; i < argv.size(); ++i) {
314-
cmd += ' ';
315-
cmd += mcpp::platform::shell::quote(argv[i]);
316-
}
274+
// Capture stdout+stderr combined (the trailing `2>&1` replaces the old
275+
// redirect). capture_with_env applies the env as a POSIX prefix / Windows
276+
// _putenv_s, so the shell is never pre-poisoned with the loader path.
277+
std::string cmd = command_from_argv(argv) + " 2>&1";
317278
return capture_with_env(cmd, extraEnv);
318-
#else
319-
// posix_spawnp + a pipe: capture stdout+stderr (replaces `2>&1`), child env
320-
// built in the parent. No post-fork setenv; parent env untouched.
321-
int fds[2];
322-
if (::pipe(fds) != 0) { result.exit_code = 127; return result; }
323-
324-
auto envStore = merged_environ(extraEnv);
325-
std::vector<char*> envp;
326-
for (auto& s : envStore) envp.push_back(s.data());
327-
envp.push_back(nullptr);
328-
std::vector<char*> cargv;
329-
for (auto& a : argv) cargv.push_back(const_cast<char*>(a.c_str()));
330-
cargv.push_back(nullptr);
331-
332-
posix_spawn_file_actions_t fa;
333-
::posix_spawn_file_actions_init(&fa);
334-
::posix_spawn_file_actions_adddup2(&fa, fds[1], 1); // stdout → pipe
335-
::posix_spawn_file_actions_adddup2(&fa, fds[1], 2); // stderr → same pipe
336-
::posix_spawn_file_actions_addclose(&fa, fds[0]);
337-
::posix_spawn_file_actions_addclose(&fa, fds[1]);
338-
339-
pid_t pid = 0;
340-
int sp = ::posix_spawnp(&pid, cargv[0], &fa, nullptr, cargv.data(), envp.data());
341-
::posix_spawn_file_actions_destroy(&fa);
342-
::close(fds[1]);
343-
if (sp != 0) { ::close(fds[0]); result.exit_code = 127; return result; }
344-
345-
std::array<char, 4096> buf{};
346-
ssize_t n;
347-
while ((n = ::read(fds[0], buf.data(), buf.size())) > 0)
348-
result.output.append(buf.data(), static_cast<size_t>(n));
349-
::close(fds[0]);
350-
int status = 0;
351-
while (::waitpid(pid, &status, 0) < 0) { /* EINTR retry */ }
352-
result.exit_code = normalize_exit_code(status);
353-
return result;
354-
#endif
355279
}
356280

357281
} // namespace mcpp::platform::process

tests/unit/test_process_run_exec.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ import mcpp.platform.process;
66

77
using namespace mcpp::platform;
88

9-
// These exercise the POSIX direct-exec path with POSIX program paths
10-
// (/bin/true, /bin/sh, /bin/echo). The Windows path (_spawnvpe) is covered by
9+
// These exercise the POSIX launch path with POSIX program paths (/bin/true,
10+
// /bin/sh, /bin/echo) — the env is applied as a `KEY='val' cmd` shell prefix,
11+
// so the parent environment is never mutated. The Windows path is covered by
1112
// the integration build (ninja launched via capture_exec).
1213
#if !defined(_WIN32)
1314

0 commit comments

Comments
 (0)