Bug 208012 – [Cocoa] Limit set of classes that can be decoded when a preference has changed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

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.

[Radar WebKit Bug Importer]

<rdar://problem/59633032>

[Per Arne Vollan]

Created attachment 391299 [details]
Patch

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 392013 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 393062 [details]
Patch

[Per Arne Vollan]

Created attachment 393478 [details]
Patch

[Per Arne Vollan]

Created attachment 393491 [details]
Patch

[Per Arne Vollan]

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

[Brent Fulgham]

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.

[Per Arne Vollan]

Comment on attachment 393491 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 393491 [details]
Patch

Clearing flags on attachment: 393491

Committed r258495: <https://trac.webkit.org/changeset/258495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.