Merge pull request #3408 from dreampiggy/threadsafe_fix_imageio_incremental_animation

Fix the potential out of bounds crash for ImageIO incremental animation decoding (like GIF)

.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

SDWebImage/Core/SDImageCoderHelper.m

@@ -173,7 +173,7 @@ static const CGFloat kDestSeemOverlap = 2.0f;   // the numbers of pixels to over
         return nil;
     }
     
-    NSMutableArray<SDImageFrame *> *frames = [NSMutableArray array];
+    NSMutableArray<SDImageFrame *> *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++) {

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;
@@ -328,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<SDImageFrame *> *frames = [NSMutableArray array];
+        NSMutableArray<SDImageFrame *> *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];
@@ -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<SDImageIOCoderFrame *> *frames = [NSMutableArray array];
+    _loopCount = loopCount;
     
+    NSMutableArray<SDImageIOCoderFrame *> *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) {
+    NSTimeInterval duration;
-        return 0;
+    // 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) {
+    UIImage *image;
-        return nil;
+    // 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, *)) {

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<UIImage *> *scaledImages = [NSMutableArray array];
+        NSArray<UIImage *> *images = image.images;
+        NSMutableArray<UIImage *> *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];
         }

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();