234991 2022-01-07 15:18:52 -0800 WebKit::AuthenticatorPresenterCoordinator() constructor falls through ASSERT_NOT_REACHED() 2022-01-19 13:26:00 -0800 1 1 1 Unclassified WebKit WebKit2 Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 234932 1 ddkilzer cdumez cdumez darin ews-watchlist jiewen_tan kkinnunen webkit-bug-importer oldest_to_newest 1829106 0 ddkilzer 2022-01-07 15:18:52 -0800 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. 1829107 1 webkit-bug-importer 2022-01-07 15:19:12 -0800 <rdar://problem/87275093> 1832090 2 449497 cdumez 2022-01-19 10:56:30 -0800 Created attachment 449497 Patch 1832091 3 449497 darin 2022-01-19 11:08:03 -0800 Comment on attachment 449497 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. 1832109 4 cdumez 2022-01-19 11:35:58 -0800 (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? 1832127 5 darin 2022-01-19 11:58:08 -0800 (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. 1832200 6 ews-feeder 2022-01-19 13:25:57 -0800 Committed r288237 (246191@main): <https://commits.webkit.org/246191@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449497. 449497 2022-01-19 10:56:30 -0800 2022-01-19 13:25:58 -0800 Patch bug-234991-20220119105630.patch text/plain 2024 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjg4MjAwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDM1NWIwYmRlOTVkOWQ3NmNh ZWRkMjFjYmRmYjhlNWViY2Y1Nzg5NWYuLjM2ODVhMmUwM2FjZTFlZTdjNmNkOGVhMDYwZjg1MTc3 ZTQyNDQxMjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjItMDEtMTkgIENocmlzIER1 bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KKworICAgICAgICBXZWJLaXQ6OkF1dGhlbnRpY2F0b3JQ cmVzZW50ZXJDb29yZGluYXRvcigpIGNvbnN0cnVjdG9yIGZhbGxzIHRocm91Z2ggQVNTRVJUX05P VF9SRUFDSEVEKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTIzNDk5MQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vODcyNzUwOTM+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRHJvcCB1bm5lY2Vzc2FyeSBk ZWZhdWx0OiBjYXNlIGluIHRoZSBzd2l0Y2ggc3RhdGVtZW50IHNpbmNlIGl0IGhhbmRsZXMgYWxs IGVudW0gdmFsdWVzIGFscmVhZHkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvV2ViQXV0aGVudGlj YXRpb24vQ29jb2EvQXV0aGVudGljYXRvclByZXNlbnRlckNvb3JkaW5hdG9yLm1tOgorICAgICAg ICAoV2ViS2l0OjpBdXRoZW50aWNhdG9yUHJlc2VudGVyQ29vcmRpbmF0b3I6OkF1dGhlbnRpY2F0 b3JQcmVzZW50ZXJDb29yZGluYXRvcik6CisKIDIwMjItMDEtMTkgIENocmlzIER1bWV6ICA8Y2R1 bWV6QGFwcGxlLmNvbT4KIAogICAgICAgICBXZWJLaXQ6Om5zVGV4dEFsaWdubWVudEZyb21UZXh0 QWxpZ25tZW50KCkgZmFsbHMgdGhyb3VnaCBBU1NFUlRfTk9UX1JFQUNIRUQoKQpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvV2ViQXV0aGVudGljYXRpb24vQ29jb2EvQXV0aGVu dGljYXRvclByZXNlbnRlckNvb3JkaW5hdG9yLm1tIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3Mv V2ViQXV0aGVudGljYXRpb24vQ29jb2EvQXV0aGVudGljYXRvclByZXNlbnRlckNvb3JkaW5hdG9y Lm1tCmluZGV4IGExN2Y4YTJkYmNkM2EwMmYzZjhhZTliNTYzMWJmOGRlMTMxMjE4ZWMuLjIwMzE2 MGY3MjZhZTJjODg1ZTk1Y2NlM2VhZDE3ZTk1ZTEzNmU0YzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJLaXQvVUlQcm9jZXNzL1dlYkF1dGhlbnRpY2F0aW9uL0NvY29hL0F1dGhlbnRpY2F0b3JQcmVz ZW50ZXJDb29yZGluYXRvci5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9XZWJBdXRo ZW50aWNhdGlvbi9Db2NvYS9BdXRoZW50aWNhdG9yUHJlc2VudGVyQ29vcmRpbmF0b3IubW0KQEAg LTYwLDggKzYwLDYgQEAgQXV0aGVudGljYXRvclByZXNlbnRlckNvb3JkaW5hdG9yOjpBdXRoZW50 aWNhdG9yUHJlc2VudGVyQ29vcmRpbmF0b3IoY29uc3QgQXV0aGUKICAgICAgICAgaWYgKCh0cmFu c3BvcnRzLmNvbnRhaW5zKEF1dGhlbnRpY2F0b3JUcmFuc3BvcnQ6OlVzYikgfHwgdHJhbnNwb3J0 cy5jb250YWlucyhBdXRoZW50aWNhdG9yVHJhbnNwb3J0OjpOZmMpKSAmJiAhdHJhbnNwb3J0cy5j b250YWlucyhBdXRoZW50aWNhdG9yVHJhbnNwb3J0OjpJbnRlcm5hbCkpCiAgICAgICAgICAgICBb bV9jb250ZXh0IGFkZExvZ2luQ2hvaWNlOmFkb3B0TlMoW2FsbG9jQVNDU2VjdXJpdHlLZXlQdWJs aWNLZXlDcmVkZW50aWFsTG9naW5DaG9pY2VJbnN0YW5jZSgpIGluaXRBc3NlcnRpb25QbGFjZWhv bGRlckNob2ljZV0pLmdldCgpXTsKICAgICAgICAgYnJlYWs7Ci0gICAgZGVmYXVsdDoKLSAgICAg ICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAgICAgfQogCiAgICAgbV9wcmVzZW50ZXJEZWxlZ2F0 ZSA9IGFkb3B0TlMoW1tXS0FTQ0F1dGhvcml6YXRpb25QcmVzZW50ZXJEZWxlZ2F0ZSBhbGxvY10g aW5pdFdpdGhDb29yZGluYXRvcjoqdGhpc10pOwo=