Skip to content

Commit 91c45b4

Browse files
author
Itai Ferber
committed
Restore encoder state after throwing on encode
Resolve SR-6078 by restoring the JSON/PlistEncoder stack if an error is thrown after a container was pushed during encode.
1 parent de596ee commit 91c45b4

File tree

4 files changed

+184
-5
lines changed

4 files changed

+184
-5
lines changed

‎stdlib/public/SDK/Foundation/JSONEncoder.swift

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,7 @@ extension _JSONEncoder {
722722
switch self.options.dateEncodingStrategy {
723723
case .deferredToDate:
724724
// Must be called with a surrounding with(pushedKey:) call.
725+
// Dates encode as single-value objects; this can't both throw and push a container, so no need to catch the error.
725726
try date.encode(to: self)
726727
return self.storage.popContainer()
727728

@@ -743,7 +744,16 @@ extension _JSONEncoder {
743744

744745
case .custom(let closure):
745746
let depth = self.storage.count
746-
try closure(date, self)
747+
do {
748+
try closure(date, self)
749+
} catch {
750+
// If the value pushed a container before throwing, pop it back off to restore state.
751+
if self.storage.count > depth {
752+
let _ = self.storage.popContainer()
753+
}
754+
755+
throw error
756+
}
747757

748758
guard self.storage.count > depth else {
749759
// The closure didn't encode anything. Return the default keyed container.
@@ -759,15 +769,36 @@ extension _JSONEncoder {
759769
switch self.options.dataEncodingStrategy {
760770
case .deferredToData:
761771
// Must be called with a surrounding with(pushedKey:) call.
762-
try data.encode(to: self)
772+
let depth = self.storage.count
773+
do {
774+
try data.encode(to: self)
775+
} catch {
776+
// If the value pushed a container before throwing, pop it back off to restore state.
777+
// This shouldn't be possible for Data (which encodes as an array of bytes), but it can't hurt to catch a failure.
778+
if self.storage.count > depth {
779+
let _ = self.storage.popContainer()
780+
}
781+
782+
throw error
783+
}
784+
763785
return self.storage.popContainer()
764786

765787
case .base64:
766788
return NSString(string: data.base64EncodedString())
767789

768790
case .custom(let closure):
769791
let depth = self.storage.count
770-
try closure(data, self)
792+
do {
793+
try closure(data, self)
794+
} catch {
795+
// If the value pushed a container before throwing, pop it back off to restore state.
796+
if self.storage.count > depth {
797+
let _ = self.storage.popContainer()
798+
}
799+
800+
throw error
801+
}
771802

772803
guard self.storage.count > depth else {
773804
// The closure didn't encode anything. Return the default keyed container.
@@ -801,7 +832,16 @@ extension _JSONEncoder {
801832

802833
// The value should request a container from the _JSONEncoder.
803834
let depth = self.storage.count
804-
try value.encode(to: self)
835+
do {
836+
try value.encode(to: self)
837+
} catch {
838+
// If the value pushed a container before throwing, pop it back off to restore state.
839+
if self.storage.count > depth {
840+
let _ = self.storage.popContainer()
841+
}
842+
843+
throw error
844+
}
805845

806846
// The top container should be a new container.
807847
guard self.storage.count > depth else {

‎stdlib/public/SDK/Foundation/PlistEncoder.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,16 @@ extension _PlistEncoder {
490490

491491
// The value should request a container from the _PlistEncoder.
492492
let depth = self.storage.count
493-
try value.encode(to: self)
493+
do {
494+
try value.encode(to: self)
495+
} catch let error {
496+
// If the value pushed a container before throwing, pop it back off to restore state.
497+
if self.storage.count > depth {
498+
let _ = self.storage.popContainer()
499+
}
500+
501+
throw error
502+
}
494503

495504
// The top container should be a new container.
496505
guard self.storage.count > depth else {

‎test/stdlib/TestJSONEncoder.swift

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,85 @@ class TestJSONEncoder : TestJSONEncoderSuper {
799799
expectEqual(type(of: decoded), Employee.self, "Expected decoded value to be of type Employee; got \(type(of: decoded)) instead.")
800800
}
801801

802+
// MARK: - Encoder State
803+
// SR-6078
804+
func testEncoderStateThrowOnEncode() {
805+
struct ReferencingEncoderWrapper<T : Encodable> : Encodable {
806+
let value: T
807+
init(_ value: T) { self.value = value }
808+
809+
func encode(to encoder: Encoder) throws {
810+
// This approximates a subclass calling into its superclass, where the superclass encodes a value that might throw.
811+
// The key here is that getting the superEncoder creates a referencing encoder.
812+
var container = encoder.unkeyedContainer()
813+
let superEncoder = container.superEncoder()
814+
815+
// Pushing a nested container on leaves the referencing encoder with multiple containers.
816+
var nestedContainer = superEncoder.unkeyedContainer()
817+
try nestedContainer.encode(value)
818+
}
819+
}
820+
821+
// The structure that would be encoded here looks like
822+
//
823+
// [[[Float.infinity]]]
824+
//
825+
// The wrapper asks for an unkeyed container ([^]), gets a super encoder, and creates a nested container into that ([[^]]).
826+
// We then encode an array into that ([[[^]]]), which happens to be a value that causes us to throw an error.
827+
//
828+
// The issue at hand reproduces when you have a referencing encoder (superEncoder() creates one) that has a container on the stack (unkeyedContainer() adds one) that encodes a value going through box_() (Array does that) that encodes something which throws (Float.infinity does that).
829+
// When reproducing, this will cause a test failure via fatalError().
830+
_ = try? JSONEncoder().encode(ReferencingEncoderWrapper([Float.infinity]))
831+
}
832+
833+
func testEncoderStateThrowOnEncodeCustomDate() {
834+
// This test is identical to testEncoderStateThrowOnEncode, except throwing via a custom Date closure.
835+
struct ReferencingEncoderWrapper<T : Encodable> : Encodable {
836+
let value: T
837+
init(_ value: T) { self.value = value }
838+
func encode(to encoder: Encoder) throws {
839+
var container = encoder.unkeyedContainer()
840+
let superEncoder = container.superEncoder()
841+
var nestedContainer = superEncoder.unkeyedContainer()
842+
try nestedContainer.encode(value)
843+
}
844+
}
845+
846+
// The closure needs to push a container before throwing an error to trigger.
847+
let encoder = JSONEncoder()
848+
encoder.dateEncodingStrategy = .custom({ _, encoder in
849+
let _ = encoder.unkeyedContainer()
850+
enum CustomError : Error { case foo }
851+
throw CustomError.foo
852+
})
853+
854+
_ = try? encoder.encode(ReferencingEncoderWrapper(Date()))
855+
}
856+
857+
func testEncoderStateThrowOnEncodeCustomData() {
858+
// This test is identical to testEncoderStateThrowOnEncode, except throwing via a custom Data closure.
859+
struct ReferencingEncoderWrapper<T : Encodable> : Encodable {
860+
let value: T
861+
init(_ value: T) { self.value = value }
862+
func encode(to encoder: Encoder) throws {
863+
var container = encoder.unkeyedContainer()
864+
let superEncoder = container.superEncoder()
865+
var nestedContainer = superEncoder.unkeyedContainer()
866+
try nestedContainer.encode(value)
867+
}
868+
}
869+
870+
// The closure needs to push a container before throwing an error to trigger.
871+
let encoder = JSONEncoder()
872+
encoder.dataEncodingStrategy = .custom({ _, encoder in
873+
let _ = encoder.unkeyedContainer()
874+
enum CustomError : Error { case foo }
875+
throw CustomError.foo
876+
})
877+
878+
_ = try? encoder.encode(ReferencingEncoderWrapper(Data()))
879+
}
880+
802881
// MARK: - Helper Functions
803882
private var _jsonEmptyDictionary: Data {
804883
return "{}".data(using: .utf8)!
@@ -1439,5 +1518,8 @@ JSONEncoderTests.test("testInterceptDecimal") { TestJSONEncoder().testInterceptD
14391518
JSONEncoderTests.test("testInterceptURL") { TestJSONEncoder().testInterceptURL() }
14401519
JSONEncoderTests.test("testTypeCoercion") { TestJSONEncoder().testTypeCoercion() }
14411520
JSONEncoderTests.test("testDecodingConcreteTypeParameter") { TestJSONEncoder().testDecodingConcreteTypeParameter() }
1521+
JSONEncoderTests.test("testEncoderStateThrowOnEncode") { TestJSONEncoder().testEncoderStateThrowOnEncode() }
1522+
JSONEncoderTests.test("testEncoderStateThrowOnEncodeCustomDate") { TestJSONEncoder().testEncoderStateThrowOnEncodeCustomDate() }
1523+
JSONEncoderTests.test("testEncoderStateThrowOnEncodeCustomData") { TestJSONEncoder().testEncoderStateThrowOnEncodeCustomData() }
14421524
runAllTests()
14431525
#endif

‎test/stdlib/TestPlistEncoder.swift

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,53 @@ class TestPropertyListEncoder : TestPropertyListEncoderSuper {
214214
expectEqual(type(of: decoded), Employee.self, "Expected decoded value to be of type Employee; got \(type(of: decoded)) instead.")
215215
}
216216

217+
// MARK: - Encoder State
218+
// SR-6078
219+
func testEncoderStateThrowOnEncode() {
220+
struct Wrapper<T : Encodable> : Encodable {
221+
let value: T
222+
init(_ value: T) { self.value = value }
223+
224+
func encode(to encoder: Encoder) throws {
225+
// This approximates a subclass calling into its superclass, where the superclass encodes a value that might throw.
226+
// The key here is that getting the superEncoder creates a referencing encoder.
227+
var container = encoder.unkeyedContainer()
228+
let superEncoder = container.superEncoder()
229+
230+
// Pushing a nested container on leaves the referencing encoder with multiple containers.
231+
var nestedContainer = superEncoder.unkeyedContainer()
232+
try nestedContainer.encode(value)
233+
}
234+
}
235+
236+
struct Throwing : Encodable {
237+
private enum EncodingError : Error {
238+
case foo
239+
}
240+
241+
func encode(to encoder: Encoder) throws {
242+
throw EncodingError.foo
243+
}
244+
}
245+
246+
// The structure that would be encoded here looks like
247+
//
248+
// <array>
249+
// <array>
250+
// <array>
251+
// [throwing]
252+
// </array>
253+
// </array>
254+
// </array>
255+
//
256+
// The wrapper asks for an unkeyed container ([^]), gets a super encoder, and creates a nested container into that ([[^]]).
257+
// We then encode an array into that ([[[^]]]), which happens to be a value that causes us to throw an error.
258+
//
259+
// The issue at hand reproduces when you have a referencing encoder (superEncoder() creates one) that has a container on the stack (unkeyedContainer() adds one) that encodes a value going through box_() (Array does that) that encodes something which throws (Throwing does that).
260+
// When reproducing, this will cause a test failure via fatalError().
261+
_ = try? PropertyListEncoder().encode(Wrapper([Throwing()]))
262+
}
263+
217264
// MARK: - Helper Functions
218265
private var _plistEmptyDictionaryBinary: Data {
219266
return Data(base64Encoded: "YnBsaXN0MDDQCAAAAAAAAAEBAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAJ")!
@@ -748,5 +795,6 @@ PropertyListEncoderTests.test("testInterceptData") { TestPropertyListEncoder().t
748795
PropertyListEncoderTests.test("testInterceptDate") { TestPropertyListEncoder().testInterceptDate() }
749796
PropertyListEncoderTests.test("testTypeCoercion") { TestPropertyListEncoder().testTypeCoercion() }
750797
PropertyListEncoderTests.test("testDecodingConcreteTypeParameter") { TestPropertyListEncoder().testDecodingConcreteTypeParameter() }
798+
PropertyListEncoderTests.test("testEncoderStateThrowOnEncode") { TestPropertyListEncoder().testEncoderStateThrowOnEncode() }
751799
runAllTests()
752800
#endif

0 commit comments

Comments
 (0)