Network Process crash when resuming downloads: '-[__NSDictionaryI setObject:forKey:]: unrecognized selector sent to instance %p'

[David Quesada]

rdar://problem/47553456

[David Quesada]

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.

[David Quesada]

Created attachment 360866 [details]
Patch

[Geoffrey Garen]

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]?

[David Quesada]

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

[Geoffrey Garen]

Comment on attachment 360866 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 360866 [details]
Patch

Clearing flags on attachment: 360866

Committed r240881: <https://trac.webkit.org/changeset/240881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.