As a hardening measure, limit the set of ObjectiveC classes that can be decoded in the WebContent process as a result of a preference change.
<rdar://problem/59633032>
Created attachment 391299 [details] Patch
Comment on attachment 391299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391299&action=review I think this looks good, but marking r- because I think this introduces a leak. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; We should consider making this a static thing that doesn't have to get reconstructed every time there is a preference change. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; Don't we still need to retain the unarchived object returned by this method? Either we were over-releasing previously, or you've introduced a leak here.
Created attachment 392013 [details] Patch
(In reply to Brent Fulgham from comment #3) > Comment on attachment 391299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391299&action=review > > I think this looks good, but marking r- because I think this introduces a > leak. > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; > > We should consider making this a static thing that doesn't have to get > reconstructed every time there is a preference change. > Fixed. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; > > Don't we still need to retain the unarchived object returned by this method? > Either we were over-releasing previously, or you've introduced a leak here. I could be wrong, but I believe this patch does not change the retain count of the object after returning from the method, since the RetainPtr variable was local. Thanks for reviewing!
Created attachment 393062 [details] Patch
Created attachment 393478 [details] Patch
Created attachment 393491 [details] Patch
(In reply to Brent Fulgham from comment #3) > Comment on attachment 391299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391299&action=review > > I think this looks good, but marking r- because I think this introduces a > leak. > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; > > We should consider making this a static thing that doesn't have to get > reconstructed every time there is a preference change. > For some reason, making this static introduced a crash, so I went back to declaring it as a stack allocated variable. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; > > Don't we still need to retain the unarchived object returned by this method? > Either we were over-releasing previously, or you've introduced a leak here.
Comment on attachment 393491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393491&action=review r=me > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:915 > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; Oh, excellent. Thank you for cleaning this one up.
Comment on attachment 393491 [details] Patch Thanks for reviewing!
Comment on attachment 393491 [details] Patch Clearing flags on attachment: 393491 Committed r258495: <https://trac.webkit.org/changeset/258495>
All reviewed patches have been landed. Closing bug.