Skip to content

Commit 7dc6e54

Browse files
jakepetroulesjakepetroules
authored
Disable tests which double-close file descriptors (#153)
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.
1 parent 3d88b68 commit 7dc6e54

File tree

3 files changed

+77
-72
lines changed

3 files changed

+77
-72
lines changed

‎Tests/SubprocessTests/IntegrationTests.swift‎

Lines changed: 42 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -973,17 +973,19 @@ extension SubprocessIntegrationTests {
973973
options: .create,
974974
permissions: [.ownerReadWrite, .groupReadWrite]
975975
)
976-
let echoResult = try await _run(
977-
setup,
978-
input: .none,
979-
output: .fileDescriptor(
980-
outputFile,
981-
closeAfterSpawningProcess: false
982-
),
983-
error: .discarded
984-
)
985-
#expect(echoResult.terminationStatus.isSuccess)
986-
try outputFile.close()
976+
let echoResult = try await outputFile.closeAfter {
977+
let echoResult = try await _run(
978+
setup,
979+
input: .none,
980+
output: .fileDescriptor(
981+
outputFile,
982+
closeAfterSpawningProcess: false
983+
),
984+
error: .discarded
985+
)
986+
#expect(echoResult.terminationStatus.isSuccess)
987+
return echoResult
988+
}
987989
let outputData: Data = try Data(
988990
contentsOf: URL(filePath: outputFilePath.string)
989991
)
@@ -994,7 +996,7 @@ extension SubprocessIntegrationTests {
994996
#expect(output == expected)
995997
}
996998

997-
@Test func testFileDescriptorOutputAutoClose() async throws {
999+
@Test(.disabled("Cannot ever safely call unbalanced close() on the same fd")) func testFileDescriptorOutputAutoClose() async throws {
9981000
#if os(Windows)
9991001
let setup = TestSetup(
10001002
executable: .name("cmd.exe"),
@@ -1254,17 +1256,19 @@ extension SubprocessIntegrationTests {
12541256
options: .create,
12551257
permissions: [.ownerReadWrite, .groupReadWrite]
12561258
)
1257-
let echoResult = try await _run(
1258-
setup,
1259-
input: .none,
1260-
output: .discarded,
1261-
error: .fileDescriptor(
1262-
outputFile,
1263-
closeAfterSpawningProcess: false
1259+
let echoResult = try await outputFile.closeAfter {
1260+
let echoResult = try await _run(
1261+
setup,
1262+
input: .none,
1263+
output: .discarded,
1264+
error: .fileDescriptor(
1265+
outputFile,
1266+
closeAfterSpawningProcess: false
1267+
)
12641268
)
1265-
)
1266-
#expect(echoResult.terminationStatus.isSuccess)
1267-
try outputFile.close()
1269+
#expect(echoResult.terminationStatus.isSuccess)
1270+
return echoResult
1271+
}
12681272
let outputData: Data = try Data(
12691273
contentsOf: URL(filePath: outputFilePath.string)
12701274
)
@@ -1275,7 +1279,7 @@ extension SubprocessIntegrationTests {
12751279
#expect(output == expected)
12761280
}
12771281

1278-
@Test func testFileDescriptorErrorOutputAutoClose() async throws {
1282+
@Test(.disabled("Cannot ever safely call unbalanced close() on the same fd")) func testFileDescriptorErrorOutputAutoClose() async throws {
12791283
#if os(Windows)
12801284
let setup = TestSetup(
12811285
executable: .name("cmd.exe"),
@@ -1338,20 +1342,22 @@ extension SubprocessIntegrationTests {
13381342
options: .create,
13391343
permissions: [.ownerReadWrite, .groupReadWrite]
13401344
)
1341-
let echoResult = try await _run(
1342-
setup,
1343-
input: .none,
1344-
output: .fileDescriptor(
1345-
outputFile,
1346-
closeAfterSpawningProcess: false
1347-
),
1348-
error: .fileDescriptor(
1349-
outputFile,
1350-
closeAfterSpawningProcess: false
1345+
let echoResult = try await outputFile.closeAfter {
1346+
let echoResult = try await _run(
1347+
setup,
1348+
input: .none,
1349+
output: .fileDescriptor(
1350+
outputFile,
1351+
closeAfterSpawningProcess: false
1352+
),
1353+
error: .fileDescriptor(
1354+
outputFile,
1355+
closeAfterSpawningProcess: false
1356+
)
13511357
)
1352-
)
1353-
#expect(echoResult.terminationStatus.isSuccess)
1354-
try outputFile.close()
1358+
#expect(echoResult.terminationStatus.isSuccess)
1359+
return echoResult
1360+
}
13551361
let outputData: Data = try Data(
13561362
contentsOf: URL(filePath: outputFilePath.string)
13571363
)

‎Tests/SubprocessTests/ProcessMonitoringTests.swift‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ extension SubprocessProcessMonitoringTests {
299299
let testCount = 100
300300
var spawnedProcesses: [ProcessIdentifier] = []
301301

302+
defer {
303+
for pid in spawnedProcesses {
304+
pid.close()
305+
}
306+
}
307+
302308
for _ in 0 ..< testCount {
303309
let config = self.immediateExitProcess(withExitCode: 0)
304310
let spawnResult = try config.spawn(
@@ -309,12 +315,6 @@ extension SubprocessProcessMonitoringTests {
309315
spawnedProcesses.append(spawnResult.execution.processIdentifier)
310316
}
311317

312-
defer {
313-
for pid in spawnedProcesses {
314-
pid.close()
315-
}
316-
}
317-
318318
try await withThrowingTaskGroup { group in
319319
for pid in spawnedProcesses {
320320
group.addTask {

‎Tests/SubprocessTests/UnixTests.swift‎

Lines changed: 29 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -381,37 +381,36 @@ extension SubprocessUnixTests {
381381

382382
@Test(.requiresBash) func testSubprocessDoesNotInheritRandomFileDescriptors() async throws {
383383
let pipe = try FileDescriptor.ssp_pipe()
384-
defer {
385-
try? pipe.readEnd.close()
386-
try? pipe.writeEnd.close()
387-
}
388-
// Spawn bash and then attempt to write to the write end
389-
let result = try await Subprocess.run(
390-
.path("/bin/bash"),
391-
arguments: [
392-
"-c",
393-
"""
394-
echo this string should be discarded >&\(pipe.writeEnd.rawValue);
395-
echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?;
396-
"""
397-
],
398-
input: .none,
399-
output: .string(limit: 64),
400-
error: .discarded
401-
)
402-
try pipe.writeEnd.close()
403-
#expect(result.terminationStatus.isSuccess)
404-
// Make sure nothing is written to the pipe
405-
var readBytes: [UInt8] = Array(repeating: 0, count: 1024)
406-
let readCount = try readBytes.withUnsafeMutableBytes { ptr in
407-
return try FileDescriptor(rawValue: pipe.readEnd.rawValue)
408-
.read(into: ptr, retryOnInterrupt: true)
384+
try await pipe.readEnd.closeAfter {
385+
let result = try await pipe.writeEnd.closeAfter {
386+
// Spawn bash and then attempt to write to the write end
387+
try await Subprocess.run(
388+
.path("/bin/bash"),
389+
arguments: [
390+
"-c",
391+
"""
392+
echo this string should be discarded >&\(pipe.writeEnd.rawValue);
393+
echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?;
394+
"""
395+
],
396+
input: .none,
397+
output: .string(limit: 64),
398+
error: .discarded
399+
)
400+
}
401+
#expect(result.terminationStatus.isSuccess)
402+
// Make sure nothing is written to the pipe
403+
var readBytes: [UInt8] = Array(repeating: 0, count: 1024)
404+
let readCount = try readBytes.withUnsafeMutableBytes { ptr in
405+
return try FileDescriptor(rawValue: pipe.readEnd.rawValue)
406+
.read(into: ptr, retryOnInterrupt: true)
407+
}
408+
#expect(readCount == 0)
409+
#expect(
410+
result.standardOutput?.trimmingNewLineAndQuotes() ==
411+
"wrote into \(pipe.writeEnd.rawValue), echo exit code 1"
412+
)
409413
}
410-
#expect(readCount == 0)
411-
#expect(
412-
result.standardOutput?.trimmingNewLineAndQuotes() ==
413-
"wrote into \(pipe.writeEnd.rawValue), echo exit code 1"
414-
)
415414
}
416415
}
417416

0 commit comments

Comments
 (0)