| Summary: | Networking process crash due to missing -[WebCoreAuthenticationClientAsChallengeSender performDefaultHandlingForAuthenticationChallenge:] implementation | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
| Component: | Page Loading | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, ap, beidson, commit-queue, dbates, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=160234 https://bugs.webkit.org/show_bug.cgi?id=160235 |
||||||||||
| Attachments: |
|
||||||||||
|
Description
David Kilzer (:ddkilzer)
2016-04-22 21:57:21 PDT
Created attachment 277136 [details]
Patch v1
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.
(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?) 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. Created attachment 277147 [details]
Patch v2
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.
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? > 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.
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. (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. I think we should at least land the respondsToSelector checks. 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. (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? 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. Reopening to attach new patch. Created attachment 284636 [details]
Patch
Comment on attachment 284636 [details] Patch Clearing flags on attachment: 284636 Committed r203743: <http://trac.webkit.org/changeset/203743> All reviewed patches have been landed. Closing bug. Landed without respondsToSelector checks. Having missing methods in WebCoreAuthenticationClientAsChallengeSender is demonstrably wrong, so we added them with a test. Minor test fix in https://trac.webkit.org/changeset/203755 I assume that the INVALID state was accidental, moving to FIXED. |