Bug 194144

Summary: Network Process crash when resuming downloads: '-[__NSDictionaryI setObject:forKey:]: unrecognized selector sent to instance %p'
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description David Quesada 2019-01-31 22:49:27 PST
rdar://problem/47553456
Comment 1 David Quesada 2019-01-31 22:54:59 PST
Resuming a download occasionally causes a Network Process crash due to an uncaught NSInvalidArgumentException. In Download::resume(), we decode the root object from the resume data, assume it's a mutable dictionary (with no type checking), and try to -setObject:forKey: it.
Comment 2 David Quesada 2019-02-01 09:39:16 PST
Created attachment 360866 [details]
Patch
Comment 3 Geoffrey Garen 2019-02-01 13:26:55 PST
Comment on attachment 360866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360866&action=review

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:61
> +    auto dictionary = adoptNS(static_cast<NSMutableDictionary *>([[unarchiver decodeObjectOfClasses:plistClasses forKey:@"NSKeyedArchiveRootObjectKey"] mutableCopy]));

Why do we pass plistClasses to decodeObjectOfClasses? My reading of this code is that any root object class other than NSDictionary would be an error.

Should we just decodeObjectOfClass: [NSDictionary class]?
Comment 4 David Quesada 2019-02-01 13:45:45 PST
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 360866 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360866&action=review
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:61
> > +    auto dictionary = adoptNS(static_cast<NSMutableDictionary *>([[unarchiver decodeObjectOfClasses:plistClasses forKey:@"NSKeyedArchiveRootObjectKey"] mutableCopy]));
> 
> Why do we pass plistClasses to decodeObjectOfClasses? My reading of this
> code is that any root object class other than NSDictionary would be an error.
> 
> Should we just decodeObjectOfClass: [NSDictionary class]?

That won't work. The class whitelist also applies to the objects being decoded by the dictionary. So if we only allow decoding NSDictionary, we wouldn't get any object unless the resume data is a dictionary that only contains other dictionaries as objects (which themselves can only contain dictionaries as objects, recursively).
Comment 5 Geoffrey Garen 2019-02-01 14:31:21 PST
Comment on attachment 360866 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2019-02-01 15:08:53 PST
Comment on attachment 360866 [details]
Patch

Clearing flags on attachment: 360866

Committed r240881: <https://trac.webkit.org/changeset/240881>
Comment 7 WebKit Commit Bot 2019-02-01 15:08:55 PST
All reviewed patches have been landed.  Closing bug.