From 347cf1d1cc4feed24971e78dd68763fb132c1950 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 26 Sep 2022 17:32:25 +0800 Subject: [PATCH 1/4] Fix the potential out of bounds crash for ImageIO incremental animation decoding (like GIF) This patch from the SDWebImageWebPCoder/pull/68 --- SDWebImage/Core/SDImageIOAnimatedCoder.m | 59 +++++++++++++++++++++--- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/SDWebImage/Core/SDImageIOAnimatedCoder.m b/SDWebImage/Core/SDImageIOAnimatedCoder.m index 56b8af8d..247b3eac 100644 --- a/SDWebImage/Core/SDImageIOAnimatedCoder.m +++ b/SDWebImage/Core/SDImageIOAnimatedCoder.m @@ -13,6 +13,7 @@ #import "SDImageCoderHelper.h" #import "SDAnimatedImageRep.h" #import "UIImage+ForceDecode.h" +#import "SDInternalMacros.h" // Specify DPI for vector format in CGImageSource, like PDF static NSString * kSDCGImageSourceRasterizationDPI = @"kCGImageSourceRasterizationDPI"; @@ -32,6 +33,8 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination @implementation SDImageIOAnimatedCoder { size_t _width, _height; CGImageSourceRef _imageSource; + BOOL _incremental; + SD_LOCK_DECLARE(_lock); // Lock only apply for incremental animation decoding NSData *_imageData; CGFloat _scale; NSUInteger _loopCount; @@ -364,6 +367,7 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination if (self) { NSString *imageUTType = self.class.imageUTType; _imageSource = CGImageSourceCreateIncremental((__bridge CFDictionaryRef)@{(__bridge NSString *)kCGImageSourceTypeIdentifierHint : imageUTType}); + _incremental = YES; CGFloat scale = 1; NSNumber *scaleFactor = options[SDImageCoderDecodeScaleFactor]; if (scaleFactor != nil) { @@ -386,6 +390,7 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination preserveAspectRatio = preserveAspectRatioValue.boolValue; } _preserveAspectRatio = preserveAspectRatio; + SD_LOCK_INIT(_lock); #if SD_UIKIT [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didReceiveMemoryWarning:) name:UIApplicationDidReceiveMemoryWarningNotification object:nil]; #endif @@ -394,6 +399,7 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination } - (void)updateIncrementalData:(NSData *)data finished:(BOOL)finished { + NSCParameterAssert(_incremental); if (_finished) { return; } @@ -421,11 +427,14 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination } } + SD_LOCK(_lock); // For animated image progressive decoding because the frame count and duration may be changed. [self scanAndCheckFramesValidWithImageSource:_imageSource]; + SD_UNLOCK(_lock); } - (UIImage *)incrementalDecodedImageWithOptions:(SDImageCoderOptions *)options { + NSCParameterAssert(_incremental); UIImage *image; if (_width + _height > 0) { @@ -606,17 +615,21 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination } NSUInteger frameCount = CGImageSourceGetCount(imageSource); NSUInteger loopCount = [self.class imageLoopCountWithSource:imageSource]; - NSMutableArray *frames = [NSMutableArray array]; + _loopCount = loopCount; + NSMutableArray *frames = [NSMutableArray arrayWithCapacity:frameCount]; for (size_t i = 0; i < frameCount; i++) { SDImageIOCoderFrame *frame = [[SDImageIOCoderFrame alloc] init]; frame.index = i; frame.duration = [self.class frameDurationAtIndex:i source:imageSource]; [frames addObject:frame]; } + if (frames.count != frameCount) { + // frames not match, do not override current value + return NO; + } _frameCount = frameCount; - _loopCount = loopCount; _frames = [frames copy]; return YES; @@ -635,16 +648,48 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination } - (NSTimeInterval)animatedImageDurationAtIndex:(NSUInteger)index { - if (index >= _frameCount) { - return 0; + NSTimeInterval duration; + // Incremental Animation decoding may update frames when new bytes available + // Which should use lock to ensure frame count and frames match, ensure atomic logic + if (_incremental) { + SD_LOCK(_lock); + if (index >= _frames.count) { + SD_UNLOCK(_lock); + return 0; + } + duration = _frames[index].duration; + SD_UNLOCK(_lock); + } else { + if (index >= _frames.count) { + return 0; + } + duration = _frames[index].duration; } - return _frames[index].duration; + return duration; } - (UIImage *)animatedImageFrameAtIndex:(NSUInteger)index { - if (index >= _frameCount) { - return nil; + UIImage *image; + // Incremental Animation decoding may update frames when new bytes available + // Which should use lock to ensure frame count and frames match, ensure atomic logic + if (_incremental) { + SD_LOCK(_lock); + if (index >= _frames.count) { + SD_UNLOCK(_lock); + return nil; + } + image = [self safeAnimatedImageFrameAtIndex:index]; + SD_UNLOCK(_lock); + } else { + if (index >= _frames.count) { + return nil; + } + image = [self safeAnimatedImageFrameAtIndex:index]; } + return image; +} + +- (UIImage *)safeAnimatedImageFrameAtIndex:(NSUInteger)index { NSDictionary *options; BOOL forceDecode = NO; if (@available(iOS 15, tvOS 15, *)) { From 9b2ddc9ea84434741c1491bce6440f80d86612fa Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 26 Sep 2022 17:34:55 +0800 Subject: [PATCH 2/4] [NFC] A little optimization for MutableArray creation --- SDWebImage/Core/SDImageCoderHelper.m | 4 +++- SDWebImage/Core/SDImageIOAnimatedCoder.m | 2 +- SDWebImage/Core/SDWebImageDefine.m | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/SDWebImage/Core/SDImageCoderHelper.m b/SDWebImage/Core/SDImageCoderHelper.m index da30b65a..8e89407a 100644 --- a/SDWebImage/Core/SDImageCoderHelper.m +++ b/SDWebImage/Core/SDImageCoderHelper.m @@ -173,7 +173,7 @@ static const CGFloat kDestSeemOverlap = 2.0f; // the numbers of pixels to over return nil; } - NSMutableArray *frames = [NSMutableArray array]; + NSMutableArray *frames; NSUInteger frameCount = 0; #if SD_UIKIT || SD_WATCH @@ -182,6 +182,7 @@ static const CGFloat kDestSeemOverlap = 2.0f; // the numbers of pixels to over if (frameCount == 0) { return nil; } + frames = [NSMutableArray arrayWithCapacity:frameCount]; NSTimeInterval avgDuration = animatedImage.duration / frameCount; if (avgDuration == 0) { @@ -223,6 +224,7 @@ static const CGFloat kDestSeemOverlap = 2.0f; // the numbers of pixels to over if (frameCount == 0) { return nil; } + frames = [NSMutableArray arrayWithCapacity:frameCount]; CGFloat scale = animatedImage.scale; for (size_t i = 0; i < frameCount; i++) { diff --git a/SDWebImage/Core/SDImageIOAnimatedCoder.m b/SDWebImage/Core/SDImageIOAnimatedCoder.m index 247b3eac..74a724ae 100644 --- a/SDWebImage/Core/SDImageIOAnimatedCoder.m +++ b/SDWebImage/Core/SDImageIOAnimatedCoder.m @@ -331,7 +331,7 @@ static NSString * kSDCGImageDestinationRequestedFileSize = @"kCGImageDestination if (decodeFirstFrame || count <= 1) { animatedImage = [self.class createFrameAtIndex:0 source:source scale:scale preserveAspectRatio:preserveAspectRatio thumbnailSize:thumbnailSize forceDecode:NO options:nil]; } else { - NSMutableArray *frames = [NSMutableArray array]; + NSMutableArray *frames = [NSMutableArray arrayWithCapacity:count]; for (size_t i = 0; i < count; i++) { UIImage *image = [self.class createFrameAtIndex:i source:source scale:scale preserveAspectRatio:preserveAspectRatio thumbnailSize:thumbnailSize forceDecode:NO options:nil]; diff --git a/SDWebImage/Core/SDWebImageDefine.m b/SDWebImage/Core/SDWebImageDefine.m index aee8ca4d..8fb2de44 100644 --- a/SDWebImage/Core/SDWebImageDefine.m +++ b/SDWebImage/Core/SDWebImageDefine.m @@ -85,9 +85,10 @@ inline UIImage * _Nullable SDScaledImageForScaleFactor(CGFloat scale, UIImage * UIImage *animatedImage; #if SD_UIKIT || SD_WATCH // `UIAnimatedImage` images share the same size and scale. - NSMutableArray *scaledImages = [NSMutableArray array]; + NSArray *images = image.images; + NSMutableArray *scaledImages = [NSMutableArray arrayWithCapacity:images.count]; - for (UIImage *tempImage in image.images) { + for (UIImage *tempImage in images) { UIImage *tempScaledImage = [[UIImage alloc] initWithCGImage:tempImage.CGImage scale:scale orientation:tempImage.imageOrientation]; [scaledImages addObject:tempScaledImage]; } From 080db1afb916e68edbe5e7c3089fbfaae58bf831 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 26 Sep 2022 18:24:41 +0800 Subject: [PATCH 3/4] Try to investigate test case failure issue --- .github/workflows/CI.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index fd4bed8c..0707d592 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -126,19 +126,19 @@ jobs: - name: Test - ${{ matrix.iosDestination }} run: | set -o pipefail - xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests iOS" -destination "${{ matrix.iosDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO | xcpretty -c + xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests iOS" -destination "${{ matrix.iosDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO mv ~/Library/Developer/Xcode/DerivedData/ ./DerivedData/iOS - name: Test - ${{ matrix.macOSDestination }} run: | set -o pipefail - xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests Mac" -destination "${{ matrix.macOSDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO | xcpretty -c + xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests Mac" -destination "${{ matrix.macOSDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO mv ~/Library/Developer/Xcode/DerivedData/ ./DerivedData/macOS - name: Test - ${{ matrix.tvOSDestination }} run: | set -o pipefail - xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests TV" -destination "${{ matrix.tvOSDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO | xcpretty -c + xcodebuild test -workspace "${{ env.WORKSPACE_NAME }}" -scheme "Tests TV" -destination "${{ matrix.tvOSDestination }}" -configuration Debug CODE_SIGNING_ALLOWED=NO mv ~/Library/Developer/Xcode/DerivedData/ ./DerivedData/tvOS - name: Code Coverage From 364034d11aaa6415e8611faaa1739e24c6fb0383 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 26 Sep 2022 18:43:26 +0800 Subject: [PATCH 4/4] Fix test cases `test15CancelQueryShouldCallbackOnceInSync` --- Tests/Tests/SDImageCacheTests.m | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Tests/Tests/SDImageCacheTests.m b/Tests/Tests/SDImageCacheTests.m index cb4cf086..fcc571db 100644 --- a/Tests/Tests/SDImageCacheTests.m +++ b/Tests/Tests/SDImageCacheTests.m @@ -217,9 +217,11 @@ static NSString *kTestImageKeyPNG = @"TestImageKey.png"; __block BOOL callced = NO; SDImageCacheToken *token = [SDImageCache.sharedImageCache queryCacheOperationForKey:key done:^(UIImage * _Nullable image, NSData * _Nullable data, SDImageCacheType cacheType) { callced = true; - [expectation fulfill]; // callback once fulfill once + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0), dispatch_get_main_queue(), ^{ + [expectation fulfill]; // callback once fulfill once + }); }]; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0), dispatch_get_main_queue(), ^{ expect(callced).beFalsy(); [token cancel]; // sync expect(callced).beTruthy();