Bug 234991 – WebKit::AuthenticatorPresenterCoordinator() constructor falls through ASSERT_NOT_REACHED()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/87275093>

[Chris Dumez]

Created attachment 449497 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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

[Darin Adler]

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

[EWS]

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