Skip to content

Commit f835957

Browse files
jakepetroulesjakepetroules
authored
Fix a crash due to improper error handling on Windows (#103)
If the internal preSpawn() function fails, we need to close the file descriptors, same as any other error case. Additionally, createPlatformDiskIO() was effectively creating a clone of the file descriptor leading to a later double-free. Simply return the original object. Closes #84
1 parent 2988b8b commit f835957

File tree

1 file changed

+97
-54
lines changed

1 file changed

+97
-54
lines changed

‎Sources/Subprocess/Platforms/Subprocess+Windows.swift

Lines changed: 97 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,40 @@ extension Configuration {
4848
outputPipe: consuming CreatedPipe,
4949
errorPipe: consuming CreatedPipe
5050
) throws -> SpawnResult {
51+
var inputPipeBox: CreatedPipe? = consume inputPipe
52+
var outputPipeBox: CreatedPipe? = consume outputPipe
53+
var errorPipeBox: CreatedPipe? = consume errorPipe
54+
55+
var _inputPipe = inputPipeBox.take()!
56+
var _outputPipe = outputPipeBox.take()!
57+
var _errorPipe = errorPipeBox.take()!
58+
59+
let inputReadFileDescriptor: TrackedFileDescriptor? = _inputPipe.readFileDescriptor()
60+
let inputWriteFileDescriptor: TrackedFileDescriptor? = _inputPipe.writeFileDescriptor()
61+
let outputReadFileDescriptor: TrackedFileDescriptor? = _outputPipe.readFileDescriptor()
62+
let outputWriteFileDescriptor: TrackedFileDescriptor? = _outputPipe.writeFileDescriptor()
63+
let errorReadFileDescriptor: TrackedFileDescriptor? = _errorPipe.readFileDescriptor()
64+
let errorWriteFileDescriptor: TrackedFileDescriptor? = _errorPipe.writeFileDescriptor()
65+
5166
let (
5267
applicationName,
5368
commandAndArgs,
5469
environment,
5570
intendedWorkingDir
56-
) = try self.preSpawn()
57-
58-
var inputReadFileDescriptor: TrackedFileDescriptor? = inputPipe.readFileDescriptor()
59-
var inputWriteFileDescriptor: TrackedFileDescriptor? = inputPipe.writeFileDescriptor()
60-
var outputReadFileDescriptor: TrackedFileDescriptor? = outputPipe.readFileDescriptor()
61-
var outputWriteFileDescriptor: TrackedFileDescriptor? = outputPipe.writeFileDescriptor()
62-
var errorReadFileDescriptor: TrackedFileDescriptor? = errorPipe.readFileDescriptor()
63-
var errorWriteFileDescriptor: TrackedFileDescriptor? = errorPipe.writeFileDescriptor()
71+
): (String?, String, String, String?)
72+
do {
73+
(applicationName, commandAndArgs, environment, intendedWorkingDir) = try self.preSpawn()
74+
} catch {
75+
try self.safelyCloseMultiple(
76+
inputRead: inputReadFileDescriptor,
77+
inputWrite: inputWriteFileDescriptor,
78+
outputRead: outputReadFileDescriptor,
79+
outputWrite: outputWriteFileDescriptor,
80+
errorRead: errorReadFileDescriptor,
81+
errorWrite: errorWriteFileDescriptor
82+
)
83+
throw error
84+
}
6485

6586
var startupInfo = try self.generateStartupInfo(
6687
withInputRead: inputReadFileDescriptor,
@@ -77,15 +98,15 @@ extension Configuration {
7798
try configurator(&createProcessFlags, &startupInfo)
7899
}
79100
// Spawn!
80-
try applicationName.withOptionalNTPathRepresentation { applicationNameW in
101+
let created = try applicationName.withOptionalNTPathRepresentation { applicationNameW in
81102
try commandAndArgs.withCString(
82103
encodedAs: UTF16.self
83104
) { commandAndArgsW in
84105
try environment.withCString(
85106
encodedAs: UTF16.self
86107
) { environmentW in
87108
try intendedWorkingDir.withOptionalNTPathRepresentation { intendedWorkingDirW in
88-
let created = CreateProcessW(
109+
CreateProcessW(
89110
applicationNameW,
90111
UnsafeMutablePointer<WCHAR>(mutating: commandAndArgsW),
91112
nil, // lpProcessAttributes
@@ -97,26 +118,27 @@ extension Configuration {
97118
&startupInfo,
98119
&processInfo
99120
)
100-
guard created else {
101-
let windowsError = GetLastError()
102-
try self.safelyCloseMultiple(
103-
inputRead: inputReadFileDescriptor.take(),
104-
inputWrite: inputWriteFileDescriptor.take(),
105-
outputRead: outputReadFileDescriptor.take(),
106-
outputWrite: outputWriteFileDescriptor.take(),
107-
errorRead: errorReadFileDescriptor.take(),
108-
errorWrite: errorWriteFileDescriptor.take()
109-
)
110-
throw SubprocessError(
111-
code: .init(.spawnFailed),
112-
underlyingError: .init(rawValue: windowsError)
113-
)
114-
}
115121
}
116122
}
117123
}
118124
}
119125

126+
guard created else {
127+
let windowsError = GetLastError()
128+
try self.safelyCloseMultiple(
129+
inputRead: inputReadFileDescriptor,
130+
inputWrite: inputWriteFileDescriptor,
131+
outputRead: outputReadFileDescriptor,
132+
outputWrite: outputWriteFileDescriptor,
133+
errorRead: errorReadFileDescriptor,
134+
errorWrite: errorWriteFileDescriptor
135+
)
136+
throw SubprocessError(
137+
code: .init(.spawnFailed),
138+
underlyingError: .init(rawValue: windowsError)
139+
)
140+
}
141+
120142
let pid = ProcessIdentifier(
121143
value: processInfo.dwProcessId
122144
)
@@ -157,19 +179,40 @@ extension Configuration {
157179
errorPipe: consuming CreatedPipe,
158180
userCredentials: PlatformOptions.UserCredentials
159181
) throws -> SpawnResult {
182+
var inputPipeBox: CreatedPipe? = consume inputPipe
183+
var outputPipeBox: CreatedPipe? = consume outputPipe
184+
var errorPipeBox: CreatedPipe? = consume errorPipe
185+
186+
var _inputPipe = inputPipeBox.take()!
187+
var _outputPipe = outputPipeBox.take()!
188+
var _errorPipe = errorPipeBox.take()!
189+
190+
let inputReadFileDescriptor: TrackedFileDescriptor? = _inputPipe.readFileDescriptor()
191+
let inputWriteFileDescriptor: TrackedFileDescriptor? = _inputPipe.writeFileDescriptor()
192+
let outputReadFileDescriptor: TrackedFileDescriptor? = _outputPipe.readFileDescriptor()
193+
let outputWriteFileDescriptor: TrackedFileDescriptor? = _outputPipe.writeFileDescriptor()
194+
let errorReadFileDescriptor: TrackedFileDescriptor? = _errorPipe.readFileDescriptor()
195+
let errorWriteFileDescriptor: TrackedFileDescriptor? = _errorPipe.writeFileDescriptor()
196+
160197
let (
161198
applicationName,
162199
commandAndArgs,
163200
environment,
164201
intendedWorkingDir
165-
) = try self.preSpawn()
166-
167-
var inputReadFileDescriptor: TrackedFileDescriptor? = inputPipe.readFileDescriptor()
168-
var inputWriteFileDescriptor: TrackedFileDescriptor? = inputPipe.writeFileDescriptor()
169-
var outputReadFileDescriptor: TrackedFileDescriptor? = outputPipe.readFileDescriptor()
170-
var outputWriteFileDescriptor: TrackedFileDescriptor? = outputPipe.writeFileDescriptor()
171-
var errorReadFileDescriptor: TrackedFileDescriptor? = errorPipe.readFileDescriptor()
172-
var errorWriteFileDescriptor: TrackedFileDescriptor? = errorPipe.writeFileDescriptor()
202+
): (String?, String, String, String?)
203+
do {
204+
(applicationName, commandAndArgs, environment, intendedWorkingDir) = try self.preSpawn()
205+
} catch {
206+
try self.safelyCloseMultiple(
207+
inputRead: inputReadFileDescriptor,
208+
inputWrite: inputWriteFileDescriptor,
209+
outputRead: outputReadFileDescriptor,
210+
outputWrite: outputWriteFileDescriptor,
211+
errorRead: errorReadFileDescriptor,
212+
errorWrite: errorWriteFileDescriptor
213+
)
214+
throw error
215+
}
173216

174217
var startupInfo = try self.generateStartupInfo(
175218
withInputRead: inputReadFileDescriptor,
@@ -186,7 +229,7 @@ extension Configuration {
186229
try configurator(&createProcessFlags, &startupInfo)
187230
}
188231
// Spawn (featuring pyramid!)
189-
try userCredentials.username.withCString(
232+
let created = try userCredentials.username.withCString(
190233
encodedAs: UTF16.self
191234
) { usernameW in
192235
try userCredentials.password.withCString(
@@ -203,7 +246,7 @@ extension Configuration {
203246
encodedAs: UTF16.self
204247
) { environmentW in
205248
try intendedWorkingDir.withOptionalNTPathRepresentation { intendedWorkingDirW in
206-
let created = CreateProcessWithLogonW(
249+
CreateProcessWithLogonW(
207250
usernameW,
208251
domainW,
209252
passwordW,
@@ -216,21 +259,6 @@ extension Configuration {
216259
&startupInfo,
217260
&processInfo
218261
)
219-
guard created else {
220-
let windowsError = GetLastError()
221-
try self.safelyCloseMultiple(
222-
inputRead: inputReadFileDescriptor.take(),
223-
inputWrite: inputWriteFileDescriptor.take(),
224-
outputRead: outputReadFileDescriptor.take(),
225-
outputWrite: outputWriteFileDescriptor.take(),
226-
errorRead: errorReadFileDescriptor.take(),
227-
errorWrite: errorWriteFileDescriptor.take()
228-
)
229-
throw SubprocessError(
230-
code: .init(.spawnFailed),
231-
underlyingError: .init(rawValue: windowsError)
232-
)
233-
}
234262
}
235263
}
236264
}
@@ -239,6 +267,22 @@ extension Configuration {
239267
}
240268
}
241269

270+
guard created else {
271+
let windowsError = GetLastError()
272+
try self.safelyCloseMultiple(
273+
inputRead: inputReadFileDescriptor,
274+
inputWrite: inputWriteFileDescriptor,
275+
outputRead: outputReadFileDescriptor,
276+
outputWrite: outputWriteFileDescriptor,
277+
errorRead: errorReadFileDescriptor,
278+
errorWrite: errorWriteFileDescriptor
279+
)
280+
throw SubprocessError(
281+
code: .init(.spawnFailed),
282+
underlyingError: .init(rawValue: windowsError)
283+
)
284+
}
285+
242286
let pid = ProcessIdentifier(
243287
value: processInfo.dwProcessId
244288
)
@@ -1038,10 +1082,9 @@ extension FileDescriptor {
10381082

10391083
extension TrackedFileDescriptor {
10401084
internal consuming func createPlatformDiskIO() -> TrackedPlatformDiskIO {
1041-
return .init(
1042-
self.fileDescriptor,
1043-
closeWhenDone: self.closeWhenDone
1044-
)
1085+
// TrackedPlatformDiskIO is a typealias of TrackedFileDescriptor on Windows (they're the same type)
1086+
// Just return the same object so we don't create a copy and try to double-close the fd.
1087+
return self
10451088
}
10461089

10471090
internal func readUntilEOF(

0 commit comments

Comments
 (0)