Bug 234991

Summary: WebKit::AuthenticatorPresenterCoordinator() constructor falls through ASSERT_NOT_REACHED()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, ews-watchlist, jiewen_tan, kkinnunen, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234932    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Description David Kilzer (:ddkilzer) 2022-01-07 15:18:52 PST
WebKit::AuthenticatorPresenterCoordinator() constructor falls through ASSERT_NOT_REACHED().

If the wrong `type` is passed in, the constructor would make an invalid object.

I'd recommend using a "create()" pattern that can return `nullptr` (or empty std::unique_ptr<>) which checks the `type` parameter before calling the constructor.

AuthenticatorPresenterCoordinator::AuthenticatorPresenterCoordinator(const AuthenticatorManager& manager, const String& rpId, const TransportSet& transports, ClientDataType type, const String& username)
    : m_manager(manager)
{
#if HAVE(ASC_AUTH_UI)
    m_context = adoptNS([allocASCAuthorizationPresentationContextInstance() initWithRequestContext:nullptr appIdentifier:nullptr]);
    if ([getASCAuthorizationPresentationContextClass() instancesRespondToSelector:@selector(setServiceName:)])
        [m_context setServiceName:rpId];

    switch (type) {
    case ClientDataType::Create: {
        auto options = adoptNS([allocASCPublicKeyCredentialCreationOptionsInstance() init]);
        [options setUserName:username];

        if (transports.contains(AuthenticatorTransport::Internal))
            [m_context addLoginChoice:adoptNS([allocASCPlatformPublicKeyCredentialLoginChoiceInstance() initRegistrationChoiceWithOptions:options.get()]).get()];
        if (transports.contains(AuthenticatorTransport::Usb) || transports.contains(AuthenticatorTransport::Nfc))
            [m_context addLoginChoice:adoptNS([allocASCSecurityKeyPublicKeyCredentialLoginChoiceInstance() initRegistrationChoiceWithOptions:options.get()]).get()];
        break;
    }
    case ClientDataType::Get:
        if ((transports.contains(AuthenticatorTransport::Usb) || transports.contains(AuthenticatorTransport::Nfc)) && !transports.contains(AuthenticatorTransport::Internal))
            [m_context addLoginChoice:adoptNS([allocASCSecurityKeyPublicKeyCredentialLoginChoiceInstance() initAssertionPlaceholderChoice]).get()];
        break;
    default:
        ASSERT_NOT_REACHED();
    }

    [...]
#endif // HAVE(ASC_AUTH_UI)
}

See Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm.
Comment 1 Radar WebKit Bug Importer 2022-01-07 15:19:12 PST
<rdar://problem/87275093>
Comment 2 Chris Dumez 2022-01-19 10:56:30 PST
Created attachment 449497 [details]
Patch
Comment 3 Darin Adler 2022-01-19 11:08:03 PST
Comment on attachment 449497 [details]
Patch

I guess this is OK.

I never know what we should do for illegal enum values. They are impossible the same what that a null reference is impossible, but that doesn't mean they can’t happen if there is some bug elsewhere. So what should our strategy be. Compilers don’t even agree whether code is "reachable" after a switch statement that covers all enumeration values. Some consider that dead code, others warn if that code doesn’t have a return statement of the correct type.

It would be valuable to drop into the debugger if an illegal enum value was present here. Inelegant to have to write default, annoying that writing default sacrifices the "unhandled enum value" warning, but definitely a good things if we detected the bad value.
Comment 4 Chris Dumez 2022-01-19 11:35:58 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 449497 [details]
> Patch
> 
> I guess this is OK.
> 
> I never know what we should do for illegal enum values. They are impossible
> the same what that a null reference is impossible, but that doesn't mean
> they can’t happen if there is some bug elsewhere. So what should our
> strategy be. Compilers don’t even agree whether code is "reachable" after a
> switch statement that covers all enumeration values. Some consider that dead
> code, others warn if that code doesn’t have a return statement of the
> correct type.
> 
> It would be valuable to drop into the debugger if an illegal enum value was
> present here. Inelegant to have to write default, annoying that writing
> default sacrifices the "unhandled enum value" warning, but definitely a good
> things if we detected the bad value.

enum class ClientDataType : bool;

Given that it is a boolean, I don't think there can be a "bad" value?
Comment 5 Darin Adler 2022-01-19 11:58:08 PST
(In reply to Chris Dumez from comment #4)
> Given that it is a boolean, I don't think there can be a "bad" value?

A boolean stored in a byte can be 0 or 1, the good values, or some other value, 2-255. And how the code behaves when the byte has an illegal value is undefined and unpredictable. Again, just like a reference "can't be null", but it can. A boolean *can* be 2-255 if the value is overwritten. This becomes even more clear in the case of non-boolean enumerations.
Comment 6 EWS 2022-01-19 13:25:57 PST
Committed r288237 (246191@main): <https://commits.webkit.org/246191@main>

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