Skip to content

Call chdir to change working directory in older versions of glibc #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Call chdir to change working directory in older versions of glibc #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13db301
Call chdir to change working directory in older versions of glibc
yim-lee Dec 1, 2023
0216fd2
Use fork and execve instead
yim-lee Dec 2, 2023
984f556
Fix compilation error on macOS
yim-lee Dec 2, 2023
e8b89b5
Fix compilation error in macOS
yim-lee Dec 5, 2023
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
32 changes: 25 additions & 7 deletions Sources/TSCBasic/Process.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -658,6 +658,7 @@ public final class Process {
posix_spawn_file_actions_init(&fileActions)
defer { posix_spawn_file_actions_destroy(&fileActions) }

var spawnFunc: SpawnFunc = .posix_spawn
if let workingDirectory = workingDirectory?.pathString {
#if canImport(Darwin)
// The only way to set a workingDirectory is using an availability-gated initializer, so we don't need
Expand All @@ -667,11 +668,11 @@ public final class Process {
posix_spawn_file_actions_addchdir_np(&fileActions, workingDirectory)
}
#elseif os(Linux)
guard SPM_posix_spawn_file_actions_addchdir_np_supported() else {
throw Process.Error.workingDirectoryNotSupported
if SPM_posix_spawn_file_actions_addchdir_np_supported() {
SPM_posix_spawn_file_actions_addchdir_np(&fileActions, workingDirectory)
} else {
spawnFunc = .fork_exec(workingDirectory: workingDirectory)
}

SPM_posix_spawn_file_actions_addchdir_np(&fileActions, workingDirectory)
#else
throw Process.Error.workingDirectoryNotSupported
#endif
Expand Down Expand Up @@ -725,8 +726,18 @@ public final class Process {
}
let argv = CStringArray(resolvedArgs)
let env = CStringArray(environment.map({ "\($0.0)=\($0.1)" }))
let rv = posix_spawnp(&processID, argv.cArray[0]!, &fileActions, &attributes, argv.cArray, env.cArray)

let rv: Int32
switch spawnFunc {
case .posix_spawn:
rv = posix_spawnp(&processID, argv.cArray[0]!, &fileActions, &attributes, argv.cArray, env.cArray)
case .fork_exec(let workingDirectory):
#if os(Linux)
rv = SPM_fork_exec_chdir(&processID, workingDirectory, argv.cArray[0]!, argv.cArray, env.cArray, &stdinPipe, &outputPipe, &stderrPipe, outputRedirection.redirectsOutput, outputRedirection.redirectStderr)
#else
throw Process.Error.workingDirectoryNotSupported
#endif
}

guard rv == 0 else {
throw SystemError.posix_spawn(rv, arguments)
}
Expand Down Expand Up @@ -1349,3 +1360,10 @@ extension Process {
stdoutStream.flush()
}
}

extension Process {
enum SpawnFunc {
case posix_spawn
case fork_exec(workingDirectory: String)
}
}
4 changes: 4 additions & 0 deletions Sources/TSCclibc/include/process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,8 @@ int SPM_posix_spawn_file_actions_addchdir_np(posix_spawn_file_actions_t *restric
// Runtime check for the availability of posix_spawn_file_actions_addchdir_np. Returns 0 if the method is available, -1 if not.
bool SPM_posix_spawn_file_actions_addchdir_np_supported();

// Alternative for posix_spawn_file_actions_addchdir_np on Linux that mimics its behavior using fork, exec*, and chdir.
int SPM_fork_exec_chdir(pid_t *pid, const char *target_directory, const char *cmd, char *const argv[], char *const envp[],
int in_pipe[], int out_pipe[], int err_pipe[], bool redirect_out, bool redirect_err);

#endif
61 changes: 61 additions & 0 deletions Sources/TSCclibc/process.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
#endif

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>

#include "process.h"

Expand Down Expand Up @@ -32,4 +36,61 @@ bool SPM_posix_spawn_file_actions_addchdir_np_supported() {
#endif
}

int SPM_fork_exec_chdir(pid_t *pid, const char *cwd,
const char *cmd, char *const argv[], char *const envp[],
int in_pipe[], int out_pipe[], int err_pipe[], bool redirect_out, bool redirect_err) {
*pid = fork();

if (*pid < 0) {
perror("fork() failed");
_exit(EXIT_FAILURE);
} else if (*pid > 0) { // Parent process
// Wait for child process to finish
int status;
waitpid(*pid, &status, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is suspicious: usually the parent needs to read from the stdout & stderr pipes it passed into the child. If not, the child will block if it writes more than the pipe has space for (usually somewhere between 8 and 128 kiB).

return status;
} else { // Child process
// Change working directory then execute cmd
if (chdir(cwd)) {
_exit(EXIT_FAILURE);
}

// Replicate pipe logic in TSCBasic.Process.launch()

// Dupe the read portion of the remote to 0.
dup2(in_pipe[0], 0);
// Close the other side's pipe since it was duped to 0.
close(in_pipe[0]);
close(in_pipe[1]);

if (redirect_out) {
// Open the write end of the pipe.
dup2(out_pipe[1], 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

who's reading that pipe? It appears like the parent waits for the child to complete and then reads those pipes.

That's a deadlock.

The child will fill up the pipe until it's full (assuming the child writes enough). And then the child will block until something reads from that pipe. So the child won't exit but the parent will wait for it to exit:

child waits for parent to read <-> parent waits for child to exit

// Close the other ends of the pipe since they were duped to 1.
close(out_pipe[0]);
close(out_pipe[1]);

if (redirect_err) {
// If merged was requested, send stderr to stdout.
dup2(1, 2);
} else {
// If no redirect was requested, open the pipe for stderr.
dup2(err_pipe[1], 2);
// Close the other ends of the pipe since they were dupped to 2.
close(err_pipe[0]);
close(err_pipe[1]);
}
} else {
dup2(1, 1);
dup2(2, 2);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

there are a bunch of things missing here:

  • we need a loop to close all other inherited file descriptors
  • the signal mask is also inherited into the child. On dispatch worker threads pretty much all signals are blocked. So you'd inherit that which means that even a kill CHILD_PID would not kill the child. So you'd want to reset the signal mask
  • here's some code that I have written in a previous life: https://github.com/BromiumInc/BromiumCoreUtils/blob/2ec1c52a09831893618b9620383015e4362f7aa5/BromiumCoreUtils/BRUTask.m#L277-L347 . It's a mostly complete NSTask re-implementation in Obj-C. But the fork/exec code is pretty much completely straight C anyway, so you can look at that (it's BSD licensed)

execve(cmd, argv, envp);

// If execve returns, it must have failed.
perror(cmd);
_exit(EXIT_FAILURE);
}
}

#endif