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>
Created attachment 234467 [details] Patch v1 - Speculative fix
(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".
(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.
(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.
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.
Created attachment 234468 [details] Patch v2 - A way better speculative fix
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 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
Fixed that locally, landed in http://trac.webkit.org/changeset/170831
The problem isn’t fixed, and I have a much-less-speculative fix. Patch forthcoming.
Created attachment 234841 [details] Add SecAccessControlRef support to ArgumentCodersCF
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?
(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.
Fixed in <http://trac.webkit.org/r171060>.
Great find! Thanks!