Call chdir to change working directory in older versions of glibc #449
Conversation
yim-lee
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci please test |
|
@swift-ci test windows |
|
Added more reviewers who either dealt with this problem before, or have to deal with old glibc now. 🙂 |
Sources/TSCclibc/process.c
Outdated
| @@ -13,7 +14,8 @@ int SPM_posix_spawn_file_actions_addchdir_np(posix_spawn_file_actions_t *restric | |||
| # if __GLIBC_PREREQ(2, 29) | |||
| return posix_spawn_file_actions_addchdir_np(file_actions, path); | |||
| # else | |||
| return ENOSYS; | |||
| // Change working directory which child process will inherit | |||
| return chdir(path); | |||
There was a problem hiding this comment.
please don't do this. This is very racy and affects the whole process.
There was a problem hiding this comment.
If you spawn two things at the same time this will fail horribly. return ENOSYS is correct here.
There was a problem hiding this comment.
Is there a way to make it non racy? Or alternative solution?
There was a problem hiding this comment.
With posix_spawn, the only way is posix_spawn_file_actions_addchdir_np.
You could use fork() + execve() and then you could chdir in the child process.
But if you don't want to rewrite to fork/exec and you don't have posix_spawn_file_actions_addchdir_np, then there's no way, no.
There was a problem hiding this comment.
does Foundation process solve this problem, or is posix_spawn_file_actions_addchdir_np used there as well?
There was a problem hiding this comment.
Foundation uses that function as well, which is the cause of swiftlang/swift#59610, a fix for which had to be reverted.
There was a problem hiding this comment.
0216fd2 uses fork and execve. PTAL.
Motivation: `SPM_posix_spawn_file_actions_addchdir_np_supported` and `SPM_posix_spawn_file_actions_addchdir_np` simply return error for glibc versions < 2.29. This causes SwiftPM tools such as `swift package-registry publish` to be unusable on Amazon Linux 2, which has an older version of glibc installed. Modifications: For older versions of glibc, call `chdir` to change working directory as an alternative to `posix_spawn_file_actions_addchdir_np`. This way child process will inherit the working directory path.
yim-lee
force-pushed
the
proc-working-dir
branch
from
8e89879 to
e380e4c
Compare
yim-lee
force-pushed
the
proc-working-dir
branch
from
e380e4c to
0216fd2
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows |
|
@swift-ci please test |
|
@swift-ci please test Windows |
|
@swift-ci please test |
|
Can we make the switch to fork accessible in some way such that we can run the tests on Linux in both posix_spawn and fork modes? Separately, I am a bit concerned that we're introducing yet another capability that Foundation doesn't have, it will make it even harder to ever switch away from maintaining our own process code. |
Will look into this once we think the fork impl generally looks ok |
|
|
||
| if (redirect_out) { | ||
| // Open the write end of the pipe. | ||
| dup2(out_pipe[1], 1); |
There was a problem hiding this comment.
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
| dup2(1, 1); | ||
| dup2(2, 2); | ||
| } | ||
|
|
There was a problem hiding this comment.
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_PIDwould 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
NSTaskre-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)
| } else if (*pid > 0) { // Parent process | ||
| // Wait for child process to finish | ||
| int status; | ||
| waitpid(*pid, &status, 0); |
There was a problem hiding this comment.
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).
|
swiftlang/swift-package-manager#7187 is alternative approach that implements workaround just for unblocking |
|
Doing swiftlang/swift-package-manager#7187 instead. Closing this. |
Motivation:
SPM_posix_spawn_file_actions_addchdir_np_supportedandSPM_posix_spawn_file_actions_addchdir_npsimply return error for glibc versions < 2.29. This causes SwiftPM tools such asswift package-registry publishto be unusable on Amazon Linux 2, which has an older version of glibc installed.Modifications:
For older versions of glibc, call
chdirto change working directory as an alternative toposix_spawn_file_actions_addchdir_np. This way child process will inherit the working directory path.