From b3a5f960d0753e10a019e37e14e3a12ad2654a2d Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Wed, 20 Aug 2025 23:07:16 -0700 Subject: [PATCH] Disable tests which double-close file descriptors A couple tests attempt to double-close a file descriptor. You cannot ever safely do this as even the act of spawning a process itself may have re-allocated the same file descriptor in the calling process - for example, the pidfd file descriptor, or other threads may have changed state. Disable these tests until we figure out what to do with them. It's possible the solution may involve #161, where Subprocess's own FileDescriptor type can have its own logic for refusing to double-close, and we validate the condition at that level rather than at the POSIX level. This hasn't failed in CI so far due to the older Linux kernel in use. Also moves some other code to using closeAfter to guarantee proper cleanup. --- Tests/SubprocessTests/IntegrationTests.swift | 78 ++++++++++--------- .../ProcessMonitoringTests.swift | 12 +-- Tests/SubprocessTests/UnixTests.swift | 59 +++++++------- 3 files changed, 77 insertions(+), 72 deletions(-) diff --git a/Tests/SubprocessTests/IntegrationTests.swift b/Tests/SubprocessTests/IntegrationTests.swift index a2f6431a..d60be0e9 100644 --- a/Tests/SubprocessTests/IntegrationTests.swift +++ b/Tests/SubprocessTests/IntegrationTests.swift @@ -973,17 +973,19 @@ extension SubprocessIntegrationTests { options: .create, permissions: [.ownerReadWrite, .groupReadWrite] ) - let echoResult = try await _run( - setup, - input: .none, - output: .fileDescriptor( - outputFile, - closeAfterSpawningProcess: false - ), - error: .discarded - ) - #expect(echoResult.terminationStatus.isSuccess) - try outputFile.close() + let echoResult = try await outputFile.closeAfter { + let echoResult = try await _run( + setup, + input: .none, + output: .fileDescriptor( + outputFile, + closeAfterSpawningProcess: false + ), + error: .discarded + ) + #expect(echoResult.terminationStatus.isSuccess) + return echoResult + } let outputData: Data = try Data( contentsOf: URL(filePath: outputFilePath.string) ) @@ -994,7 +996,7 @@ extension SubprocessIntegrationTests { #expect(output == expected) } - @Test func testFileDescriptorOutputAutoClose() async throws { + @Test(.disabled("Cannot ever safely call unbalanced close() on the same fd")) func testFileDescriptorOutputAutoClose() async throws { #if os(Windows) let setup = TestSetup( executable: .name("cmd.exe"), @@ -1254,17 +1256,19 @@ extension SubprocessIntegrationTests { options: .create, permissions: [.ownerReadWrite, .groupReadWrite] ) - let echoResult = try await _run( - setup, - input: .none, - output: .discarded, - error: .fileDescriptor( - outputFile, - closeAfterSpawningProcess: false + let echoResult = try await outputFile.closeAfter { + let echoResult = try await _run( + setup, + input: .none, + output: .discarded, + error: .fileDescriptor( + outputFile, + closeAfterSpawningProcess: false + ) ) - ) - #expect(echoResult.terminationStatus.isSuccess) - try outputFile.close() + #expect(echoResult.terminationStatus.isSuccess) + return echoResult + } let outputData: Data = try Data( contentsOf: URL(filePath: outputFilePath.string) ) @@ -1275,7 +1279,7 @@ extension SubprocessIntegrationTests { #expect(output == expected) } - @Test func testFileDescriptorErrorOutputAutoClose() async throws { + @Test(.disabled("Cannot ever safely call unbalanced close() on the same fd")) func testFileDescriptorErrorOutputAutoClose() async throws { #if os(Windows) let setup = TestSetup( executable: .name("cmd.exe"), @@ -1338,20 +1342,22 @@ extension SubprocessIntegrationTests { options: .create, permissions: [.ownerReadWrite, .groupReadWrite] ) - let echoResult = try await _run( - setup, - input: .none, - output: .fileDescriptor( - outputFile, - closeAfterSpawningProcess: false - ), - error: .fileDescriptor( - outputFile, - closeAfterSpawningProcess: false + let echoResult = try await outputFile.closeAfter { + let echoResult = try await _run( + setup, + input: .none, + output: .fileDescriptor( + outputFile, + closeAfterSpawningProcess: false + ), + error: .fileDescriptor( + outputFile, + closeAfterSpawningProcess: false + ) ) - ) - #expect(echoResult.terminationStatus.isSuccess) - try outputFile.close() + #expect(echoResult.terminationStatus.isSuccess) + return echoResult + } let outputData: Data = try Data( contentsOf: URL(filePath: outputFilePath.string) ) diff --git a/Tests/SubprocessTests/ProcessMonitoringTests.swift b/Tests/SubprocessTests/ProcessMonitoringTests.swift index 3f31d8c8..742e263c 100644 --- a/Tests/SubprocessTests/ProcessMonitoringTests.swift +++ b/Tests/SubprocessTests/ProcessMonitoringTests.swift @@ -299,6 +299,12 @@ extension SubprocessProcessMonitoringTests { let testCount = 100 var spawnedProcesses: [ProcessIdentifier] = [] + defer { + for pid in spawnedProcesses { + pid.close() + } + } + for _ in 0 ..< testCount { let config = self.immediateExitProcess(withExitCode: 0) let spawnResult = try config.spawn( @@ -309,12 +315,6 @@ extension SubprocessProcessMonitoringTests { spawnedProcesses.append(spawnResult.execution.processIdentifier) } - defer { - for pid in spawnedProcesses { - pid.close() - } - } - try await withThrowingTaskGroup { group in for pid in spawnedProcesses { group.addTask { diff --git a/Tests/SubprocessTests/UnixTests.swift b/Tests/SubprocessTests/UnixTests.swift index 0d154bca..ccb8013b 100644 --- a/Tests/SubprocessTests/UnixTests.swift +++ b/Tests/SubprocessTests/UnixTests.swift @@ -381,37 +381,36 @@ extension SubprocessUnixTests { @Test(.requiresBash) func testSubprocessDoesNotInheritRandomFileDescriptors() async throws { let pipe = try FileDescriptor.ssp_pipe() - defer { - try? pipe.readEnd.close() - try? pipe.writeEnd.close() - } - // Spawn bash and then attempt to write to the write end - let result = try await Subprocess.run( - .path("/bin/bash"), - arguments: [ - "-c", - """ - echo this string should be discarded >&\(pipe.writeEnd.rawValue); - echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?; - """ - ], - input: .none, - output: .string(limit: 64), - error: .discarded - ) - try pipe.writeEnd.close() - #expect(result.terminationStatus.isSuccess) - // Make sure nothing is written to the pipe - var readBytes: [UInt8] = Array(repeating: 0, count: 1024) - let readCount = try readBytes.withUnsafeMutableBytes { ptr in - return try FileDescriptor(rawValue: pipe.readEnd.rawValue) - .read(into: ptr, retryOnInterrupt: true) + try await pipe.readEnd.closeAfter { + let result = try await pipe.writeEnd.closeAfter { + // Spawn bash and then attempt to write to the write end + try await Subprocess.run( + .path("/bin/bash"), + arguments: [ + "-c", + """ + echo this string should be discarded >&\(pipe.writeEnd.rawValue); + echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?; + """ + ], + input: .none, + output: .string(limit: 64), + error: .discarded + ) + } + #expect(result.terminationStatus.isSuccess) + // Make sure nothing is written to the pipe + var readBytes: [UInt8] = Array(repeating: 0, count: 1024) + let readCount = try readBytes.withUnsafeMutableBytes { ptr in + return try FileDescriptor(rawValue: pipe.readEnd.rawValue) + .read(into: ptr, retryOnInterrupt: true) + } + #expect(readCount == 0) + #expect( + result.standardOutput?.trimmingNewLineAndQuotes() == + "wrote into \(pipe.writeEnd.rawValue), echo exit code 1" + ) } - #expect(readCount == 0) - #expect( - result.standardOutput?.trimmingNewLineAndQuotes() == - "wrote into \(pipe.writeEnd.rawValue), echo exit code 1" - ) } }