REGRESSION (r258337): Crash when right clicking on link that uses the system UI font with optimizeLegibility on Mojave

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

REGRESSION (r258337): Crash when right clicking on link that uses the system UI font with optimizeLegibility on Mojave

[Tim Horton]

Created attachment 396583 [details]
Patch

[Tim Horton]

<rdar://problem/61646717>

[Darin Adler]

Comment on attachment 396583 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:70
> +    if ([object isKindOfClass:[NSFont class]]) {
> +        NSFont *font = static_cast<NSFont *>(object);

Could be slightly more elegant using something like dynamic_cf_cast. Sadly that template has an assertion in it, ASSERT_WITH_SECURITY_IMPLICATION no less, for no reason I can discern given its name.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:388
> +    auto delegate = adoptNS([[WKSecureCodingFontAttributeNormalizer alloc] init]);
> +    [archiver setDelegate:delegate.get()];

One line version is even better.

[Darin Adler]

Comment on attachment 396583 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:73
> +        return [NSFont fontWithDescriptor:WebKit::fontDescriptorWithFontAttributes(font.fontDescriptor.fontAttributes) size:0];

Could we share even more code?

[Darin Adler]

Comment on attachment 396583 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:73
>> +        return [NSFont fontWithDescriptor:WebKit::fontDescriptorWithFontAttributes(font.fontDescriptor.fontAttributes) size:0];
> 
> Could we share even more code?

Seems all the callers end up calling fontDescriptorWithFontAttributes and then -[NSFont fontWithDescriptor:size:], offering us a refactoring opportunity.

[David Kilzer (:ddkilzer)]

Comment on attachment 396583 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:70
>> +        NSFont *font = static_cast<NSFont *>(object);
> 
> Could be slightly more elegant using something like dynamic_cf_cast. Sadly that template has an assertion in it, ASSERT_WITH_SECURITY_IMPLICATION no less, for no reason I can discern given its name.

ASSERT_WITH_SECURITY_IMPLICATION is a way to classify Debug assertions so that they could be enabled for fuzzing in Release builds.  (This can be accomplished by defining a pre-processor macro at build time.)

[Tim Horton]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> Comment on attachment 396583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396583&action=review
> 
> >> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:70
> >> +        NSFont *font = static_cast<NSFont *>(object);
> > 
> > Could be slightly more elegant using something like dynamic_cf_cast. Sadly that template has an assertion in it, ASSERT_WITH_SECURITY_IMPLICATION no less, for no reason I can discern given its name.
> 
> ASSERT_WITH_SECURITY_IMPLICATION is a way to classify Debug assertions so
> that they could be enabled for fuzzing in Release builds.  (This can be
> accomplished by defining a pre-processor macro at build time.)

I think Darin's point was that it would be nice if it just did the type check and returned nil if it were the wrong type, instead of crashing. There is no actual security implication of "you kindly asked me to carefully convert this thing and it wasn't actually what you asked so I returned nothing". That way you could use it to turn those two lines into one.

[Tim Horton]

I wonder why the API test is failing. Going to upload a version with logging since it doesn't fail for me.

[Tim Horton]

Created attachment 396596 [details]
Patch

[Tim Horton]

(In reply to Darin Adler from comment #3)
> Comment on attachment 396583 [details]
> Patch
> 
> > Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:388
> > +    auto delegate = adoptNS([[WKSecureCodingFontAttributeNormalizer alloc] init]);
> > +    [archiver setDelegate:delegate.get()];
> 
> One line version is even better.

Wait, no it's not! What would keep the delegate alive in that case? (Or were you imagining a different one-line than me?)

[Tim Horton]

Created attachment 396602 [details]
Patch

[Darin Adler]

Comment on attachment 396583 [details]
Patch

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

>>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:388
>>> +    auto delegate = adoptNS([[WKSecureCodingFontAttributeNormalizer alloc] init]);
>>> +    [archiver setDelegate:delegate.get()];
>> 
>> One line version is even better.
> 
> Wait, no it's not! What would keep the delegate alive in that case? (Or were you imagining a different one-line than me?)

Wow, you are so right, and I was so wrong.

Let me say the opposite: Is the guarantee that an archiver won’t call its delegate after "finishEncoding" super-strong? If not, could make this code more straightforwardly robust by calling [archiver setDelegate:nil] after calling fnishEncoding. Generally seems risky to leave a dangling reference inside the archiver.

[Tim Horton]

(In reply to Darin Adler from comment #12)
> Comment on attachment 396583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396583&action=review
> 
> >>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:388
> >>> +    auto delegate = adoptNS([[WKSecureCodingFontAttributeNormalizer alloc] init]);
> >>> +    [archiver setDelegate:delegate.get()];
> >> 
> >> One line version is even better.
> > 
> > Wait, no it's not! What would keep the delegate alive in that case? (Or were you imagining a different one-line than me?)
> 
> Wow, you are so right, and I was so wrong.
> 
> Let me say the opposite: Is the guarantee that an archiver won’t call its
> delegate after "finishEncoding" super-strong? If not, could make this code
> more straightforwardly robust by calling [archiver setDelegate:nil] after
> calling fnishEncoding. Generally seems risky to leave a dangling reference
> inside the archiver.

Good point. I'll clear it out! I do think that -finishEncoding does mean you won't get called back again, but no reason not to be careful.

[Tim Horton]

Created attachment 396614 [details]
Patch

[EWS]

Committed r260170: <https://trac.webkit.org/changeset/260170>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396614 [details].

[Darin Adler]

(In reply to Tim Horton from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > > Could be slightly more elegant using something like dynamic_cf_cast. Sadly that template has an assertion in it, ASSERT_WITH_SECURITY_IMPLICATION no less, for no reason I can discern given its name.
> > 
> > ASSERT_WITH_SECURITY_IMPLICATION is a way to classify Debug assertions so
> > that they could be enabled for fuzzing in Release builds.  (This can be
> > accomplished by defining a pre-processor macro at build time.)
> 
> I think Darin's point was that it would be nice if it just did the type
> check and returned nil if it were the wrong type, instead of crashing. There
> is no actual security implication of "you kindly asked me to carefully
> convert this thing and it wasn't actually what you asked so I returned
> nothing". That way you could use it to turn those two lines into one.

Yes, Tim’s right.

It's a mistake for dynamic_cf_cast to have any assertion. If dynamic_cf_cast was going to include an assertion then:

1) It should not be named after the C++ language feature dynamic_cast, since that is a check that is used in cases where it can either succeed or fail, and doesn't include an assertion.

2) It should *never* be used when there's a legitimate reason the type might not match. We don't assert things based on hope, but based on knowledge.

3) There's no obvious reason why we would have both dyanmic_cf_cast and checked_cf_cast. The whole point of checked_cf_cast is that we use it when we know the type and that's why *it* aborts at runtime if the check is wrong.

Not that the checkin checked_cf_cast is not really what I would call an assertion. It's a runtime check, important for security. I know we are now using the name RELEASE_ASSERT for these things, but honestly I don't think that "assertion" is the right name for checks of things the code doesn't guarantee to be true. Checking for overflow given an "untrusted" argument or for bad parameters passed across process boundaries is extremely important, but not quite the same thing as asserting something we believe the code already guarantees. There is some overlap between the concepts of course, but I think this primarily leads to confusion.

So if we want to preserve the assertions done by dynamic_cf_cast, then:

- Callers of dynamic_cf_cast should be changed to checked_cf_cast to make sure they keep doing checks. (Unless for some reason we prefer assertions that are not included in release builds, in which case we could create a asserting_cf_cast or something like that.)

- Then the assertion in dynamic_cf_cast should be removed and we can start using it in cases like this one.

- Or we could just remove dyanmic_cf_cast.

[Darin Adler]

Comment on attachment 396614 [details]
Patch

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

Love this version you landed!

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2893
> +        PlatformFont *font = fontWithAttributes(attributeDictionary, fontSize);

I’d suggest auto here instead of naming the type.