Replace `popen` with `spawn` by Martin-Zeithaml · Pull Request #211 · zowe/launcher

Martin-Zeithaml · 2026-07-08T08:56:09Z

Why the change

Shell injection / quoting bugs — env values had to be manually quoted (get_sharedenv wrapped each value in "…"). Any value containing quotes, $, spaces, etc. could corrupt the command or be misinterpreted by the shell.
Command-length limits — packing the whole environment plus two full paths into one argv string risks overflowing shell/exec length limits on large configurations.
Path issues — long or oddly-encoded root paths could silently break the constructed command.

Review pointers for maintainers

This is z/OS-specific (spawn.h spawn() with fd remapping) — verify it builds and runs on the target LE/USS environment.
Confirm shared_uss_env is always NULL-terminated and non-NULL by the time prepare_instance() runs
The envp debug dump at the top of run_command iterates envp — worth a sanity check that env logging in debug mode doesn't leak anything sensitive.

JoeNemo · 2026-07-15T15:29:25Z

When I had a "Fabulous" LLM review this code, it said:

1. waitpid has no EINTR handling, and may race the launcher's child-reaper. waitpid(pid, &status, 0) → if a signal lands (the launcher
  catches SIGTERM/SIGCHLD), it returns -1/EINTR and prepare fails spuriously. Worse, if a SIGCHLD handler does waitpid(-1,…) it could reap
  this child first → our waitpid(pid) gets ECHILD. Worth confirming prepare_instance() runs before the signal loop is armed, and wrapping in
  while (waitpid(...)==-1 && errno==EINTR).

  2. Debug env dump can leak secrets (his pointer #3). DEBUG("  %s\n", *p) prints every value, and shared_uss_env includes the whole system
  environ — so in debug mode a password/token in the environment lands in the log. Suggest keys-only, or gate behind a higher trace level.

  3. const-correctness: for (char **p = envp; …) where envp is const char ** drops const → will warn under strict flags. Should be const char
  **p. (Same story as the (const char **)shared_uss_env cast.)

  4. Pipe fds aren't FD_CLOEXEC. spawn's fd_map remaps 0/1/2, but the child inherits c_stdout[0]/[1] as extra descriptors. No deadlock in
  this read-then-waitpid flow (EOF still arrives on child exit), but it's a leak and a foot-gun if the child ever spawns a grandchild.

JoeNemo · 2026-08-04T14:53:47Z

Many things about this fix seem sensible, but why did popen() become spawn. What was bad about popen().

Martin-Zeithaml · 2026-08-05T06:03:34Z

@JoeNemo

Many things about this fix seem sensible, but why did popen() become spawn. What was bad about popen().

Before: popen + get_sharedenv
- Extra functionality to process environment variables and check for special characters
Now: only spawn

ifakhrutdinov · 2026-08-21T05:03:22Z

@Martin-Zeithaml, I'm looking at the code which seems to have evolved from a simpler PR to, well, a one with more moving parts.

Does this PR, especially, after 4d9b256, contain changes which could better fit in multiple PRs? I've still not understood the whole PR so I might be wrong, but it feels like several PRs.

Martin-Zeithaml · 2026-08-21T05:14:30Z

@Martin-Zeithaml, I'm looking at the code which seems to have evolved from a simpler PR to, well, a one with more moving parts.

Does this PR, especially, after 4d9b256, contain changes which could better fit in multiple PRs? I've still not understood the whole PR so I might be wrong, but it feels like several PRs.

4d9b256 is for this feedback. This PR is just one PR.

ifakhrutdinov · 2026-08-21T06:23:19Z

With permission from Martin, I've re-written the branch's history to have commit that match logical changes; the before and after changes match exactly.

ifakhrutdinov · 2026-08-21T06:24:20Z

-static int run_command(const char *command, handle_line_callback_t handle_line, void *data) {
-  DEBUG("about to run command '%s'\n", command);
-  FILE *fp = popen(command, "r");
+static int run_command(const char *bin, const char *argv[], const char *envp[], handle_line_callback_t handle_line, void *data) {


Nit: this should probably now be called run_bin or similar.

ifakhrutdinov · 2026-08-21T06:57:43Z

+  }
+
+  for (int i = 0; i < 2; i++) {
+    if (fcntl(c_stdout[i], F_SETFD, FD_CLOEXEC) < 0) {


What if a child process is created between the pipe call and fcntl(FD_CLOEXEC)? Isn't there still a window where we can leak descriptors?

Looks like we may need to use pipe2 with O_CLOEXEC. If so, let's not do that in this PR; I'd rather it is fixed separately since the existing code would also need to be changed.

ifakhrutdinov · 2026-08-21T07:36:35Z

-static int run_command(const char *command, handle_line_callback_t handle_line, void *data) {
-  DEBUG("about to run command '%s'\n", command);
-  FILE *fp = popen(command, "r");
+static int run_command(const char *bin, const char *argv[], const char *envp[], handle_line_callback_t handle_line, void *data) {


I think in the future we should consider handling hangs in the commands. Looks like now, if a child process hangs, and the launcher is terminated via SIGINT/SIGTERM, the child process will be left orphaned?

I mean, at this point we wouldn't even be able to terminate the launcher via the operator command because it'd happen before the console handler is set up.

ifakhrutdinov

It's good to go, but we'll need to improve on the following items in the future:

It'd be nice not to duplicate all the spawn logic between components and run_command. I know 2 instances is fine to duplicate, but in this case there is a lot of error prone code. We could at least abstract away some logic.
Some fixes could leave in a separate PR; and some fixes like CLOEXEC seem really irrelevant (all the spawn calls use fd_map, so leaks are kind of impossible) and also not 100% done.

Preserve existing open flags when setting O_NONBLOCK on the read end (F_GETFL before F_SETFL instead of overwriting), check the fcntl return codes, and set FD_CLOEXEC on both pipe fds so they are not inherited by unrelated children. Also close both pipe fds on every error path to avoid descriptor leaks. Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>

Run the instance-prepare step (configmgr JS) via spawn() with an explicit argv and the shared_uss_env envp array instead of building a single shell command string and running it through popen(). Because spawn() takes the environment as a real argv/envp pair, the shell is no longer involved, so the manual per-value quoting and the whole command-string assembly are gone: - remove get_sharedenv() (shell value quoting workaround) - remove get_start_prepare_cmd() (command-string builder) run_command() now creates its own pipe, spawns the child with stdout/stderr redirected, drains the output, and waitpid()s for the exit status (WIFEXITED/WEXITSTATUS), with EINTR retry. prepare_instance() builds the configmgr binary path and the cli.js path with snprintf and checks for truncation. Historical note: popen here was never a spawn replacement; it was added later to run helper shell scripts. Those are now JS invoked via configmgr, so the shell layer is no longer needed. Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>

When debug logging is on, print the keys (names only, never values) of the environment passed to spawn(). This aids troubleshooting without leaking secrets that may be present in the environment values. Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>

github-project-automation Bot added this to zOS Squad Board Jul 8, 2026

Martin-Zeithaml marked this pull request as ready for review July 15, 2026 08:41

Martin-Zeithaml marked this pull request as draft July 20, 2026 12:33

Martin-Zeithaml marked this pull request as ready for review July 21, 2026 12:23

Martin-Zeithaml requested a review from ifakhrutdinov August 4, 2026 14:52

ifakhrutdinov force-pushed the v3.x/bugfix/popen2spawn branch from 479a8c3 to f70f8b1 Compare August 21, 2026 06:22

ifakhrutdinov reviewed Aug 21, 2026

View reviewed changes

ifakhrutdinov approved these changes Aug 21, 2026

View reviewed changes

Martin-Zeithaml added 3 commits August 21, 2026 10:47

ifakhrutdinov force-pushed the v3.x/bugfix/popen2spawn branch from f70f8b1 to 63d067e Compare August 21, 2026 07:47

ifakhrutdinov merged commit a7aa7cd into v3.x/staging Aug 21, 2026
5 of 7 checks passed

github-project-automation Bot moved this to Done in zOS Squad Board Aug 21, 2026

ifakhrutdinov deleted the v3.x/bugfix/popen2spawn branch August 21, 2026 07:47

Navigation Menu

Uh oh!

Replace `popen` with `spawn` - #211

Replace `popen` with `spawn`#211
ifakhrutdinov merged 3 commits into
v3.x/stagingfrom
v3.x/bugfix/popen2spawn

Martin-Zeithaml commented Jul 8, 2026 •

edited

Loading

Uh oh!

JoeNemo commented Jul 15, 2026 •

edited by Martin-Zeithaml

Loading

Uh oh!

JoeNemo commented Aug 4, 2026

Uh oh!

Martin-Zeithaml commented Aug 5, 2026

Uh oh!

ifakhrutdinov commented Aug 21, 2026

Uh oh!

Martin-Zeithaml commented Aug 21, 2026

Uh oh!

ifakhrutdinov commented Aug 21, 2026

Uh oh!

ifakhrutdinov Aug 21, 2026

Uh oh!

ifakhrutdinov Aug 21, 2026

Uh oh!

ifakhrutdinov Aug 21, 2026

Uh oh!

ifakhrutdinov left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Martin-Zeithaml commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the change

Review pointers for maintainers

Uh oh!

JoeNemo commented Jul 15, 2026 • edited by Martin-Zeithaml Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeNemo commented Aug 4, 2026

Uh oh!

Martin-Zeithaml commented Aug 5, 2026

Uh oh!

ifakhrutdinov commented Aug 21, 2026

Uh oh!

Martin-Zeithaml commented Aug 21, 2026

Uh oh!

ifakhrutdinov commented Aug 21, 2026

Uh oh!

ifakhrutdinov Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ifakhrutdinov Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ifakhrutdinov Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

ifakhrutdinov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Martin-Zeithaml commented Jul 8, 2026 •

edited

Loading

JoeNemo commented Jul 15, 2026 •

edited by Martin-Zeithaml

Loading