Skip to content

Commit e4242f6

Browse files
jakepetroulesjakepetroules
committed
Isolate tests which double-close file descriptors using exit tests
A couple tests attempt to double-close a file descriptor, which is inherently racy. Force these tests to run in a separate process in order to guarantee that they can't potentially close a re-used file descriptor number when running concurrently with other tests in the parent process. Also moves some other code to using closeAfter to guarantee proper cleanup.
1 parent f9881e5 commit e4242f6

File tree

3 files changed

+156
-141
lines changed

3 files changed

+156
-141
lines changed

‎Tests/SubprocessTests/IntegrationTests.swift‎

Lines changed: 121 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -965,17 +965,19 @@ extension SubprocessIntegrationTests {
965965
options: .create,
966966
permissions: [.ownerReadWrite, .groupReadWrite]
967967
)
968-
let echoResult = try await _run(
969-
setup,
970-
input: .none,
971-
output: .fileDescriptor(
972-
outputFile,
973-
closeAfterSpawningProcess: false
974-
),
975-
error: .discarded
976-
)
977-
#expect(echoResult.terminationStatus.isSuccess)
978-
try outputFile.close()
968+
let echoResult = try await outputFile.closeAfter {
969+
let echoResult = try await _run(
970+
setup,
971+
input: .none,
972+
output: .fileDescriptor(
973+
outputFile,
974+
closeAfterSpawningProcess: false
975+
),
976+
error: .discarded
977+
)
978+
#expect(echoResult.terminationStatus.isSuccess)
979+
return echoResult
980+
}
979981
let outputData: Data = try Data(
980982
contentsOf: URL(filePath: outputFilePath.string)
981983
)
@@ -986,44 +988,49 @@ extension SubprocessIntegrationTests {
986988
#expect(output == expected)
987989
}
988990

991+
#if compiler(>=6.2)
989992
@Test func testFileDescriptorOutputAutoClose() async throws {
990-
#if os(Windows)
991-
let setup = TestSetup(
992-
executable: .name("cmd.exe"),
993-
arguments: ["/c", "echo Hello World"]
994-
)
995-
#else
996-
let setup = TestSetup(
997-
executable: .path("/bin/sh"),
998-
arguments: ["-c", "echo Hello World"]
999-
)
1000-
#endif
1001-
let outputFilePath = FilePath(FileManager.default.temporaryDirectory._fileSystemPath)
1002-
.appending("Test.out")
1003-
if FileManager.default.fileExists(atPath: outputFilePath.string) {
1004-
try FileManager.default.removeItem(atPath: outputFilePath.string)
1005-
}
1006-
let outputFile: FileDescriptor = try .open(
1007-
outputFilePath,
1008-
.readWrite,
1009-
options: .create,
1010-
permissions: [.ownerReadWrite, .groupReadWrite]
1011-
)
1012-
let echoResult = try await _run(
1013-
setup,
1014-
input: .none,
1015-
output: .fileDescriptor(
1016-
outputFile,
1017-
closeAfterSpawningProcess: true
1018-
),
1019-
error: .discarded
1020-
)
1021-
#expect(echoResult.terminationStatus.isSuccess)
1022-
// Make sure the file descriptor is already closed
1023-
#expect(throws: Errno.badFileDescriptor) {
1024-
try outputFile.close()
1025-
}
993+
// Use an exit test to isolate the test runner process from our deliberate attempt to double-close a file descriptor
994+
await #expect(processExitsWith: .success, performing: {
995+
#if os(Windows)
996+
let setup = TestSetup(
997+
executable: .name("cmd.exe"),
998+
arguments: ["/c", "echo Hello World"]
999+
)
1000+
#else
1001+
let setup = TestSetup(
1002+
executable: .path("/bin/sh"),
1003+
arguments: ["-c", "echo Hello World"]
1004+
)
1005+
#endif
1006+
let outputFilePath = FilePath(FileManager.default.temporaryDirectory._fileSystemPath)
1007+
.appending("Test.out")
1008+
if FileManager.default.fileExists(atPath: outputFilePath.string) {
1009+
try FileManager.default.removeItem(atPath: outputFilePath.string)
1010+
}
1011+
let outputFile: FileDescriptor = try .open(
1012+
outputFilePath,
1013+
.readWrite,
1014+
options: .create,
1015+
permissions: [.ownerReadWrite, .groupReadWrite]
1016+
)
1017+
let echoResult = try await _run(
1018+
setup,
1019+
input: .none,
1020+
output: .fileDescriptor(
1021+
outputFile,
1022+
closeAfterSpawningProcess: true
1023+
),
1024+
error: .discarded
1025+
)
1026+
#expect(echoResult.terminationStatus.isSuccess)
1027+
// Make sure the file descriptor is already closed
1028+
#expect(throws: Errno.badFileDescriptor) {
1029+
try outputFile.close()
1030+
}
1031+
})
10261032
}
1033+
#endif
10271034

10281035
#if SubprocessFoundation
10291036
@Test func testDataOutput() async throws {
@@ -1242,17 +1249,19 @@ extension SubprocessIntegrationTests {
12421249
options: .create,
12431250
permissions: [.ownerReadWrite, .groupReadWrite]
12441251
)
1245-
let echoResult = try await _run(
1246-
setup,
1247-
input: .none,
1248-
output: .discarded,
1249-
error: .fileDescriptor(
1250-
outputFile,
1251-
closeAfterSpawningProcess: false
1252+
let echoResult = try await outputFile.closeAfter {
1253+
let echoResult = try await _run(
1254+
setup,
1255+
input: .none,
1256+
output: .discarded,
1257+
error: .fileDescriptor(
1258+
outputFile,
1259+
closeAfterSpawningProcess: false
1260+
)
12521261
)
1253-
)
1254-
#expect(echoResult.terminationStatus.isSuccess)
1255-
try outputFile.close()
1262+
#expect(echoResult.terminationStatus.isSuccess)
1263+
return echoResult
1264+
}
12561265
let outputData: Data = try Data(
12571266
contentsOf: URL(filePath: outputFilePath.string)
12581267
)
@@ -1263,44 +1272,49 @@ extension SubprocessIntegrationTests {
12631272
#expect(output == expected)
12641273
}
12651274

1275+
#if compiler(>=6.2)
12661276
@Test func testFileDescriptorErrorOutputAutoClose() async throws {
1267-
#if os(Windows)
1268-
let setup = TestSetup(
1269-
executable: .name("cmd.exe"),
1270-
arguments: ["/c", "echo Hello World", "1>&2"]
1271-
)
1272-
#else
1273-
let setup = TestSetup(
1274-
executable: .path("/bin/sh"),
1275-
arguments: ["-c", "echo Hello World", "1>&2"]
1276-
)
1277-
#endif
1278-
let outputFilePath = FilePath(FileManager.default.temporaryDirectory._fileSystemPath)
1279-
.appending("TestError.out")
1280-
if FileManager.default.fileExists(atPath: outputFilePath.string) {
1281-
try FileManager.default.removeItem(atPath: outputFilePath.string)
1282-
}
1283-
let outputFile: FileDescriptor = try .open(
1284-
outputFilePath,
1285-
.readWrite,
1286-
options: .create,
1287-
permissions: [.ownerReadWrite, .groupReadWrite]
1288-
)
1289-
let echoResult = try await _run(
1290-
setup,
1291-
input: .none,
1292-
output: .discarded,
1293-
error: .fileDescriptor(
1294-
outputFile,
1295-
closeAfterSpawningProcess: true
1277+
// Use an exit test to isolate the test runner process from our deliberate attempt to double-close a file descriptor
1278+
await #expect(processExitsWith: .success, performing: {
1279+
#if os(Windows)
1280+
let setup = TestSetup(
1281+
executable: .name("cmd.exe"),
1282+
arguments: ["/c", "echo Hello World", "1>&2"]
12961283
)
1297-
)
1298-
#expect(echoResult.terminationStatus.isSuccess)
1299-
// Make sure the file descriptor is already closed
1300-
#expect(throws: Errno.badFileDescriptor) {
1301-
try outputFile.close()
1302-
}
1284+
#else
1285+
let setup = TestSetup(
1286+
executable: .path("/bin/sh"),
1287+
arguments: ["-c", "echo Hello World", "1>&2"]
1288+
)
1289+
#endif
1290+
let outputFilePath = FilePath(FileManager.default.temporaryDirectory._fileSystemPath)
1291+
.appending("TestError.out")
1292+
if FileManager.default.fileExists(atPath: outputFilePath.string) {
1293+
try FileManager.default.removeItem(atPath: outputFilePath.string)
1294+
}
1295+
let outputFile: FileDescriptor = try .open(
1296+
outputFilePath,
1297+
.readWrite,
1298+
options: .create,
1299+
permissions: [.ownerReadWrite, .groupReadWrite]
1300+
)
1301+
let echoResult = try await _run(
1302+
setup,
1303+
input: .none,
1304+
output: .discarded,
1305+
error: .fileDescriptor(
1306+
outputFile,
1307+
closeAfterSpawningProcess: true
1308+
)
1309+
)
1310+
#expect(echoResult.terminationStatus.isSuccess)
1311+
// Make sure the file descriptor is already closed
1312+
#expect(throws: Errno.badFileDescriptor) {
1313+
try outputFile.close()
1314+
}
1315+
})
13031316
}
1317+
#endif
13041318

13051319
@Test func testFileDescriptorOutputErrorToSameFile() async throws {
13061320
#if os(Windows)
@@ -1326,20 +1340,22 @@ extension SubprocessIntegrationTests {
13261340
options: .create,
13271341
permissions: [.ownerReadWrite, .groupReadWrite]
13281342
)
1329-
let echoResult = try await _run(
1330-
setup,
1331-
input: .none,
1332-
output: .fileDescriptor(
1333-
outputFile,
1334-
closeAfterSpawningProcess: false
1335-
),
1336-
error: .fileDescriptor(
1337-
outputFile,
1338-
closeAfterSpawningProcess: false
1343+
let echoResult = try await outputFile.closeAfter {
1344+
let echoResult = try await _run(
1345+
setup,
1346+
input: .none,
1347+
output: .fileDescriptor(
1348+
outputFile,
1349+
closeAfterSpawningProcess: false
1350+
),
1351+
error: .fileDescriptor(
1352+
outputFile,
1353+
closeAfterSpawningProcess: false
1354+
)
13391355
)
1340-
)
1341-
#expect(echoResult.terminationStatus.isSuccess)
1342-
try outputFile.close()
1356+
#expect(echoResult.terminationStatus.isSuccess)
1357+
return echoResult
1358+
}
13431359
let outputData: Data = try Data(
13441360
contentsOf: URL(filePath: outputFilePath.string)
13451361
)

‎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
@@ -401,37 +401,36 @@ extension SubprocessUnixTests {
401401
)
402402
func testSubprocessDoesNotInheritRandomFileDescriptors() async throws {
403403
let pipe = try FileDescriptor.ssp_pipe()
404-
defer {
405-
try? pipe.readEnd.close()
406-
try? pipe.writeEnd.close()
407-
}
408-
// Spawn bash and then attempt to write to the write end
409-
let result = try await Subprocess.run(
410-
.path("/bin/sh"),
411-
arguments: [
412-
"-c",
413-
"""
414-
echo this string should be discarded >&\(pipe.writeEnd.rawValue);
415-
echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?;
416-
"""
417-
],
418-
input: .none,
419-
output: .string(limit: 64),
420-
error: .discarded
421-
)
422-
try pipe.writeEnd.close()
423-
#expect(result.terminationStatus.isSuccess)
424-
// Make sure nothing is written to the pipe
425-
var readBytes: [UInt8] = Array(repeating: 0, count: 1024)
426-
let readCount = try readBytes.withUnsafeMutableBytes { ptr in
427-
return try FileDescriptor(rawValue: pipe.readEnd.rawValue)
428-
.read(into: ptr, retryOnInterrupt: true)
404+
try await pipe.readEnd.closeAfter {
405+
let result = try await pipe.writeEnd.closeAfter {
406+
// Spawn bash and then attempt to write to the write end
407+
try await Subprocess.run(
408+
.path("/bin/sh"),
409+
arguments: [
410+
"-c",
411+
"""
412+
echo this string should be discarded >&\(pipe.writeEnd.rawValue);
413+
echo wrote into \(pipe.writeEnd.rawValue), echo exit code $?;
414+
"""
415+
],
416+
input: .none,
417+
output: .string(limit: 64),
418+
error: .discarded
419+
)
420+
}
421+
#expect(result.terminationStatus.isSuccess)
422+
// Make sure nothing is written to the pipe
423+
var readBytes: [UInt8] = Array(repeating: 0, count: 1024)
424+
let readCount = try readBytes.withUnsafeMutableBytes { ptr in
425+
return try FileDescriptor(rawValue: pipe.readEnd.rawValue)
426+
.read(into: ptr, retryOnInterrupt: true)
427+
}
428+
#expect(readCount == 0)
429+
#expect(
430+
result.standardOutput?.trimmingNewLineAndQuotes() ==
431+
"wrote into \(pipe.writeEnd.rawValue), echo exit code 1"
432+
)
429433
}
430-
#expect(readCount == 0)
431-
#expect(
432-
result.standardOutput?.trimmingNewLineAndQuotes() ==
433-
"wrote into \(pipe.writeEnd.rawValue), echo exit code 1"
434-
)
435434
}
436435
}
437436

0 commit comments

Comments
 (0)