diff --git a/PINRemoteImage.xcodeproj/project.pbxproj b/PINRemoteImage.xcodeproj/project.pbxproj index cac0ba98..9c45470a 100644 --- a/PINRemoteImage.xcodeproj/project.pbxproj +++ b/PINRemoteImage.xcodeproj/project.pbxproj @@ -44,6 +44,8 @@ 139D50B01F672BBF00DE64E0 /* PINRemoteImageDownloadQueue.h in Headers */ = {isa = PBXBuildFile; fileRef = 68B1F27F1E679D7A00ED87C4 /* PINRemoteImageDownloadQueue.h */; }; 139D50B11F672BBF00DE64E0 /* PINResume.h in Headers */ = {isa = PBXBuildFile; fileRef = 68B7E3B01E736C73000FC887 /* PINResume.h */; }; 139D50B21F672BBF00DE64E0 /* PINSpeedRecorder.h in Headers */ = {isa = PBXBuildFile; fileRef = 6860CB061F578287005E886E /* PINSpeedRecorder.h */; }; + 15E2A2683A71757D64EADA89 /* PINProgressiveImage+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */; }; + 2035B2727792667AFEFDC0F2 /* PINRemoteLockTests.m in Sources */ = {isa = PBXBuildFile; fileRef = E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */; }; 440872302BFFC55700C053C2 /* PINRemoteImageDataConvertible.h in Headers */ = {isa = PBXBuildFile; fileRef = 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */; settings = {ATTRIBUTES = (Public, ); }; }; 440872312BFFC55B00C053C2 /* PINRemoteImageDataConvertible.h in Headers */ = {isa = PBXBuildFile; fileRef = 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */; settings = {ATTRIBUTES = (Public, ); }; }; 445C57202BF26B6A0005D70F /* NSData+ImageDetectors.h in Headers */ = {isa = PBXBuildFile; fileRef = 445C570A2BF26B6A0005D70F /* NSData+ImageDetectors.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -153,6 +155,8 @@ B5BB4024274DB69F0042AE1D /* Media.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = B5BB4022274DB69F0042AE1D /* Media.xcassets */; }; D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; }; D93A340324182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; }; + EAFC80CEC3D80C1A6CABE195 /* PINProgressiveImageTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */; }; + F17058DB8820DB9B406074E0 /* PINProgressiveImage+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */; }; F1B918FF1BCF23C900710963 /* PINRemoteImageCategoryManager.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918DC1BCF23C800710963 /* PINRemoteImageCategoryManager.m */; }; F1B919011BCF23C900710963 /* NSData+ImageDetectors.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918DF1BCF23C800710963 /* NSData+ImageDetectors.m */; }; F1B9190F1BCF23C900710963 /* PINProgressiveImage.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918EE1BCF23C800710963 /* PINProgressiveImage.m */; }; @@ -201,8 +205,10 @@ /* Begin PBXFileReference section */ 0CF0743524093139008F126B /* PINAPNGAnimatedImage.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PINAPNGAnimatedImage.m; sourceTree = ""; }; 0E71BE2522E94C7200FC5B99 /* fireworks.gif */ = {isa = PBXFileReference; lastKnownFileType = image.gif; path = fireworks.gif; sourceTree = ""; }; + 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.objc; path = PINProgressiveImageTests.m; sourceTree = ""; }; 139D3A5A1F67284400F82935 /* PINRemoteImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = PINRemoteImage.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 139D3A621F67284400F82935 /* PINRemoteImage-tvOSTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "PINRemoteImage-tvOSTests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; }; + 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = "PINProgressiveImage+Private.h"; sourceTree = ""; }; 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PINRemoteImageDataConvertible.h; sourceTree = ""; }; 4419469E2BF3AAF100E7A59C /* WebP.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = WebP.xcframework; path = webp/WebP.xcframework; sourceTree = ""; }; 441946A12BF3AB6500E7A59C /* WebPDemux.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = WebPDemux.xcframework; path = webp/WebPDemux.xcframework; sourceTree = ""; }; @@ -283,6 +289,7 @@ B5BB4022274DB69F0042AE1D /* Media.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Media.xcassets; sourceTree = ""; }; D93A340024182D46005EB15E /* TestAnimatedImage.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestAnimatedImage.h; sourceTree = ""; }; D93A340124182D46005EB15E /* TestAnimatedImage.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = TestAnimatedImage.m; sourceTree = ""; }; + E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.objc; path = PINRemoteLockTests.m; sourceTree = ""; }; F1B918D11BCF239200710963 /* PINRemoteImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = PINRemoteImage.framework; sourceTree = BUILT_PRODUCTS_DIR; }; F1B918D61BCF239200710963 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; F1B918DC1BCF23C800710963 /* PINRemoteImageCategoryManager.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PINRemoteImageCategoryManager.m; sourceTree = ""; }; @@ -409,6 +416,8 @@ ACD288C6E6B13E6DA226D252 /* NSDate+PINCacheTests.h */, ACD28D963D79EEC14EE071CE /* NSDate+PINCacheTests.m */, B5BB4022274DB69F0042AE1D /* Media.xcassets */, + 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */, + E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */, ); path = Tests; sourceTree = ""; @@ -508,6 +517,7 @@ 68B7E3B11E736C73000FC887 /* PINResume.m */, 6860CB061F578287005E886E /* PINSpeedRecorder.h */, 6860CB071F578287005E886E /* PINSpeedRecorder.m */, + 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */, ); name = Project; path = Classes; @@ -621,6 +631,7 @@ 139D50B21F672BBF00DE64E0 /* PINSpeedRecorder.h in Headers */, 68912D87208FDB4900F5FE0E /* PINRemoteWeakProxy.h in Headers */, 689613E5208FD90B00D2095C /* PINDisplayLink.h in Headers */, + 15E2A2683A71757D64EADA89 /* PINProgressiveImage+Private.h in Headers */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -667,6 +678,7 @@ 445C572C2BF26B6A0005D70F /* PINRemoteImage.h in Headers */, 445C57202BF26B6A0005D70F /* NSData+ImageDetectors.h in Headers */, 445C57222BF26B6A0005D70F /* PINAnimatedImage.h in Headers */, + F17058DB8820DB9B406074E0 /* PINProgressiveImage+Private.h in Headers */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -879,6 +891,8 @@ 683128F51F95045200D5B4A8 /* PINAnimatedImage+PINAnimatedImageTesting.m in Sources */, D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */, ACD28AB87FABF6BA3B9BF4E4 /* NSDate+PINCacheTests.m in Sources */, + EAFC80CEC3D80C1A6CABE195 /* PINProgressiveImageTests.m in Sources */, + 2035B2727792667AFEFDC0F2 /* PINRemoteLockTests.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Source/Classes/PINProgressiveImage+Private.h b/Source/Classes/PINProgressiveImage+Private.h new file mode 100644 index 00000000..b56ba1d8 --- /dev/null +++ b/Source/Classes/PINProgressiveImage+Private.h @@ -0,0 +1,31 @@ +// +// PINProgressiveImage+Private.h +// PINRemoteImage +// +// Created by Vitor Navarro on 30/7/26. +// +// + +#import + +NS_ASSUME_NONNULL_BEGIN + +@interface PINProgressiveImage (Private) + +/** + Returns the current data and relinquishes ownership of it, avoiding a copy. + + This is destructive and deliberately kept out of the public header: it must only be + called once no further data can be appended for this task -- i.e. from the URL + session completion handler, after every -didReceiveData: for the task has already + been delivered synchronously on its serial delegate queue. Calling it from any other + context (e.g. mid-flight, via KVC, or from a debugger) can silently truncate the + buffer fed to an in-progress incremental image decode. + + @return NSData The data accumulated so far. Returns nil on a second call. + */ +- (nullable NSData *)takeData; + +@end + +NS_ASSUME_NONNULL_END diff --git a/Source/Classes/PINProgressiveImage.m b/Source/Classes/PINProgressiveImage.m index ff37e8b9..5e147d8e 100644 --- a/Source/Classes/PINProgressiveImage.m +++ b/Source/Classes/PINProgressiveImage.m @@ -28,6 +28,10 @@ @interface PINProgressiveImage () @property (nonatomic, assign) NSUInteger scannedByte; @property (nonatomic, assign) NSInteger sosCount; @property (nonatomic, strong) PINRemoteLock *lock; +// Set once -takeData has handed off mutableData. Overloads mutableData == nil (which +// otherwise means "nothing received yet") with a second meaning: "already handed off, +// this object is terminal." See the guard at the top of -updateProgressiveImageWithData:. +@property (nonatomic, assign) BOOL dataTaken; #if DEBUG @property (nonatomic, assign) CFTimeInterval scanTime; #endif @@ -133,6 +137,13 @@ - (CFTimeInterval)l_estimatedRemainingTime - (void)updateProgressiveImageWithData:(nonnull NSData *)data expectedNumberOfBytes:(int64_t)expectedNumberOfBytes isResume:(BOOL)isResume { [self.lock lock]; + if (self.dataTaken) { + // Data was already handed off to the completion path (-takeData); this object + // is terminal. Without this guard, a late append would allocate a fresh, short + // buffer here and silently corrupt the incremental image source it fed earlier. + [self.lock unlock]; + return; + } if (isResume) { NSAssert(self.mutableData == nil, @"If we're resuming, data shouldn't be setup yet."); self.startingBytes = data.length; @@ -265,7 +276,47 @@ - (PINImage *)currentImageBlurred:(BOOL)blurred maxProgressiveRenderSize:(CGSize - (NSData *)data { [self.lock lock]; - NSData *data = [self.mutableData copy]; + NSData *data = nil; + if (self.memorySafeDataEnabled) { + @try { + data = [self.mutableData copy]; + } @catch (NSException *exception) { + // Foundation's page allocator raises NSInvalidArgumentException rather than + // returning nil when it cannot satisfy a large allocation. Callers already + // treat nil as "no data" (see PINRemoteImageDownloadTask). + data = nil; + } + } else { + data = [self.mutableData copy]; + } + [self.lock unlock]; + return data; +} + +// Exactly equivalent to `[self data] == nil` today, without materializing a copy. +// NOT the same as `data.length == 0` -- a fresh-but-allocated zero-length buffer makes +// `-data` return non-nil (see PINRemoteImageDownloadTask's retry/emptiness check). +- (BOOL)hasData +{ + [self.lock lock]; + BOOL hasData = (self.mutableData != nil); + [self.lock unlock]; + return hasData; +} + +// Transfers ownership of the accumulated buffer to the caller without copying. +// ONLY safe once no further data can arrive for this task -- i.e. from the URL +// session completion handler, whose per-task delegate queue is serial and has +// already delivered every -didReceiveData: synchronously. +// Returns a dynamically-mutable NSMutableData upcast to NSData: it is uniquely owned +// (we drop our reference) and nothing in-repo mutates it. Foundation offers no way to +// obtain a truly immutable NSData from an NSMutableData without copying. +- (NSData *)takeData +{ + [self.lock lock]; + NSData *data = self.mutableData; + self.mutableData = nil; + self.dataTaken = YES; [self.lock unlock]; return data; } diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m index 3c062b7d..1ae9bb22 100644 --- a/Source/Classes/PINRemoteImageDownloadTask.m +++ b/Source/Classes/PINRemoteImageDownloadTask.m @@ -14,6 +14,7 @@ #import "PINRemoteImageCallbacks.h" #import "PINRemoteLock.h" #import "PINSpeedRecorder.h" +#import "PINProgressiveImage+Private.h" @interface PINRemoteImageDownloadTask () { @@ -311,9 +312,15 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request #endif if (error.code != NSURLErrorCancelled) { - NSData *data = self.progressImage.data; - - if (error == nil && data == nil) { + // Read once so this completion stays on a single path even if the switch flips mid-flight. + BOOL memorySafeData = self.manager.isMemorySafeProgressiveDataEnabled; + // When memory-safe, decide emptiness and retry WITHOUT materializing the buffer -- + // on a retry the copy was discarded, and the allocation itself can fail under memory + // pressure. -hasData preserves the copy path's semantics exactly: + // an allocated-but-zero-length buffer is not "empty" (see PINProgressiveImage). + NSData *data = memorySafeData ? nil : self.progressImage.data; + BOOL isEmpty = memorySafeData ? (self.progressImage.hasData == NO) : (data == nil); + if (error == nil && isEmpty) { error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain code:PINRemoteImageManagerErrorImageEmpty userInfo:nil]; @@ -338,10 +345,26 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request return; } + if (memorySafeData) { + // Terminal: no further data can arrive, so hand off the buffer instead of + // copying it. -takeData allocates nothing, so it cannot throw under memory + // pressure -- this is what actually removes the doubling. + data = [self.progressImage takeData]; + if (error == nil && data == nil) { + // Load-bearing: with -hasData gating the retry decision above, this only + // fires if -progressImage itself is nil, keeping completionHandler from + // ever seeing (nil, response, nil). + error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain + code:PINRemoteImageManagerErrorImageEmpty + userInfo:nil]; + } + } + completionHandler(data, response, error); } }]; }]]; + self->_progressImage.memorySafeDataEnabled = self.manager.isMemorySafeProgressiveDataEnabled; }]; } diff --git a/Source/Classes/PINRemoteImageManager+Private.h b/Source/Classes/PINRemoteImageManager+Private.h index 39550e32..0cd7b911 100644 --- a/Source/Classes/PINRemoteImageManager+Private.h +++ b/Source/Classes/PINRemoteImageManager+Private.h @@ -23,6 +23,7 @@ typedef void (^PINRemoteImageManagerDataCompletion)(NSData *data, NSURLResponse @property (nonatomic, readonly) NSArray *progressThresholds; @property (nonatomic, readonly) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, readonly) BOOL shouldBlurProgressive; +@property (nonatomic, readonly) BOOL isMemorySafeProgressiveDataEnabled; @property (nonatomic, readonly) CGSize maxProgressiveRenderSize; @end diff --git a/Source/Classes/PINRemoteImageManager.m b/Source/Classes/PINRemoteImageManager.m index aa48d0da..13e7abca 100644 --- a/Source/Classes/PINRemoteImageManager.m +++ b/Source/Classes/PINRemoteImageManager.m @@ -140,6 +140,7 @@ @interface PINRemoteImageManager () @property (nonatomic, strong) NSMapTable *UUIDToTask; @property (nonatomic, strong) NSArray *progressThresholds; @property (nonatomic, assign) BOOL shouldBlurProgressive; +@property (nonatomic, assign) BOOL isMemorySafeProgressiveDataEnabled; @property (nonatomic, assign) CGSize maxProgressiveRenderSize; @property (nonatomic, assign) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, strong) dispatch_queue_t callbackQueue; @@ -262,6 +263,7 @@ -(nonnull instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration * _shouldUpgradeLowQualityImages = configuration.shouldUpgradeLowQualityImages; _shouldBlurProgressive = configuration.shouldBlurProgressive; _maxProgressiveRenderSize = configuration.maxProgressiveRenderSize; + _isMemorySafeProgressiveDataEnabled = configuration.isMemorySafeProgressiveDataEnabled; self.tasks = [[NSMutableDictionary alloc] init]; self.canceledTasks = [[NSHashTable alloc] initWithOptions:NSHashTableWeakMemory capacity:5]; self.UUIDToTask = [NSMapTable weakToWeakObjectsMapTable]; diff --git a/Source/Classes/PINRemoteImageManagerConfiguration.m b/Source/Classes/PINRemoteImageManagerConfiguration.m index 88896fd0..5289add1 100644 --- a/Source/Classes/PINRemoteImageManagerConfiguration.m +++ b/Source/Classes/PINRemoteImageManagerConfiguration.m @@ -21,6 +21,7 @@ - (nonnull instancetype)init { _highQualityBPSThreshold = 500000; _lowQualityBPSThreshold = 50000; // approximately edge speed _shouldUpgradeLowQualityImages = NO; + _isMemorySafeProgressiveDataEnabled = NO; } return self; } diff --git a/Source/Classes/PINRemoteLock.m b/Source/Classes/PINRemoteLock.m index 6cf74d62..830649c6 100644 --- a/Source/Classes/PINRemoteLock.m +++ b/Source/Classes/PINRemoteLock.m @@ -80,13 +80,16 @@ - (void)lockWithBlock:(dispatch_block_t)block #else pthread_mutex_lock(&_lock); #endif - block(); + @try { + block(); + } @finally { #if PINREMOTELOCK_DEBUG - [_lock unlock]; - [_recursiveLock unlock]; + [_lock unlock]; + [_recursiveLock unlock]; #else - pthread_mutex_unlock(&_lock); + pthread_mutex_unlock(&_lock); #endif + } } - (void)lock diff --git a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h index e5e57b46..b9128c83 100644 --- a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h +++ b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h @@ -25,6 +25,8 @@ @property (atomic, assign) CFTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, strong, readonly, nonnull) NSURLSessionDataTask * dataTask; @property (nonatomic, readonly) CFTimeInterval estimatedRemainingTime; +/** When YES, -data returns nil instead of raising if copying the buffer fails. Defaults to NO. */ +@property (atomic, assign, getter=isMemorySafeDataEnabled) BOOL memorySafeDataEnabled; - (nonnull instancetype)init NS_UNAVAILABLE; - (nonnull instancetype)initWithDataTask:(nonnull NSURLSessionDataTask *)dataTask; @@ -49,4 +51,7 @@ */ - (nullable NSData *)data; +/// YES if any data has been received. Equivalent to `[self data] != nil`, without copying. +@property (nonatomic, readonly) BOOL hasData; + @end diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h index 20b0746c..528bbec1 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h @@ -43,4 +43,13 @@ /** Whether high quality images should be downloaded when a low quality image is cached if network connectivity has improved. */ @property (nonatomic, readwrite, assign) BOOL shouldUpgradeLowQualityImages; +/** + Opt-in switch for memory-safe handling of downloaded image data. Defaults to NO. + + When YES, a finished download hands its accumulated buffer to the completion path instead of copying it, and + -[PINProgressiveImage data] returns nil instead of raising when Foundation fails to allocate the copy under + memory pressure. + */ +@property (nonatomic, readwrite, assign) BOOL isMemorySafeProgressiveDataEnabled; + @end diff --git a/Tests/PINProgressiveImageTests.m b/Tests/PINProgressiveImageTests.m new file mode 100644 index 00000000..93729c4d --- /dev/null +++ b/Tests/PINProgressiveImageTests.m @@ -0,0 +1,182 @@ +// +// PINProgressiveImageTests.m +// PINRemoteImage Tests +// +// Tests for the memory-safety fix to PINProgressiveImage's data hand-off: +// - `-data` must survive an allocation failure by returning nil instead of +// letting NSInvalidArgumentException escape (Bugsnag 67fad806301606795e34c849). +// - `-hasData` must answer without copying the buffer. +// - `-takeData` must transfer ownership without allocating, and must be +// guarded so a late append after hand-off cannot silently corrupt state. +// + +#import +#import + +#import +#import +#import "PINProgressiveImage+Private.h" + +/// Dynamically subclasses `object`'s class so that `-copy`/`-copyWithZone:` throw, +/// mimicking Foundation's page allocator raising NSInvalidArgumentException instead +/// of returning nil for a large allocation it can't satisfy. Scoped to the single +/// instance via isa-swizzling so it can't affect any other NSMutableData in the process. +static void PINTestMakeCopyThrowOnInstance(NSObject *object) +{ + Class originalClass = object_getClass(object); + NSString *name = [NSString stringWithFormat:@"PINTestThrowingCopy_%@_%p", NSStringFromClass(originalClass), object]; + Class throwingClass = objc_allocateClassPair(originalClass, name.UTF8String, 0); + + IMP throwingCopyWithZoneIMP = imp_implementationWithBlock(^id(id _self, NSZone *zone) { + @throw [NSException exceptionWithName:NSInvalidArgumentException + reason:@"*** NSAllocateMemoryPages(N) failed" + userInfo:nil]; + }); + IMP throwingCopyIMP = imp_implementationWithBlock(^id(id _self) { + @throw [NSException exceptionWithName:NSInvalidArgumentException + reason:@"*** NSAllocateMemoryPages(N) failed" + userInfo:nil]; + }); + class_addMethod(throwingClass, @selector(copyWithZone:), throwingCopyWithZoneIMP, "@@:^{_NSZone=}"); + class_addMethod(throwingClass, @selector(copy), throwingCopyIMP, "@@:"); + + objc_registerClassPair(throwingClass); + object_setClass(object, throwingClass); +} + +@interface PINProgressiveImageTests : XCTestCase + +@end + +@implementation PINProgressiveImageTests + +- (PINProgressiveImage *)freshProgressiveImage +{ + NSURLSessionDataTask *dataTask = [[NSURLSession sharedSession] dataTaskWithURL:[NSURL URLWithString:@"https://example.com/image.jpg"]]; + PINProgressiveImage *progressiveImage = [[PINProgressiveImage alloc] initWithDataTask:dataTask]; + progressiveImage.memorySafeDataEnabled = YES; + return progressiveImage; +} + +#pragma mark - hasData + +- (void)testHasDataIsFalseBeforeAnyDataArrives +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + XCTAssertFalse(progressiveImage.hasData); + XCTAssertNil(progressiveImage.data); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); +} + +- (void)testHasDataIsTrueAfterDataArrives +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"hello" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + XCTAssertTrue(progressiveImage.hasData); + XCTAssertEqualObjects(progressiveImage.data, chunk); + XCTAssertEqual(progressiveImage.data.length, chunk.length); +} + +// Regression guard: an allocated-but-empty buffer must NOT be treated as "no data". +// This is the one case where `data.length == 0` and `hasData == NO` disagree; the +// retry/emptiness check in PINRemoteImageDownloadTask must use -hasData, not +// data.length, or a zero-byte `didReceiveData:` would newly become ErrorImageEmpty. +- (void)testHasDataIsTrueForAllocatedZeroLengthBuffer +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + [progressiveImage updateProgressiveImageWithData:[NSData data] expectedNumberOfBytes:0 isResume:NO]; + + XCTAssertTrue(progressiveImage.hasData, @"An allocated (even if zero-length) buffer must count as having data, matching today's [mutableData copy] != nil behavior."); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); + XCTAssertNotNil(progressiveImage.data); +} + +#pragma mark - takeData + +- (void)testTakeDataReturnsFullBufferOnceThenNil +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"the quick brown fox" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSData *taken = [progressiveImage takeData]; + XCTAssertEqualObjects(taken, chunk); + + XCTAssertNil([progressiveImage takeData], @"A second -takeData call must return nil, not the same buffer again."); + XCTAssertFalse(progressiveImage.hasData, @"After -takeData, the object is terminal: hasData must be NO."); + XCTAssertNil(progressiveImage.data, @"After -takeData, -data must be NO, not re-copy the (now nil'd) buffer."); +} + +- (void)testTakeDataOnEmptyProgressiveImageReturnsNil +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + XCTAssertNil([progressiveImage takeData]); +} + +// The mandatory companion to -takeData: mutableData == nil is an overloaded sentinel +// that means both "nothing received yet" and (post-fix) "already handed off". Without +// the dataTaken guard, a late append after -takeData would allocate a fresh, short +// buffer and silently feed it to CGImageSourceUpdateData, corrupting the decode instead +// of crashing. This test proves the guard makes updateProgressiveImageWithData: a no-op +// once data has been taken. +- (void)testLateAppendAfterTakeDataIsIgnored +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *firstChunk = [@"first" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:firstChunk expectedNumberOfBytes:(int64_t)firstChunk.length isResume:NO]; + + NSData *taken = [progressiveImage takeData]; + XCTAssertEqualObjects(taken, firstChunk); + + // Simulate a late in-flight `didReceiveData:` racing the completion handler. + NSData *lateChunk = [@"late-arriving-bytes" dataUsingEncoding:NSUTF8StringEncoding]; + XCTAssertNoThrow([progressiveImage updateProgressiveImageWithData:lateChunk expectedNumberOfBytes:(int64_t)lateChunk.length isResume:NO]); + + XCTAssertFalse(progressiveImage.hasData, @"A late append after -takeData must be dropped, not silently start a new, short buffer."); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); + XCTAssertNil(progressiveImage.data); +} + +#pragma mark - -data survives allocation failure + +- (void)testDataReturnsNilInsteadOfThrowingWhenCopyFails +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"payload" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSMutableData *mutableData = [progressiveImage valueForKey:@"mutableData"]; + XCTAssertNotNil(mutableData); + PINTestMakeCopyThrowOnInstance(mutableData); + + __block NSData *data; + XCTAssertNoThrow(data = progressiveImage.data, @"An allocation failure inside -copy must not escape -data as an exception."); + XCTAssertNil(data); + + // The lock must not be left held: a subsequent call through the same lock must + // not deadlock. (Regression coverage for the mutex-leak fix lives in + // PINRemoteLockTests; this just confirms -data's own explicit lock/unlock pairs + // correctly around the @try/@catch.) + XCTAssertTrue(progressiveImage.hasData, @"hasData does not copy, so it must still reflect the buffer's presence even though -data failed."); +} + +- (void)testDataStillThrowsWhenCopyFailsAndMemorySafeDataIsDisabled +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + progressiveImage.memorySafeDataEnabled = NO; + NSData *chunk = [@"payload" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSMutableData *mutableData = [progressiveImage valueForKey:@"mutableData"]; + XCTAssertNotNil(mutableData); + PINTestMakeCopyThrowOnInstance(mutableData); + + // The legacy path leaves -data's lock held after the throw, and -dealloc takes that lock, + // so the object is intentionally leaked to keep the test from deadlocking. + CFBridgingRetain(progressiveImage); + XCTAssertThrowsSpecificNamed(progressiveImage.data, NSException, NSInvalidArgumentException); +} + +@end diff --git a/Tests/PINRemoteImageTests.m b/Tests/PINRemoteImageTests.m index b90494cd..1818fdc6 100644 --- a/Tests/PINRemoteImageTests.m +++ b/Tests/PINRemoteImageTests.m @@ -205,7 +205,7 @@ - (NSURL *)grayscalePNGURL - (NSURL *)veryLongURL { - return [NSURL URLWithString:@"https://loremflickr.com/200/301?longarg=helloMomHowAreYouDoing.IamFineJustMovedToLiveWithANiceChapWeTravelTogetherInHisBlueBoxThroughSpaceAndTimeMaybeYouveMetHimAlready.YesterdayWeMetACultureOfPeopleWithTentaclesWhoSingWithAVeryCelestialVoice.SoGood.SeeYouSoon.MaybeYesterday.WhoKnows.XOXO"]; + return [NSURL URLWithString:@"https://via.placeholder.com/200x301?text=helloMomHowAreYouDoing.IamFineJustMovedToLiveWithANiceChapWeTravelTogetherInHisBlueBoxThroughSpaceAndTimeMaybeYouveMetHimAlready.YesterdayWeMetACultureOfPeopleWithTentaclesWhoSingWithAVeryCelestialVoice.SoGood.SeeYouSoon.MaybeYesterday.WhoKnows.XOXO"]; } - (NSURL *)progressiveURL diff --git a/Tests/PINRemoteLockTests.m b/Tests/PINRemoteLockTests.m new file mode 100644 index 00000000..16eca9df --- /dev/null +++ b/Tests/PINRemoteLockTests.m @@ -0,0 +1,63 @@ +// +// PINRemoteLockTests.m +// PINRemoteImage Tests +// +// Regression coverage for the mutex leak fixed in PINRemoteLock: before the fix, +// an exception escaping the block passed to -lockWithBlock: skipped the matching +// unlock, leaving the lock permanently held. That single bug meant every one of +// PINProgressiveImage/PINRemoteImageDownloadTask's ~40 `lockWithBlock:` call sites +// was one exception away from a permanent deadlock, not just the OOM copy site. +// + +#import +#import "PINRemoteLock.h" + +@interface PINRemoteLockTests : XCTestCase + +@end + +@implementation PINRemoteLockTests + +- (void)testLockWithBlockReleasesTheLockWhenTheBlockThrows +{ + PINRemoteLock *lock = [[PINRemoteLock alloc] initWithName:@"PINRemoteLockTests"]; + + XCTAssertThrowsSpecificNamed({ + [lock lockWithBlock:^{ + @throw [NSException exceptionWithName:@"PINRemoteLockTestException" reason:@"boom" userInfo:nil]; + }]; + }, NSException, @"PINRemoteLockTestException"); + + // If -lockWithBlock: leaked the lock on the exception above, this second + // acquisition (deliberately from another thread, since the lock is + // non-recursive) would hang forever instead of completing. + XCTestExpectation *reacquired = [self expectationWithDescription:@"lock reacquired after throwing block"]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + [lock lockWithBlock:^{ + [reacquired fulfill]; + }]; + }); + + [self waitForExpectationsWithTimeout:5.0 handler:^(NSError * _Nullable error) { + XCTAssertNil(error, @"lockWithBlock: appears to have leaked the mutex after an exception escaped the block."); + }]; +} + +- (void)testLockWithBlockStillRunsAndUnlocksOnTheHappyPath +{ + PINRemoteLock *lock = [[PINRemoteLock alloc] initWithName:@"PINRemoteLockTests"]; + + __block BOOL ran = NO; + [lock lockWithBlock:^{ + ran = YES; + }]; + XCTAssertTrue(ran); + + XCTestExpectation *reacquired = [self expectationWithDescription:@"lock reacquired on happy path"]; + [lock lockWithBlock:^{ + [reacquired fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 handler:nil]; +} + +@end