Skip to content

Commit c31f191

Browse files
jakepetroulesjakepetroules
committed
Fix a crash due to improper error handling on Windows
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 c31f191

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)