Skip to content

Commit e5fdc09

Browse files
itaiferberitaiferber
authored
Merge pull request #13879 from itaiferber/jsonencoder-defer-container-pops
{JSON,Plist}{En,De}coder defer container pops
2 parents 551701a + 1e110b8 commit e5fdc09

File tree

4 files changed

+274
-38
lines changed

4 files changed

+274
-38
lines changed

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

+58-28
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 {
@@ -2237,9 +2277,8 @@ extension _JSONDecoder {
22372277
switch self.options.dateDecodingStrategy {
22382278
case .deferredToDate:
22392279
self.storage.push(container: value)
2240-
let date = try Date(from: self)
2241-
self.storage.popContainer()
2242-
return date
2280+
defer { self.storage.popContainer() }
2281+
return try Date(from: self)
22432282

22442283
case .secondsSince1970:
22452284
let double = try self.unbox(value, as: Double.self)!
@@ -2271,9 +2310,8 @@ extension _JSONDecoder {
22712310

22722311
case .custom(let closure):
22732312
self.storage.push(container: value)
2274-
let date = try closure(self)
2275-
self.storage.popContainer()
2276-
return date
2313+
defer { self.storage.popContainer() }
2314+
return try closure(self)
22772315
}
22782316
}
22792317

@@ -2283,9 +2321,8 @@ extension _JSONDecoder {
22832321
switch self.options.dataDecodingStrategy {
22842322
case .deferredToData:
22852323
self.storage.push(container: value)
2286-
let data = try Data(from: self)
2287-
self.storage.popContainer()
2288-
return data
2324+
defer { self.storage.popContainer() }
2325+
return try Data(from: self)
22892326

22902327
case .base64:
22912328
guard let string = value as? String else {
@@ -2300,9 +2337,8 @@ extension _JSONDecoder {
23002337

23012338
case .custom(let closure):
23022339
self.storage.push(container: value)
2303-
let data = try closure(self)
2304-
self.storage.popContainer()
2305-
return data
2340+
defer { self.storage.popContainer() }
2341+
return try closure(self)
23062342
}
23072343
}
23082344

@@ -2319,13 +2355,10 @@ extension _JSONDecoder {
23192355
}
23202356

23212357
fileprivate func unbox<T : Decodable>(_ value: Any, as type: T.Type) throws -> T? {
2322-
let decoded: T
23232358
if type == Date.self || type == NSDate.self {
2324-
guard let date = try self.unbox(value, as: Date.self) else { return nil }
2325-
decoded = date as! T
2359+
return try self.unbox(value, as: Date.self) as? T
23262360
} else if type == Data.self || type == NSData.self {
2327-
guard let data = try self.unbox(value, as: Data.self) else { return nil }
2328-
decoded = data as! T
2361+
return try self.unbox(value, as: Data.self) as? T
23292362
} else if type == URL.self || type == NSURL.self {
23302363
guard let urlString = try self.unbox(value, as: String.self) else {
23312364
return nil
@@ -2336,17 +2369,14 @@ extension _JSONDecoder {
23362369
debugDescription: "Invalid URL string."))
23372370
}
23382371

2339-
decoded = (url as! T)
2372+
return url as! T
23402373
} else if type == Decimal.self || type == NSDecimalNumber.self {
2341-
guard let decimal = try self.unbox(value, as: Decimal.self) else { return nil }
2342-
decoded = decimal as! T
2374+
return try self.unbox(value, as: Decimal.self) as? T
23432375
} else {
23442376
self.storage.push(container: value)
2345-
decoded = try type.init(from: self)
2346-
self.storage.popContainer()
2377+
defer { self.storage.popContainer() }
2378+
return try type.init(from: self)
23472379
}
2348-
2349-
return decoded
23502380
}
23512381
}
23522382

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

+14-10
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 {
@@ -1755,20 +1764,15 @@ extension _PlistDecoder {
17551764
}
17561765

17571766
fileprivate func unbox<T : Decodable>(_ value: Any, as type: T.Type) throws -> T? {
1758-
let decoded: T
17591767
if type == Date.self || type == NSDate.self {
1760-
guard let date = try self.unbox(value, as: Date.self) else { return nil }
1761-
decoded = date as! T
1768+
return try self.unbox(value, as: Date.self) as? T
17621769
} else if type == Data.self || type == NSData.self {
1763-
guard let data = try self.unbox(value, as: Data.self) else { return nil }
1764-
decoded = data as! T
1770+
return try self.unbox(value, as: Data.self) as? T
17651771
} else {
17661772
self.storage.push(container: value)
1767-
decoded = try type.init(from: self)
1768-
self.storage.popContainer()
1773+
defer { self.storage.popContainer() }
1774+
return try type.init(from: self)
17691775
}
1770-
1771-
return decoded
17721776
}
17731777
}
17741778

‎test/stdlib/TestJSONEncoder.swift

+133
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,119 @@ 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+
881+
// MARK: - Decoder State
882+
// SR-6048
883+
func testDecoderStateThrowOnDecode() {
884+
// The container stack here starts as [[1,2,3]]. Attempting to decode as [String] matches the outer layer (Array), and begins decoding the array.
885+
// Once Array decoding begins, 1 is pushed onto the container stack ([[1,2,3], 1]), and 1 is attempted to be decoded as String. This throws a .typeMismatch, but the container is not popped off the stack.
886+
// When attempting to decode [Int], the container stack is still ([[1,2,3], 1]), and 1 fails to decode as [Int].
887+
let json = "[1,2,3]".data(using: .utf8)!
888+
let _ = try! JSONDecoder().decode(EitherDecodable<[String], [Int]>.self, from: json)
889+
}
890+
891+
func testDecoderStateThrowOnDecodeCustomDate() {
892+
// This test is identical to testDecoderStateThrowOnDecode, except we're going to fail because our closure throws an error, not because we hit a type mismatch.
893+
let decoder = JSONDecoder()
894+
decoder.dateDecodingStrategy = .custom({ decoder in
895+
enum CustomError : Error { case foo }
896+
throw CustomError.foo
897+
})
898+
899+
let json = "{\"value\": 1}".data(using: .utf8)!
900+
let _ = try! decoder.decode(EitherDecodable<TopLevelWrapper<Date>, TopLevelWrapper<Int>>.self, from: json)
901+
}
902+
903+
func testDecoderStateThrowOnDecodeCustomData() {
904+
// This test is identical to testDecoderStateThrowOnDecode, except we're going to fail because our closure throws an error, not because we hit a type mismatch.
905+
let decoder = JSONDecoder()
906+
decoder.dataDecodingStrategy = .custom({ decoder in
907+
enum CustomError : Error { case foo }
908+
throw CustomError.foo
909+
})
910+
911+
let json = "{\"value\": 1}".data(using: .utf8)!
912+
let _ = try! decoder.decode(EitherDecodable<TopLevelWrapper<Data>, TopLevelWrapper<Int>>.self, from: json)
913+
}
914+
802915
// MARK: - Helper Functions
803916
private var _jsonEmptyDictionary: Data {
804917
return "{}".data(using: .utf8)!
@@ -1391,6 +1504,20 @@ fileprivate struct DoubleNaNPlaceholder : Codable, Equatable {
13911504
}
13921505
}
13931506

1507+
fileprivate enum EitherDecodable<T : Decodable, U : Decodable> : Decodable {
1508+
case t(T)
1509+
case u(U)
1510+
1511+
init(from decoder: Decoder) throws {
1512+
let container = try decoder.singleValueContainer()
1513+
do {
1514+
self = .t(try container.decode(T.self))
1515+
} catch {
1516+
self = .u(try container.decode(U.self))
1517+
}
1518+
}
1519+
}
1520+
13941521
// MARK: - Run Tests
13951522

13961523
#if !FOUNDATION_XCTEST
@@ -1439,5 +1566,11 @@ JSONEncoderTests.test("testInterceptDecimal") { TestJSONEncoder().testInterceptD
14391566
JSONEncoderTests.test("testInterceptURL") { TestJSONEncoder().testInterceptURL() }
14401567
JSONEncoderTests.test("testTypeCoercion") { TestJSONEncoder().testTypeCoercion() }
14411568
JSONEncoderTests.test("testDecodingConcreteTypeParameter") { TestJSONEncoder().testDecodingConcreteTypeParameter() }
1569+
JSONEncoderTests.test("testEncoderStateThrowOnEncode") { TestJSONEncoder().testEncoderStateThrowOnEncode() }
1570+
JSONEncoderTests.test("testEncoderStateThrowOnEncodeCustomDate") { TestJSONEncoder().testEncoderStateThrowOnEncodeCustomDate() }
1571+
JSONEncoderTests.test("testEncoderStateThrowOnEncodeCustomData") { TestJSONEncoder().testEncoderStateThrowOnEncodeCustomData() }
1572+
JSONEncoderTests.test("testDecoderStateThrowOnDecode") { TestJSONEncoder().testDecoderStateThrowOnDecode() }
1573+
JSONEncoderTests.test("testDecoderStateThrowOnDecodeCustomDate") { TestJSONEncoder().testDecoderStateThrowOnDecodeCustomDate() }
1574+
JSONEncoderTests.test("testDecoderStateThrowOnDecodeCustomData") { TestJSONEncoder().testDecoderStateThrowOnDecodeCustomData() }
14421575
runAllTests()
14431576
#endif

0 commit comments

Comments
 (0)