Skip to content

Commit 4ae10a8

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 4ae10a8

File tree

3 files changed

+154
-141
lines changed

3 files changed

+154
-141
lines changed

‎Tests/SubprocessTests/IntegrationTests.swift‎

Lines changed: 119 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
)
@@ -987,42 +989,45 @@ extension SubprocessIntegrationTests {
987989
}
988990

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

10281033
#if SubprocessFoundation
@@ -1242,17 +1247,19 @@ extension SubprocessIntegrationTests {
12421247
options: .create,
12431248
permissions: [.ownerReadWrite, .groupReadWrite]
12441249
)
1245-
let echoResult = try await _run(
1246-
setup,
1247-
input: .none,
1248-
output: .discarded,
1249-
error: .fileDescriptor(
1250-
outputFile,
1251-
closeAfterSpawningProcess: false
1250+
let echoResult = try await outputFile.closeAfter {
1251+
let echoResult = try await _run(
1252+
setup,
1253+
input: .none,
1254+
output: .discarded,
1255+
error: .fileDescriptor(
1256+
outputFile,
1257+
closeAfterSpawningProcess: false
1258+
)
12521259
)
1253-
)
1254-
#expect(echoResult.terminationStatus.isSuccess)
1255-
try outputFile.close()
1260+
#expect(echoResult.terminationStatus.isSuccess)
1261+
return echoResult
1262+
}
12561263
let outputData: Data = try Data(
12571264
contentsOf: URL(filePath: outputFilePath.string)
12581265
)
@@ -1263,44 +1270,49 @@ extension SubprocessIntegrationTests {
12631270
#expect(output == expected)
12641271
}
12651272

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

13051317
@Test func testFileDescriptorOutputErrorToSameFile() async throws {
13061318
#if os(Windows)
@@ -1326,20 +1338,22 @@ extension SubprocessIntegrationTests {
13261338
options: .create,
13271339
permissions: [.ownerReadWrite, .groupReadWrite]
13281340
)
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
1341+
let echoResult = try await outputFile.closeAfter {
1342+
let echoResult = try await _run(
1343+
setup,
1344+
input: .none,
1345+
output: .fileDescriptor(
1346+
outputFile,
1347+
closeAfterSpawningProcess: false
1348+
),
1349+
error: .fileDescriptor(
1350+
outputFile,
1351+
closeAfterSpawningProcess: false
1352+
)
13391353
)
1340-
)
1341-
#expect(echoResult.terminationStatus.isSuccess)
1342-
try outputFile.close()
1354+
#expect(echoResult.terminationStatus.isSuccess)
1355+
return echoResult
1356+
}
13431357
let outputData: Data = try Data(
13441358
contentsOf: URL(filePath: outputFilePath.string)
13451359
)

‎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)