From 803864d34cd52da795582a7a489d60111f63aa34 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 00:12:15 -0400 Subject: [PATCH 01/10] Reset the working directory of child processes we spawn on Windows. This PR modifies the Windows implementation of `spawnProcess()` so that it sets the working directory of the new process to "C:\" (or thereabouts). This prevents a race condition on Windows because that system won't let you delete a directory if it's the working directory of any process. See [The Old New Thing](https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323) for a very on-the-nose blog post. Note that we do not specify the value of the working directory in an exit test body. A test should generally not rely on it anyway because it is global state and any thread could change its value at any time. I haven't written a unit test for this change because it's unclear what I could write that would be easily verifiable, and because I don't know what state I might perturb outside such a test by calling `SetCurrentDirectory()`. Resolves #1209. (This is a speculative fix.) --- Sources/Testing/ExitTests/SpawnProcess.swift | 25 +++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index fe51a7086..6549dad97 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -284,6 +284,29 @@ func spawnExecutable( let commandLine = _escapeCommandLine(CollectionOfOne(executablePath) + arguments) let environ = environment.map { "\($0.key)=\($0.value)" }.joined(separator: "\0") + "\0\0" + // On Windows, a process holds a reference to its current working + // directory, which prevents other processes from deleting it. This causes + // code to fail if it tries to set the working directory to a temporary + // path. SEE: https://github.com/swiftlang/swift-testing/issues/1209 + // + // This problem manifests for us when we spawn a child process without + // setting its working directory, which causes it to default to that of + // the parent process. To avoid this problem, we set the working directory + // of the new process to the root directory of the boot volume (which is + // unlikely to be deleted, one hopes). + // + // SEE: https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323 + let workingDirectoryPath: UnsafeMutablePointer? = { + let systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" + if systemDrive.last == ":" { + return #"\#(systemDrive)\"# + } + return systemDrive.withCString(encodedAs: UTF16.self) { _wcsdup($0) } + }() + defer { + free(workingDirectoryPath) + } + var flags = DWORD(CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT | EXTENDED_STARTUPINFO_PRESENT) #if DEBUG // Start the process suspended so we can attach a debugger if needed. @@ -302,7 +325,7 @@ func spawnExecutable( true, // bInheritHandles flags, .init(mutating: environ), - nil, + workingDirectoryPath, startupInfo.pointer(to: \.StartupInfo)!, &processInfo ) else { From fc9bbcad38cbab6b3c2ea5acc052f98ae5424615 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 00:38:10 -0400 Subject: [PATCH 02/10] Fix typo --- Sources/Testing/ExitTests/SpawnProcess.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index 6549dad97..0e638f026 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -297,9 +297,9 @@ func spawnExecutable( // // SEE: https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323 let workingDirectoryPath: UnsafeMutablePointer? = { - let systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" + var systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" if systemDrive.last == ":" { - return #"\#(systemDrive)\"# + systemDrive = #"\#(systemDrive)\"# } return systemDrive.withCString(encodedAs: UTF16.self) { _wcsdup($0) } }() From 64b66f3cb12e1d9a0ee2301249acf624ebe5203e Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 10:10:36 -0400 Subject: [PATCH 03/10] Use proper API for adding slash rather than just appending it --- Sources/Testing/ExitTests/SpawnProcess.swift | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index 0e638f026..92a533359 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -229,6 +229,7 @@ func spawnExecutable( return pid } } + #elseif os(Windows) return try _withStartupInfoEx(attributeCount: 1) { startupInfo in func inherit(_ fileHandle: borrowing FileHandle) throws -> HANDLE? { @@ -296,15 +297,17 @@ func spawnExecutable( // unlikely to be deleted, one hopes). // // SEE: https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323 - let workingDirectoryPath: UnsafeMutablePointer? = { - var systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" - if systemDrive.last == ":" { - systemDrive = #"\#(systemDrive)\"# - } - return systemDrive.withCString(encodedAs: UTF16.self) { _wcsdup($0) } - }() + var workingDirectoryPath = UnsafeMutableBufferPointer.allocate(capacity: Int(MAX_PATH)) defer { - free(workingDirectoryPath) + workingDirectoryPath.deallocate() + } + let systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" + systemDrive.withCString(encodedAs: UTF16.self) { systemDrive in + wcscpy_s(workingDirectoryPath.baseAddress!, workingDirectoryPath.count, systemDrive) + let rAddBackslash = PathCchAddBackslashEx(workingDirectoryPath.baseAddress!, workingDirectoryPath.count, nil, nil) + guard rAddBackslash == S_OK || rAddBackslash == S_FALSE else { + fatalError("Unexpected error when normalizing system drive path '\(systemDrive)': HRESULT(\(rAddBackslash))") + } } var flags = DWORD(CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT | EXTENDED_STARTUPINFO_PRESENT) From ff08ca3d3f580eb8106c8f0b32a146d781b99310 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 10:40:07 -0400 Subject: [PATCH 04/10] Use Cch API more cleanly, abstract string munging into a computed variable --- Sources/Testing/ExitTests/SpawnProcess.swift | 55 ++++++++----------- Sources/Testing/Support/FileHandle.swift | 33 +++++++++++ .../Support/FileHandleTests.swift | 20 +++++++ 3 files changed, 76 insertions(+), 32 deletions(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index 92a533359..645164050 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -297,18 +297,7 @@ func spawnExecutable( // unlikely to be deleted, one hopes). // // SEE: https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323 - var workingDirectoryPath = UnsafeMutableBufferPointer.allocate(capacity: Int(MAX_PATH)) - defer { - workingDirectoryPath.deallocate() - } - let systemDrive = Environment.variable(named: "SYSTEMDRIVE") ?? "C:" - systemDrive.withCString(encodedAs: UTF16.self) { systemDrive in - wcscpy_s(workingDirectoryPath.baseAddress!, workingDirectoryPath.count, systemDrive) - let rAddBackslash = PathCchAddBackslashEx(workingDirectoryPath.baseAddress!, workingDirectoryPath.count, nil, nil) - guard rAddBackslash == S_OK || rAddBackslash == S_FALSE else { - fatalError("Unexpected error when normalizing system drive path '\(systemDrive)': HRESULT(\(rAddBackslash))") - } - } + let workingDirectoryPath = rootDirectoryPath var flags = DWORD(CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT | EXTENDED_STARTUPINFO_PRESENT) #if DEBUG @@ -318,30 +307,32 @@ func spawnExecutable( return try commandLine.withCString(encodedAs: UTF16.self) { commandLine in try environ.withCString(encodedAs: UTF16.self) { environ in - var processInfo = PROCESS_INFORMATION() - - guard CreateProcessW( - nil, - .init(mutating: commandLine), - nil, - nil, - true, // bInheritHandles - flags, - .init(mutating: environ), - workingDirectoryPath, - startupInfo.pointer(to: \.StartupInfo)!, - &processInfo - ) else { - throw Win32Error(rawValue: GetLastError()) - } + try workingDirectoryPath.withCString(encodedAs: UTF16.self) { workingDirectoryPath in + var processInfo = PROCESS_INFORMATION() + + guard CreateProcessW( + nil, + .init(mutating: commandLine), + nil, + nil, + true, // bInheritHandles + flags, + .init(mutating: environ), + workingDirectoryPath, + startupInfo.pointer(to: \.StartupInfo)!, + &processInfo + ) else { + throw Win32Error(rawValue: GetLastError()) + } #if DEBUG - // Resume the process. - _ = ResumeThread(processInfo.hThread!) + // Resume the process. + _ = ResumeThread(processInfo.hThread!) #endif - _ = CloseHandle(processInfo.hThread) - return processInfo.hProcess! + _ = CloseHandle(processInfo.hThread) + return processInfo.hProcess! + } } } } diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index 1c5447460..c385815a9 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -719,4 +719,37 @@ func setFD_CLOEXEC(_ flag: Bool, onFileDescriptor fd: CInt) throws { } } #endif + +/// The path to the root directory of the boot volume. +/// +/// On Windows, this string is usually of the form `"C:\"`. On UNIX-like +/// platforms, it is always equal to `"/"`. +let rootDirectoryPath: String = { +#if os(Windows) + if let systemDrive = Environment.variable(named: "SYSTEMDRIVE")?.utf16 { + // Copy the system drive string with room for up to "C:\" and a NUL. + var buffer = UnsafeMutableBufferPointer.allocate(capacity: systemDrive.count + 4) + defer { + buffer.deallocate() + } + buffer.initialize(fromContentsOf: systemDrive) + buffer[systemDrive.count] = 0 + + // On the assumption that the value of %SYSTEMDRIVE% is "C:" or similar, + // ensure a trailing slash is added to refer to the root directory. If + // somebody decides to set this environment variable to something + // nonsensical or to a deeper path, we should accept it silently. + let rAddBackslash = PathCchAddBackslashEx(buffer.baseAddress!, buffer.count, nil, nil) + if rAddBackslash == S_OK || rAddBackslash == S_FALSE, + let result = String.decodeCString(buffer.baseAddress!, as: UTF16.self)?.result { + return result + } + } + // We weren't able to get a path, so fall back to "C:\" on the assumption that + // it's the common case and most likely correct. + return #"C:\"# +#else + return "/" +#endif +}() #endif diff --git a/Tests/TestingTests/Support/FileHandleTests.swift b/Tests/TestingTests/Support/FileHandleTests.swift index 4be633ad6..2b0c93c2f 100644 --- a/Tests/TestingTests/Support/FileHandleTests.swift +++ b/Tests/TestingTests/Support/FileHandleTests.swift @@ -201,6 +201,26 @@ struct FileHandleTests { #endif } #endif + + @Test("Root directory path is correct") + func rootDirectoryPathIsCorrect() async { +#if os(Windows) +#if !SWT_NO_EXIT_TESTS + await #expect(processExitsWith: .success) { + #expect(Environment.setVariable(nil, named: "SYSTEMDRIVE")) + #expect(rootDirectoryPath == #"C:\"#) + + #expect(Environment.setVariable("Q:", named: "SYSTEMDRIVE")) + #expect(rootDirectoryPath == #"Q:\"#) + + #expect(Environment.setVariable("Q:\abc123", named: "SYSTEMDRIVE")) + #expect(rootDirectoryPath == #"Q:\abc123\"#) + } +#endif +#else + #expect(rootDirectoryPath == "/") +#endif + } } // MARK: - Fixtures From a94c0bcf6d36a62e9fa7b037a6b912a42654d8f5 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 10:41:21 -0400 Subject: [PATCH 05/10] Fix typo --- Tests/TestingTests/Support/FileHandleTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/TestingTests/Support/FileHandleTests.swift b/Tests/TestingTests/Support/FileHandleTests.swift index 2b0c93c2f..575dde826 100644 --- a/Tests/TestingTests/Support/FileHandleTests.swift +++ b/Tests/TestingTests/Support/FileHandleTests.swift @@ -213,7 +213,7 @@ struct FileHandleTests { #expect(Environment.setVariable("Q:", named: "SYSTEMDRIVE")) #expect(rootDirectoryPath == #"Q:\"#) - #expect(Environment.setVariable("Q:\abc123", named: "SYSTEMDRIVE")) + #expect(Environment.setVariable(#"Q:\abc123"#, named: "SYSTEMDRIVE")) #expect(rootDirectoryPath == #"Q:\abc123\"#) } #endif From 670b1f87892eea4969a57d2efea58d94e432cb55 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 10:53:06 -0400 Subject: [PATCH 06/10] Hoisted by my own petard --- Tests/TestingTests/Support/FileHandleTests.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Tests/TestingTests/Support/FileHandleTests.swift b/Tests/TestingTests/Support/FileHandleTests.swift index 575dde826..9030cb300 100644 --- a/Tests/TestingTests/Support/FileHandleTests.swift +++ b/Tests/TestingTests/Support/FileHandleTests.swift @@ -209,10 +209,14 @@ struct FileHandleTests { await #expect(processExitsWith: .success) { #expect(Environment.setVariable(nil, named: "SYSTEMDRIVE")) #expect(rootDirectoryPath == #"C:\"#) + } + await #expect(processExitsWith: .success) { #expect(Environment.setVariable("Q:", named: "SYSTEMDRIVE")) #expect(rootDirectoryPath == #"Q:\"#) + } + await #expect(processExitsWith: .success) { #expect(Environment.setVariable(#"Q:\abc123"#, named: "SYSTEMDRIVE")) #expect(rootDirectoryPath == #"Q:\abc123\"#) } From 14310370900bb3a666a5d380760085b406fc31f6 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 11:02:45 -0400 Subject: [PATCH 07/10] Make a copy of the command line while I'm in here per MS docs --- Sources/Testing/ExitTests/SpawnProcess.swift | 55 +++++++++++--------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/Sources/Testing/ExitTests/SpawnProcess.swift b/Sources/Testing/ExitTests/SpawnProcess.swift index 645164050..9f01a1d11 100644 --- a/Sources/Testing/ExitTests/SpawnProcess.swift +++ b/Sources/Testing/ExitTests/SpawnProcess.swift @@ -229,7 +229,6 @@ func spawnExecutable( return pid } } - #elseif os(Windows) return try _withStartupInfoEx(attributeCount: 1) { startupInfo in func inherit(_ fileHandle: borrowing FileHandle) throws -> HANDLE? { @@ -285,6 +284,14 @@ func spawnExecutable( let commandLine = _escapeCommandLine(CollectionOfOne(executablePath) + arguments) let environ = environment.map { "\($0.key)=\($0.value)" }.joined(separator: "\0") + "\0\0" + // CreateProcessW() may modify the command line argument, so we must make + // a mutable copy of it. (environ is also passed as a mutable raw pointer, + // but it is not documented as actually being mutated.) + let commandLineCopy = commandLine.withCString(encodedAs: UTF16.self) { _wcsdup($0) } + defer { + free(commandLineCopy) + } + // On Windows, a process holds a reference to its current working // directory, which prevents other processes from deleting it. This causes // code to fail if it tries to set the working directory to a temporary @@ -305,34 +312,32 @@ func spawnExecutable( flags |= DWORD(CREATE_SUSPENDED) #endif - return try commandLine.withCString(encodedAs: UTF16.self) { commandLine in - try environ.withCString(encodedAs: UTF16.self) { environ in - try workingDirectoryPath.withCString(encodedAs: UTF16.self) { workingDirectoryPath in - var processInfo = PROCESS_INFORMATION() - - guard CreateProcessW( - nil, - .init(mutating: commandLine), - nil, - nil, - true, // bInheritHandles - flags, - .init(mutating: environ), - workingDirectoryPath, - startupInfo.pointer(to: \.StartupInfo)!, - &processInfo - ) else { - throw Win32Error(rawValue: GetLastError()) - } + return try environ.withCString(encodedAs: UTF16.self) { environ in + try workingDirectoryPath.withCString(encodedAs: UTF16.self) { workingDirectoryPath in + var processInfo = PROCESS_INFORMATION() + + guard CreateProcessW( + nil, + commandLineCopy, + nil, + nil, + true, // bInheritHandles + flags, + .init(mutating: environ), + workingDirectoryPath, + startupInfo.pointer(to: \.StartupInfo)!, + &processInfo + ) else { + throw Win32Error(rawValue: GetLastError()) + } #if DEBUG - // Resume the process. - _ = ResumeThread(processInfo.hThread!) + // Resume the process. + _ = ResumeThread(processInfo.hThread!) #endif - _ = CloseHandle(processInfo.hThread) - return processInfo.hProcess! - } + _ = CloseHandle(processInfo.hThread) + return processInfo.hProcess! } } } From 83fe3d080b6dff682dd81b04575e822d908323a2 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 13:29:34 -0400 Subject: [PATCH 08/10] Use GetSystemWindowsDirectoryW() --- Sources/Testing/Support/FileHandle.swift | 36 +++++++++++------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index c385815a9..d0e5cc4d7 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -726,28 +726,26 @@ func setFD_CLOEXEC(_ flag: Bool, onFileDescriptor fd: CInt) throws { /// platforms, it is always equal to `"/"`. let rootDirectoryPath: String = { #if os(Windows) - if let systemDrive = Environment.variable(named: "SYSTEMDRIVE")?.utf16 { - // Copy the system drive string with room for up to "C:\" and a NUL. - var buffer = UnsafeMutableBufferPointer.allocate(capacity: systemDrive.count + 4) - defer { - buffer.deallocate() + var result: String? + + // The boot volume is, except in some legacy scenarios, the volume that + // contains the system Windows directory. For an explanation of the difference + // between the Windows directory and the _system_ Windows directory, see + // https://devblogs.microsoft.com/oldnewthing/20140723-00/?p=423 . + let count = GetSystemWindowsDirectoryW(nil, 0) + if count > 0 { + withUnsafeTemporaryAllocation(of: wchar_t.self, capacity: Int(count) + 1) { buffer in + _ = GetSystemWindowsDirectoryW(buffer.baseAddress!, UINT(buffer.count)) + let rStrip = PathCchStripToRoot(buffer.baseAddress!, buffer.count) + if rStrip == S_OK || rStrip == S_FALSE { + result = String.decodeCString(buffer.baseAddress!, as: UTF16.self)?.result + } } - buffer.initialize(fromContentsOf: systemDrive) - buffer[systemDrive.count] = 0 - // On the assumption that the value of %SYSTEMDRIVE% is "C:" or similar, - // ensure a trailing slash is added to refer to the root directory. If - // somebody decides to set this environment variable to something - // nonsensical or to a deeper path, we should accept it silently. - let rAddBackslash = PathCchAddBackslashEx(buffer.baseAddress!, buffer.count, nil, nil) - if rAddBackslash == S_OK || rAddBackslash == S_FALSE, - let result = String.decodeCString(buffer.baseAddress!, as: UTF16.self)?.result { - return result - } + // If we weren't able to get a path, fall back to "C:\" on the assumption + // that it's the common case and most likely correct. + return result ?? #"C:\"# } - // We weren't able to get a path, so fall back to "C:\" on the assumption that - // it's the common case and most likely correct. - return #"C:\"# #else return "/" #endif From 1df6ba4fd560ee68f2d490d96f2b99c004d0181d Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 13:41:44 -0400 Subject: [PATCH 09/10] Fix typo --- Sources/Testing/Support/FileHandle.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index d0e5cc4d7..37774b91a 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -741,11 +741,11 @@ let rootDirectoryPath: String = { result = String.decodeCString(buffer.baseAddress!, as: UTF16.self)?.result } } - - // If we weren't able to get a path, fall back to "C:\" on the assumption - // that it's the common case and most likely correct. - return result ?? #"C:\"# } + + // If we weren't able to get a path, fall back to "C:\" on the assumption that + // it's the common case and most likely correct. + return result ?? #"C:\"# #else return "/" #endif From 7cb5040cdbb1d86bd216a0391ce9dcd22f633bfd Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Jul 2025 14:17:53 -0400 Subject: [PATCH 10/10] I am not clever --- .../Support/FileHandleTests.swift | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/Tests/TestingTests/Support/FileHandleTests.swift b/Tests/TestingTests/Support/FileHandleTests.swift index 9030cb300..acca1dbea 100644 --- a/Tests/TestingTests/Support/FileHandleTests.swift +++ b/Tests/TestingTests/Support/FileHandleTests.swift @@ -203,24 +203,11 @@ struct FileHandleTests { #endif @Test("Root directory path is correct") - func rootDirectoryPathIsCorrect() async { + func rootDirectoryPathIsCorrect() throws { #if os(Windows) -#if !SWT_NO_EXIT_TESTS - await #expect(processExitsWith: .success) { - #expect(Environment.setVariable(nil, named: "SYSTEMDRIVE")) - #expect(rootDirectoryPath == #"C:\"#) - } - - await #expect(processExitsWith: .success) { - #expect(Environment.setVariable("Q:", named: "SYSTEMDRIVE")) - #expect(rootDirectoryPath == #"Q:\"#) + if let systemDrive = Environment.variable(named: "SYSTEMDRIVE") { + #expect(rootDirectoryPath.starts(with: systemDrive)) } - - await #expect(processExitsWith: .success) { - #expect(Environment.setVariable(#"Q:\abc123"#, named: "SYSTEMDRIVE")) - #expect(rootDirectoryPath == #"Q:\abc123\"#) - } -#endif #else #expect(rootDirectoryPath == "/") #endif