Bug 187110 – Add nullptr check for xpc_connection_t in AuthenticationManager::initializeConnection

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Add null check for xpc_connection_t in AuthenticationManager::initializeConnection and AuthenticationChallengeProxy::sendClientCertificateCredentialOverXpc.

[Radar WebKit Bug Importer]

<rdar://problem/41536815>

[Jiewen Tan]

Created attachment 343738 [details]
Patch

[EWS Watchlist]

Comment on attachment 343738 [details]
Patch

Attachment 343738 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8364146

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 343775 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jiewen Tan]

The tree of win EWS bot is red.

[Anders Carlsson]

The ChangeLog doesn't explain why this change is necessary.

[Brent Fulgham]

Comment on attachment 343738 [details]
Patch

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

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:43
> +    if (!connection || !connection->xpcConnection()) {

I don't understand this change. It seems like you should add your nullptr check, but KEEP the XPC_TYPE_CONNECTION check. Why are you removing the XPC type check?

> Source/WebKit/UIProcess/Authentication/cocoa/AuthenticationChallengeProxyCocoa.mm:56
> +    if (!m_connection->xpcConnection()) {

Is the issue that sometimes we go to relay client certificate data to the network process, but the network process has been terminated?

[Jiewen Tan]

Comment on attachment 343738 [details]
Patch

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

I will add a description as well.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:43
>> +    if (!connection || !connection->xpcConnection()) {
> 
> I don't understand this change. It seems like you should add your nullptr check, but KEEP the XPC_TYPE_CONNECTION check. Why are you removing the XPC type check?

I actually make a mistake last time when I added this check. What I was trying to do is to do the nullptr check. Then I discovered xpc_get_type(). Therefore I used it to do a more general check. However, xpc_get_type() doesn't do the nullptr check for its parameters... Therefore, we now crash here. Since nullptr check is what I actually need, I don't have to check the type of the object.

>> Source/WebKit/UIProcess/Authentication/cocoa/AuthenticationChallengeProxyCocoa.mm:56
>> +    if (!m_connection->xpcConnection()) {
> 
> Is the issue that sometimes we go to relay client certificate data to the network process, but the network process has been terminated?

I haven't observed any issues here. I added this just in case there are some future issues. I can remove this check here.

[Jiewen Tan]

Created attachment 343825 [details]
Patch

[Jiewen Tan]

Created attachment 343826 [details]
Patch

[Brent Fulgham]

Comment on attachment 343826 [details]
Patch

r=me

[Jiewen Tan]

Comment on attachment 343826 [details]
Patch

Thanks Brent for r+ this patch.

[WebKit Commit Bot]

Comment on attachment 343826 [details]
Patch

Clearing flags on attachment: 343826

Committed r233340: <https://trac.webkit.org/changeset/233340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.