From 8d83f3c56d239a6cf28d0d3fc0beb1437dfdd57a 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 d9449366f7..ec02996b35 100644 --- a/Sources/Foundation/Process.swift +++ b/Sources/Foundation/Process.swift @@ -938,6 +938,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) { @@ -948,6 +959,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 e314be517d..b8ec27e922 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 2b689f646d0d815214e3e580ac396579a1e2bd89 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 233b6f8775..62a233fdb8 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 335c9634377a92cbf18ef7c96a9e149018d14283 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 b4a274568f..d5e65c5be0 100644 --- a/CoreFoundation/Base.subproj/CFPlatform.c +++ b/CoreFoundation/Base.subproj/CFPlatform.c @@ -2158,6 +2158,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 ec02996b35..ea91825e49 100644 --- a/Sources/Foundation/Process.swift +++ b/Sources/Foundation/Process.swift @@ -940,7 +940,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 091bacfbf56d032ea7eff635056e98a0cea5891a 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 62a233fdb8..ff338743d2 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 634cd524f359946cf85ace6777b57aff6193d0bc 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 ff338743d2..46c1779155 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 67e71df0d879472b5f719897553c2f1a238410e0 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 b8ec27e922..80864d453a 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 0f73ddb1ee0406be4f96411138a537e4df7e285a 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 80864d453a..86c46cc52a 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 097ac8c78e850dcb9991be68f37e56e324029c90 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 a0161d29f5fbb77e5271280d96bd3ddd9b651bf8 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 86c46cc52a..aa933e5b45 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)