Bug 156947 – Networking process crash due to missing -[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:] implementation

• Component: Page Loading
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

com.apple.WebKit.Networking crashes at -[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:] due to an unrecognized selector sent to instance:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:]: unrecognized selector sent to instance 0x7fbe4ba2c100'
abort() called
terminating with uncaught exception of type NSException

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff81124e32 __exceptionPreprocess + 178
1   libobjc.A.dylib                     0x00007fff8a34f4fa objc_exception_throw + 48
2   CoreFoundation                      0x00007fff8118e34d -[NSObject(NSObject) doesNotRecognizeSelector:] + 205
3   CoreFoundation                      0x00007fff81095472 ___forwarding___ + 514
4   CoreFoundation                      0x00007fff810951e8 _CF_forwarding_prep_0 + 120
5   WebKit                              0x00007fff8bf08490 _ZN6WebKit21AuthenticationManager40performDefaultHandlingForSingleChallengeEy + 104
6   WebKit                              0x00007fff8bf083d9 _ZN6WebKit21AuthenticationManager22performDefaultHandlingEy + 73
7   WebKit                              0x00007fff8bf098ca _ZN6WebKit21AuthenticationManager17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE + 196
8   WebKit                              0x00007fff8bf44b61 _ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_14MessageDecoderE + 113
9   WebKit                              0x00007fff8bf69216 _ZN6WebKit14NetworkProcess17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE + 28
10  WebKit                              0x00007fff8bf0c602 _ZN3IPC10Connection15dispatchMessageENSt3__110unique_ptrINS_14MessageDecoderENS1_14default_deleteIS3_EEEE + 102
11  WebKit                              0x00007fff8bf0eb2e _ZN3IPC10Connection18dispatchOneMessageEv + 114
12  JavaScriptCore                      0x00007fff92100d85 _ZN3WTF7RunLoop11performWorkEv + 437
13  JavaScriptCore                      0x00007fff92101462 _ZN3WTF7RunLoop11performWorkEPv + 34
14  CoreFoundation                      0x00007fff810ba8b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
15  CoreFoundation                      0x00007fff8109a0ac __CFRunLoopDoSources0 + 556
16  CoreFoundation                      0x00007fff810995cf __CFRunLoopRun + 927
17  CoreFoundation                      0x00007fff81098fc8 CFRunLoopRunSpecific + 296
18  Foundation                          0x00007fff86bcdf59 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 270
19  Foundation                          0x00007fff86bcde38 -[NSRunLoop(NSRunLoop) run] + 74
20  libxpc.dylib                        0x00007fff8412a4c8 _xpc_objc_main + 751
21  libxpc.dylib                        0x00007fff84128f1e xpc_main + 494
22  com.apple.WebKit.Networking         0x10df6db4a main + 16 (Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:89)
23  libdyld.dylib                       0x00007fff94b305ad start + 1

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff884c60ae __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff90a9a500 pthread_kill + 90
2   libsystem_c.dylib             	0x00007fff8723137b abort + 129
3   libc++abi.dylib               	0x00007fff900baf81 abort_message + 257
4   libc++abi.dylib               	0x00007fff900e0a47 default_terminate_handler() + 267
5   libobjc.A.dylib               	0x00007fff8a35115e _objc_terminate() + 103
6   libc++abi.dylib               	0x00007fff900de19e std::__terminate(void (*)()) + 8
7   libc++abi.dylib               	0x00007fff900dde27 __cxa_rethrow + 99
8   libobjc.A.dylib               	0x00007fff8a34f998 objc_exception_rethrow + 40
9   com.apple.CoreFoundation      	0x00007fff8109905b CFRunLoopRunSpecific + 443
10  com.apple.Foundation          	0x00007fff86bcdf59 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 270
11  com.apple.Foundation          	0x00007fff86bcde38 -[NSRunLoop(NSRunLoop) run] + 74
12  libxpc.dylib                  	0x00007fff8412a4c8 _xpc_objc_main + 751
13  libxpc.dylib                  	0x00007fff84128f1e xpc_main + 494
14  com.apple.WebKit.Networking   	0x10df6db4a main + 16 (Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:89)
15  libdyld.dylib                 	0x00007fff94b305ad start + 1

[David Kilzer (:ddkilzer)]

<rdar://problem/23325160>

[David Kilzer (:ddkilzer)]

Created attachment 277136 [details]
Patch v1

[WebKit Commit Bot]

Attachment 277136 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Created attachment 277136 [details]
> Patch v1

Looking for feedback on the approach for the patch, as well as suggestions for how to write a test.  (Does this path require SSL certificate authentication?  Do changes to Tools projects look like they're reasonable?)

[Alexey Proskuryakov]

Comment on attachment 277136 [details]
Patch v1

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

> Does this path require SSL certificate authentication?

Unsure. Maybe sending -performDefaultHandlingForAuthenticationChallenge: for a password challenge would result in returning the original 401 response.

> Source/WebCore/ChangeLog:24
> +        - Add -respondsToSelector: checks since
> +          -performDefaultHandlingForAuthenticationChallenge: and
> +          -rejectProtectionSpaceAndContinueWithChallenge: are optional
> +          by API contract.

I'm not sure how this can work. If we don't send the message, then the sender will never know about the decision, and the connection will leak, won't it?

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:571
> -        [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()];
> +        if ([challenge.sender() respondsToSelector:@selector(performDefaultHandlingForAuthenticationChallenge:)])
> +            [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()];

Is the sender here always CFNetwork? It would seem surprising if it's free to decide which methods of the protocol to implement.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:206
> +    if (gTestRunner->performsDefaultHandlingForAuthenticationChallenge()) {

This is starting to look like we should let tests implement authentication callbacks in JS, instead of controlling client behavior with a set of confusing interdependent booleans.

[David Kilzer (:ddkilzer)]

Created attachment 277147 [details]
Patch v2

[WebKit Commit Bot]

Attachment 277147 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 277136 [details]
Patch v1

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

>> Source/WebCore/ChangeLog:24
>> +          by API contract.
> 
> I'm not sure how this can work. If we don't send the message, then the sender will never know about the decision, and the connection will leak, won't it?

The API contract clearly states these methods are optional:
<https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/#//apple_ref/doc/uid/20001706-SW2>

>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:571
>> +            [challenge.sender() performDefaultHandlingForAuthenticationChallenge:challenge.nsURLAuthenticationChallenge()];
> 
> Is the sender here always CFNetwork? It would seem surprising if it's free to decide which methods of the protocol to implement.

I'm not sure who the sender is, but why is that relevant.  The API contract is very clear that these methods are optional, so why wouldn't we check here?

[Alexey Proskuryakov]

> I'm not sure who the sender is, but why is that relevant.  The API contract is very clear that these methods are optional, so why wouldn't we check here?

That's not the API contract, that's something that unrelated code using this protocol may choose to do.

What matters to us is what CFNetwork does, not what unrelated theoretical class in user code is allowed to do.

[Alexey Proskuryakov]

To put it another way, if CFNetwork ever provides a sender that doesn't implement these methods, we should probably fall back to another method, not just drop the challenge on the floor. 

My uninformed guess is that CFNetwork always implements these, and we are actually dealing with a use after free.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> To put it another way, if CFNetwork ever provides a sender that doesn't
> implement these methods, we should probably fall back to another method, not
> just drop the challenge on the floor. 
> 
> My uninformed guess is that CFNetwork always implements these, and we are
> actually dealing with a use after free.

Moving to RESOLVED/INVALID.

[Alex Christensen]

I think we should at least land the respondsToSelector checks.

[Alexey Proskuryakov]

Is there any evidence that these checks are appropriate? I wouldn't want to confuse the code with unnecessary checks when we expect that there is some other kind of bug in it.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Is there any evidence that these checks are appropriate? I wouldn't want to
> confuse the code with unnecessary checks when we expect that there is some
> other kind of bug in it.

Yes.  The methods are marked as optional in the NSProtocol, so they should be checked to see if they exist before calling them on an object.

Why would it ever be correct to assume that optional methods are always implemented?

[Alexey Proskuryakov]

The methods are optional in protocol, which means that some other code using the protocol is not required to implement them. That is not evidence towards what CFNetwork is required/documented/expected to do.

[Alex Christensen]

Reopening to attach new patch.

[Alex Christensen]

Created attachment 284636 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284636 [details]
Patch

Clearing flags on attachment: 284636

Committed r203743: <http://trac.webkit.org/changeset/203743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Landed without respondsToSelector checks.  Having missing methods in WebCoreAuthenticationClientAsChallengeSender is demonstrably wrong, so we added them with a test.

[Alex Christensen]

Minor test fix in https://trac.webkit.org/changeset/203755

[Alexey Proskuryakov]

I assume that the INVALID state was accidental, moving to FIXED.