WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
134666
NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace
https://bugs.webkit.org/show_bug.cgi?id=134666
Summary
NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace
Brady Eidson
Reported
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
>
Attachments
Patch v1 - Speculative fix
(1.52 KB, patch)
2014-07-06 16:09 PDT
,
Brady Eidson
mitz: review+
Details
Formatted Diff
Diff
Patch v2 - A way better speculative fix
(3.00 KB, patch)
2014-07-06 16:46 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Add SecAccessControlRef support to ArgumentCodersCF
(5.21 KB, patch)
2014-07-13 23:47 PDT
,
mitz
thorton
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2014-07-06 16:09:26 PDT
Created
attachment 234467
[details]
Patch v1 - Speculative fix
Tim Horton
Comment 2
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".
Brady Eidson
Comment 3
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.
Tim Horton
Comment 4
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.
Brady Eidson
Comment 5
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.
Brady Eidson
Comment 6
2014-07-06 16:46:22 PDT
Created
attachment 234468
[details]
Patch v2 - A way better speculative fix
Tim Horton
Comment 7
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.
WebKit Commit Bot
Comment 8
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
Brady Eidson
Comment 9
2014-07-06 17:02:02 PDT
Fixed that locally, landed in
http://trac.webkit.org/changeset/170831
mitz
Comment 10
2014-07-13 23:34:59 PDT
The problem isn’t fixed, and I have a much-less-speculative fix. Patch forthcoming.
mitz
Comment 11
2014-07-13 23:47:32 PDT
Created
attachment 234841
[details]
Add SecAccessControlRef support to ArgumentCodersCF
Tim Horton
Comment 12
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?
mitz
Comment 13
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.
mitz
Comment 14
2014-07-14 00:20:21 PDT
Fixed in <
http://trac.webkit.org/r171060
>.
Brady Eidson
Comment 15
2014-07-14 08:22:06 PDT
Great find! Thanks!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug