From fc7983b53d691eff373e76fed446421f49467219 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Wed, 22 Jun 2022 12:30:56 +0100 Subject: [PATCH 1/9] Do not chdir before calling posix_spawn We allow users to set the working directory of the child process when they use the Process API. Unfortunately, this was implemented by calling chdir in the parent process, which has nasty global effects on the process and cannot safely be used concurrently. glibc has a non-POSIX extension to posix_spawn that can call chdir in the child process, so we add that call instead where it's available, and add a regression test. --- Sources/Foundation/Process.swift | 12 ++++++ Tests/Foundation/Tests/TestProcess.swift | 49 ++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/Sources/Foundation/Process.swift b/Sources/Foundation/Process.swift index e064711296..01b8a45612 100644 --- a/Sources/Foundation/Process.swift +++ b/Sources/Foundation/Process.swift @@ -944,6 +944,17 @@ open class Process: NSObject { } #endif + #if os(Linux) + if let dir = currentDirectoryURL?.path { + try _throwIfPosixError(posix_spawn_file_actions_addchdir_np(fileActions, dir)) + } + #else + // This is an unfortunate workaround: posix_spawn has no POSIX-specified way to set the working directory + // of the child process. glibc has a non-POSIX API option, which we use above. Here we take a brute-force + // approach of just changing our current working directory. This is not a great implementation and it's likely + // to cause subtle issues in those environments. However, the Apple Foundation library doesn't have this problem, + // and this library does the right thing on Linux and Windows, so the overwhelming majority of users are + // well-served. let fileManager = FileManager() let previousDirectoryPath = fileManager.currentDirectoryPath if let dir = currentDirectoryURL?.path, !fileManager.changeCurrentDirectoryPath(dir) { @@ -954,6 +965,7 @@ open class Process: NSObject { // Reset the previous working directory path. fileManager.changeCurrentDirectoryPath(previousDirectoryPath) } + #endif // Launch var pid = pid_t() diff --git a/Tests/Foundation/Tests/TestProcess.swift b/Tests/Foundation/Tests/TestProcess.swift index df8972c243..354f790b9d 100644 --- a/Tests/Foundation/Tests/TestProcess.swift +++ b/Tests/Foundation/Tests/TestProcess.swift @@ -7,6 +7,8 @@ // See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors // +import Dispatch + class TestProcess : XCTestCase { func test_exit0() throws { @@ -699,6 +701,53 @@ class TestProcess : XCTestCase { } } + func test_currentDirectoryDoesNotChdirParentProcess() throws { + // This test only behaves correctly on Linux and Windows: other platforms don't have an + // appropriate API for this in posix_spawn or similar. + #if os(Linux) || os(Windows) + let backgroundQueue = DispatchQueue(label: "background-processor") + let group = DispatchGroup() + let startSemaphore = DispatchSemaphore() + let currentWorkingDirectory = FileManager.shared.currentDirectoryPath + var shouldRun = true + let shouldRunLock = Lock() + + XCTAssertNotEqual(currentWorkingDirectory, "/") + + // Kick off the background task. This will spin on our current working directory and confirm + // it doesn't change. + backgroundQueue.async(group: group) { + startSemaphore.signal() + + while true { + let newCWD = FileManager.shared.currentDirectoryPath + XCTAssertEqual(newCWD, currentWorkingDirectory) + + shouldRunLock.lock() + if shouldRun { + shouldRunLock.unlock() + } else { + shouldRunLock.unlock() + break + } + } + } + + startSemaphore.wait() + + // We run the task 50 times just to try to encourage it to fail. + for _ in 0..<50 { + XCTAssertNoThrow(try runTask([xdgTestHelperURL().path, "--getcwd"], currentDirectoryPath: "/")) + } + + shouldRunLock.lock() + shouldRun = false + shouldRunLock.unlock() + + group.wait() + #endif + } + #if !os(Windows) func test_fileDescriptorsAreNotInherited() throws { let task = Process() From 15aa308ec2971c786a2c528a6cc76496d1b3202b Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Mon, 14 Nov 2022 13:08:09 +0000 Subject: [PATCH 2/9] Spike --- CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h index 642151ab34..f2cd946368 100644 --- a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h +++ b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h @@ -709,6 +709,12 @@ CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT pa #else CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT path, _CFPosixSpawnFileActionsRef file_actions, _CFPosixSpawnAttrRef _Nullable _CF_RESTRICT attrp, char *_Nullable const argv[_Nullable _CF_RESTRICT], char *_Nullable const envp[_Nullable _CF_RESTRICT]); #endif // __cplusplus + +#if TARGET_OS_LINUX +static inline _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *_CF_RESTRICT path) { + +} +#endif // TARGET_OS_LINUX #endif // !TARGET_OS_WIN32 _CF_EXPORT_SCOPE_END From 2bcb3dc105257a02952d5779349b0260d5415112 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:13:07 +0000 Subject: [PATCH 3/9] Expose non-posix extension in CoreFoundation --- CoreFoundation/Base.subproj/CFPlatform.c | 4 ++++ Sources/Foundation/Process.swift | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CoreFoundation/Base.subproj/CFPlatform.c b/CoreFoundation/Base.subproj/CFPlatform.c index 040e8966f2..3c13da72c8 100644 --- a/CoreFoundation/Base.subproj/CFPlatform.c +++ b/CoreFoundation/Base.subproj/CFPlatform.c @@ -2210,6 +2210,10 @@ CF_EXPORT int _CFPosixSpawnFileActionsAddClose(_CFPosixSpawnFileActionsRef file_ return posix_spawn_file_actions_addclose((posix_spawn_file_actions_t *)file_actions, filedes); } +CF_EXPORT int _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *path) { + return posix_spawn_file_actions_addchdir_np((posix_spawn_file_actions_t *)file_actions, path); +} + CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT path, _CFPosixSpawnFileActionsRef file_actions, _CFPosixSpawnAttrRef _Nullable _CF_RESTRICT attrp, char *_Nullable const argv[_Nullable _CF_RESTRICT], char *_Nullable const envp[_Nullable _CF_RESTRICT]) { return posix_spawn(pid, path, (posix_spawn_file_actions_t *)file_actions, (posix_spawnattr_t *)attrp, argv, envp); } diff --git a/Sources/Foundation/Process.swift b/Sources/Foundation/Process.swift index 01b8a45612..e788a83b6b 100644 --- a/Sources/Foundation/Process.swift +++ b/Sources/Foundation/Process.swift @@ -946,7 +946,7 @@ open class Process: NSObject { #if os(Linux) if let dir = currentDirectoryURL?.path { - try _throwIfPosixError(posix_spawn_file_actions_addchdir_np(fileActions, dir)) + try _throwIfPosixError(_CFPosixSpawnFileActionsAddChdirNP(fileActions, dir)) } #else // This is an unfortunate workaround: posix_spawn has no POSIX-specified way to set the working directory From 1e1bef5169b798bad608c7c98e531c0da6d54de0 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:14:08 +0000 Subject: [PATCH 4/9] Add missing header definition --- CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h | 1 + 1 file changed, 1 insertion(+) diff --git a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h index f2cd946368..3cb5052317 100644 --- a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h +++ b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h @@ -704,6 +704,7 @@ CF_EXPORT int _CFPosixSpawnFileActionsDestroy(_CFPosixSpawnFileActionsRef file_a CF_EXPORT void _CFPosixSpawnFileActionsDealloc(_CFPosixSpawnFileActionsRef file_actions); CF_EXPORT int _CFPosixSpawnFileActionsAddDup2(_CFPosixSpawnFileActionsRef file_actions, int filedes, int newfiledes); CF_EXPORT int _CFPosixSpawnFileActionsAddClose(_CFPosixSpawnFileActionsRef file_actions, int filedes); +CF_EXPORT int _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *path); #ifdef __cplusplus CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT path, _CFPosixSpawnFileActionsRef file_actions, _CFPosixSpawnAttrRef _Nullable _CF_RESTRICT attrp, char *const argv[], char *const envp[]); #else From aa486760131c4109dbf29a3d309e6330253ae151 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:16:20 +0000 Subject: [PATCH 5/9] Remove duplicate definition --- CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h index 3cb5052317..811f3fd6d7 100644 --- a/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h +++ b/CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h @@ -704,7 +704,6 @@ CF_EXPORT int _CFPosixSpawnFileActionsDestroy(_CFPosixSpawnFileActionsRef file_a CF_EXPORT void _CFPosixSpawnFileActionsDealloc(_CFPosixSpawnFileActionsRef file_actions); CF_EXPORT int _CFPosixSpawnFileActionsAddDup2(_CFPosixSpawnFileActionsRef file_actions, int filedes, int newfiledes); CF_EXPORT int _CFPosixSpawnFileActionsAddClose(_CFPosixSpawnFileActionsRef file_actions, int filedes); -CF_EXPORT int _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *path); #ifdef __cplusplus CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT path, _CFPosixSpawnFileActionsRef file_actions, _CFPosixSpawnAttrRef _Nullable _CF_RESTRICT attrp, char *const argv[], char *const envp[]); #else @@ -712,9 +711,7 @@ CF_EXPORT int _CFPosixSpawn(pid_t *_CF_RESTRICT pid, const char *_CF_RESTRICT pa #endif // __cplusplus #if TARGET_OS_LINUX -static inline _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *_CF_RESTRICT path) { - -} +CF_EXPORT int _CFPosixSpawnFileActionsAddChdirNP(_CFPosixSpawnFileActionsRef file_actions, const char *path); #endif // TARGET_OS_LINUX #endif // !TARGET_OS_WIN32 From 270412d0b5cb6cdde7b769e286acc35d959494ae Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:19:33 +0000 Subject: [PATCH 6/9] Get the test compiling --- Tests/Foundation/Tests/TestProcess.swift | 6 +- Tests/pre-push | 78 ++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 Tests/pre-push diff --git a/Tests/Foundation/Tests/TestProcess.swift b/Tests/Foundation/Tests/TestProcess.swift index 354f790b9d..15c16073b7 100644 --- a/Tests/Foundation/Tests/TestProcess.swift +++ b/Tests/Foundation/Tests/TestProcess.swift @@ -707,10 +707,10 @@ class TestProcess : XCTestCase { #if os(Linux) || os(Windows) let backgroundQueue = DispatchQueue(label: "background-processor") let group = DispatchGroup() - let startSemaphore = DispatchSemaphore() + let startSemaphore = DispatchSemaphore(value: 0) let currentWorkingDirectory = FileManager.shared.currentDirectoryPath var shouldRun = true - let shouldRunLock = Lock() + let shouldRunLock = NSLock() XCTAssertNotEqual(currentWorkingDirectory, "/") @@ -720,7 +720,7 @@ class TestProcess : XCTestCase { startSemaphore.signal() while true { - let newCWD = FileManager.shared.currentDirectoryPath + let newCWD = FileManager.default.currentDirectoryPath XCTAssertEqual(newCWD, currentWorkingDirectory) shouldRunLock.lock() diff --git a/Tests/pre-push b/Tests/pre-push new file mode 100644 index 0000000000..b2a4cd6ff2 --- /dev/null +++ b/Tests/pre-push @@ -0,0 +1,78 @@ +#!/bin/bash + +set -e + +export SKIP_FETCH=1 +echo "Testing pre-push hook..." + +# Setup workspace +workspace=/tmp/test_$RANDOM +hook=hooks/pre-push + +echo "1. Test internal and public fetch url" + +test1_dir=$workspace/test_1 +mkdir -p $test1_dir +git_cmd="git -C $test1_dir" +git init $test1_dir +cp $hook $test1_dir/.git/hooks/ +touch $test1_dir/foo +$git_cmd checkout -b main +$git_cmd add foo +$git_cmd commit -m "First commit" +$git_cmd status +$git_cmd remote add internal ssh://stash.sd.apple.com:test/test1.git +$git_cmd remote add public git@github.com:apple/swift-issues.git +set +e +$git_cmd push --dry-run public main +test1_status=$? +set -e + +echo "2. Test only public url in the remote" + +test2_dir=$workspace/test_2 +mkdir -p $test2_dir +git_cmd="git -C $test2_dir" +git init $test2_dir +cp $hook $test2_dir/.git/hooks/ +touch $test2_dir/foo +$git_cmd checkout -b test2 +$git_cmd add foo +$git_cmd commit -m "First commit" +$git_cmd status +$git_cmd remote add public git@github.com:apple/swift-issues.git +set +e +$git_cmd push --dry-run public test2 +test2_status=$? +set -e + +echo "3. Test only internal url in the remote" + +test3_dir=$workspace/test_3 +mkdir -p $test3_dir +git_cmd="git -C $test3_dir" +git init $test3_dir +cp $hook $test3_dir/.git/hooks/ +touch $test3_dir/foo +$git_cmd checkout -b test3 +$git_cmd add foo +$git_cmd commit -m "First commit" +$git_cmd status +$git_cmd remote add internal ssh://git@stash.sd.apple.com/devtoolsint/dt-git-hooks.git +set +e +$git_cmd push --dry-run internal test3 +test3_status=$? +set -e + +function assert { + if [[ $1 == $2 ]]; then + printf '%s' "✅" + else + printf '%s' "❌" + fi +} +echo "====================" +echo "Test1: `assert $test1_status 1`" +echo "Test2: `assert $test2_status 0`" +echo "Test3: `assert $test3_status 0`" +echo "====================" From df5ad314f1370cd0ea52a759d7e46515312afc20 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:24:44 +0000 Subject: [PATCH 7/9] Get compiling --- Tests/Foundation/Tests/TestProcess.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Foundation/Tests/TestProcess.swift b/Tests/Foundation/Tests/TestProcess.swift index 15c16073b7..702e4a3fe4 100644 --- a/Tests/Foundation/Tests/TestProcess.swift +++ b/Tests/Foundation/Tests/TestProcess.swift @@ -708,7 +708,7 @@ class TestProcess : XCTestCase { let backgroundQueue = DispatchQueue(label: "background-processor") let group = DispatchGroup() let startSemaphore = DispatchSemaphore(value: 0) - let currentWorkingDirectory = FileManager.shared.currentDirectoryPath + let currentWorkingDirectory = FileManager.default.currentDirectoryPath var shouldRun = true let shouldRunLock = NSLock() From 09b0f07bb484c77065546518b304cdbc863d9577 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:25:47 +0000 Subject: [PATCH 8/9] Remove pre-push --- Tests/pre-push | 78 -------------------------------------------------- 1 file changed, 78 deletions(-) delete mode 100644 Tests/pre-push diff --git a/Tests/pre-push b/Tests/pre-push deleted file mode 100644 index b2a4cd6ff2..0000000000 --- a/Tests/pre-push +++ /dev/null @@ -1,78 +0,0 @@ -#!/bin/bash - -set -e - -export SKIP_FETCH=1 -echo "Testing pre-push hook..." - -# Setup workspace -workspace=/tmp/test_$RANDOM -hook=hooks/pre-push - -echo "1. Test internal and public fetch url" - -test1_dir=$workspace/test_1 -mkdir -p $test1_dir -git_cmd="git -C $test1_dir" -git init $test1_dir -cp $hook $test1_dir/.git/hooks/ -touch $test1_dir/foo -$git_cmd checkout -b main -$git_cmd add foo -$git_cmd commit -m "First commit" -$git_cmd status -$git_cmd remote add internal ssh://stash.sd.apple.com:test/test1.git -$git_cmd remote add public git@github.com:apple/swift-issues.git -set +e -$git_cmd push --dry-run public main -test1_status=$? -set -e - -echo "2. Test only public url in the remote" - -test2_dir=$workspace/test_2 -mkdir -p $test2_dir -git_cmd="git -C $test2_dir" -git init $test2_dir -cp $hook $test2_dir/.git/hooks/ -touch $test2_dir/foo -$git_cmd checkout -b test2 -$git_cmd add foo -$git_cmd commit -m "First commit" -$git_cmd status -$git_cmd remote add public git@github.com:apple/swift-issues.git -set +e -$git_cmd push --dry-run public test2 -test2_status=$? -set -e - -echo "3. Test only internal url in the remote" - -test3_dir=$workspace/test_3 -mkdir -p $test3_dir -git_cmd="git -C $test3_dir" -git init $test3_dir -cp $hook $test3_dir/.git/hooks/ -touch $test3_dir/foo -$git_cmd checkout -b test3 -$git_cmd add foo -$git_cmd commit -m "First commit" -$git_cmd status -$git_cmd remote add internal ssh://git@stash.sd.apple.com/devtoolsint/dt-git-hooks.git -set +e -$git_cmd push --dry-run internal test3 -test3_status=$? -set -e - -function assert { - if [[ $1 == $2 ]]; then - printf '%s' "✅" - else - printf '%s' "❌" - fi -} -echo "====================" -echo "Test1: `assert $test1_status 1`" -echo "Test2: `assert $test2_status 0`" -echo "Test3: `assert $test3_status 0`" -echo "====================" From ba3f9c5a0f2777893d7ce53fb37beadfaf48aaeb Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 18 Nov 2022 12:39:54 +0000 Subject: [PATCH 9/9] Correctly run the test --- Tests/Foundation/Tests/TestProcess.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Tests/Foundation/Tests/TestProcess.swift b/Tests/Foundation/Tests/TestProcess.swift index 702e4a3fe4..b179a430c2 100644 --- a/Tests/Foundation/Tests/TestProcess.swift +++ b/Tests/Foundation/Tests/TestProcess.swift @@ -745,6 +745,8 @@ class TestProcess : XCTestCase { shouldRunLock.unlock() group.wait() + #else + throw XCTSkip() #endif } @@ -915,6 +917,7 @@ class TestProcess : XCTestCase { ("test_currentDirectory", test_currentDirectory), ("test_pipeCloseBeforeLaunch", test_pipeCloseBeforeLaunch), ("test_multiProcesses", test_multiProcesses), + ("test_currentDirectoryDoesNotChdirParentProcess", test_currentDirectoryDoesNotChdirParentProcess), ] #if !os(Windows)