Bug 134666

Summary: NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, mitz, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1 - Speculative fix
mitz: review+
Patch v2 - A way better speculative fix
none
Add SecAccessControlRef support to ArgumentCodersCF thorton: review+

Description Brady Eidson 2014-07-06 16:04:31 PDT
NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace

Looking at the backtrace, it's hanging forever inside the SecItem shim, waiting for a response from secItemRequest.

Looking at the SecItemShimProxy in the secItemRequest handler, it seems the only possible way a response would not be sent is if the request type was SecItemRequestData::Invalid.  That condition does an early return instead of sending back a response.

At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.

<rdar://problem/17398060>
Comment 1 Brady Eidson 2014-07-06 16:09:26 PDT
Created attachment 234467 [details]
Patch v1 - Speculative fix
Comment 2 Tim Horton 2014-07-06 16:22:11 PDT
(In reply to comment #0)
> At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.

Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".
Comment 3 Brady Eidson 2014-07-06 16:36:37 PDT
(In reply to comment #2)
> (In reply to comment #0)
> > At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.
> 
> Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".

Killing the NetworkProcess isn't quite a recoverable scenario the same way killing a WebProcess is.

That said:  Your comment encouraged me to look at all the possible scenarios in a little more detail.

The current "invalid" handler is definitely one way the UIProcess would never respond, if a request of type "invalid" was received.

But, as you said, we never explicitly send an invalid request.

Another way the UIProcess might not ever respond is if the message failed to decode.

Looking at SecItemRequestData::decode and comparing it to SecItemRequestData::encode there's an obvious way that the decode could fail - if the CFDictionaryRef for the original query was null.

We'd happily encode such a case, but fail to decode it.
Comment 4 Tim Horton 2014-07-06 16:39:51 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.
> > 
> > Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".
> 
> Killing the NetworkProcess isn't quite a recoverable scenario the same way killing a WebProcess is.

Ah, I suppose! Losing all your downloads and whatnot.

> Looking at SecItemRequestData::decode and comparing it to SecItemRequestData::encode there's an obvious way that the decode could fail - if the CFDictionaryRef for the original query was null.
> 
> We'd happily encode such a case, but fail to decode it.

Aha! Sounds somewhat more promising than the Invalid thing.
Comment 5 Brady Eidson 2014-07-06 16:40:53 PDT
We might've assumed that passing in a null query dictionary is invalid, and therefore a case we didn't need to support...  but maybe we need to support it to catch these invalid uses.
Comment 6 Brady Eidson 2014-07-06 16:46:22 PDT
Created attachment 234468 [details]
Patch v2 - A way better speculative fix
Comment 7 Tim Horton 2014-07-06 16:48:47 PDT
Comment on attachment 234468 [details]
Patch v2 - A way better speculative fix

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

> Source/WebKit2/Shared/mac/SecItemRequestData.cpp:70
> +    bool expectQuery;

maybe "hasQuery"? either way is fine.
Comment 8 WebKit Commit Bot 2014-07-06 16:56:33 PDT
Comment on attachment 234468 [details]
Patch v2 - A way better speculative fix

Rejecting attachment 234468 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
cess/mac/SecItemShimProxy.cpp:66:40: error: use of undeclared identifier 'errSecParam'
        response = SecItemResponseData(errSecParam, nullptr);
                                       ^
1 error generated.

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/SecItemShimProxy.o UIProcess/mac/SecItemShimProxy.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5929684067942400
Comment 9 Brady Eidson 2014-07-06 17:02:02 PDT
Fixed that locally, landed in http://trac.webkit.org/changeset/170831
Comment 10 mitz 2014-07-13 23:34:59 PDT
The problem isn’t fixed, and I have a much-less-speculative fix. Patch forthcoming.
Comment 11 mitz 2014-07-13 23:47:32 PDT
Created attachment 234841 [details]
Add SecAccessControlRef support to ArgumentCodersCF
Comment 12 Tim Horton 2014-07-13 23:55:58 PDT
Comment on attachment 234841 [details]
Add SecAccessControlRef support to ArgumentCodersCF

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

> Source/WebKit2/ChangeLog:11
> +        by ArgumentCodersCF. In debug builds, trying to encode a CFDictionary containing a value of
> +        unsupprted type causes an assertion to fail, but in release builds encoding succeeds, and
> +        only decoding fails, in this case silently, simply not delivering the

This seems like a pretty unfortunate failure mode...

> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:731
> +    result = adoptCF(SecAccessControlCreateFromData(kCFAllocatorDefault, data.get(), nullptr));

Do we know that these things are sufficiently safe to IPC as raw data?
Comment 13 mitz 2014-07-14 00:16:15 PDT
(In reply to comment #12)
> (From update of attachment 234841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234841&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        by ArgumentCodersCF. In debug builds, trying to encode a CFDictionary containing a value of
> > +        unsupprted type causes an assertion to fail, but in release builds encoding succeeds, and
> > +        only decoding fails, in this case silently, simply not delivering the
> 
> This seems like a pretty unfortunate failure mode...

Agreed.

> 
> > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:731
> > +    result = adoptCF(SecAccessControlCreateFromData(kCFAllocatorDefault, data.get(), nullptr));
> 
> Do we know that these things are sufficiently safe to IPC as raw data?

Yes.

Thanks for the review.
Comment 14 mitz 2014-07-14 00:20:21 PDT
Fixed in <http://trac.webkit.org/r171060>.
Comment 15 Brady Eidson 2014-07-14 08:22:06 PDT
Great find!  Thanks!