Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate method assumes NSURLResponse is a NSHTTPURLResponse resulting in possible crash #1109

Closed
sgtsquiggs opened this issue Apr 2, 2015 · 3 comments
Milestone

Comments

@sgtsquiggs
Copy link

in SDWebImageDownloaderOperation.m there is a mistake which will result in a crash when connection:didReceiveResponse: handles a NSURLResponse (ie, a link to a local file in cache) rather than a NSHTTPURLResponse.

The issue is a simple commutative mistake with a simple fix.

The offending delegate method:

- (void)connection:(NSURLConnection *)connection didReceiveResponse:(NSURLResponse *)response {

    //'304 Not Modified' is an exceptional one
    if ((![response respondsToSelector:@selector(statusCode)] || [((NSHTTPURLResponse *)response) statusCode] < 400) && [((NSHTTPURLResponse *)response) statusCode] != 304) {
        NSInteger expected = response.expectedContentLength > 0 ? (NSInteger)response.expectedContentLength : 0;
        self.expectedSize = expected;
        if (self.progressBlock) {
            self.progressBlock(0, expected);
        }

        self.imageData = [[NSMutableData alloc] initWithCapacity:expected];
        self.response = response;
        dispatch_async(dispatch_get_main_queue(), ^{
            [[NSNotificationCenter defaultCenter] postNotificationName:SDWebImageDownloadReceiveResponseNotification object:self];
        });
    }
    else {
        NSUInteger code = [((NSHTTPURLResponse *)response) statusCode];

        //This is the case when server returns '304 Not Modified'. It means that remote image is not changed.
        //In case of 304 we need just cancel the operation and return cached image from the cache.
        if (code == 304) {
            [self cancelInternal];
        } else {
            [self.connection cancel];
        }
        dispatch_async(dispatch_get_main_queue(), ^{
            [[NSNotificationCenter defaultCenter] postNotificationName:SDWebImageDownloadStopNotification object:self];
        });

        if (self.completedBlock) {
            self.completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:[((NSHTTPURLResponse *)response) statusCode] userInfo:nil], YES);
        }
        CFRunLoopStop(CFRunLoopGetCurrent());
        [self done];
    }
}

The offending line alone:

if ((![response respondsToSelector:@selector(statusCode)] || [((NSHTTPURLResponse *)response) statusCode] < 400) && [((NSHTTPURLResponse *)response) statusCode] != 304) {

The proposed fix:

if (![response respondsToSelector:@selector(statusCode)] || ([((NSHTTPURLResponse *)response) statusCode] < 400 && [((NSHTTPURLResponse *)response) statusCode] != 304)) {
@sgtsquiggs
Copy link
Author

Looks like this was just fixed a few days ago by @pollarm in PR #1104. Carry on.

@lqthien
Copy link

lqthien commented May 25, 2015

Seems like the line is still causing crashes on 3.7.2. Our project's code runs fine if we specify 3.7.1.

@bpoplauschi
Copy link
Member

@lqthien we will soon release 3.7.3 with this fix included (#1104).

@bpoplauschi bpoplauschi added this to the 3.7.3 milestone Jun 24, 2015
mwaterfall added a commit to mwaterfall/MWPhotoBrowser that referenced this issue Jul 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants