Bug 198388

Summary: Network process crash when decoding SecItemResponseData
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ggaren, mitz, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2019-05-30 13:24:39 PDT
Network process crash when decoding SecItemResponseData:
Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil'
terminating with uncaught exception of type NSException
abort() called

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff49efcd49 __exceptionPreprocess + 256
1   libobjc.A.dylib                     0x00007fff750f8a17 objc_exception_throw + 48
2   CoreFoundation                      0x00007fff49f3bd44 -[CFPrefsConfigurationFileSource initWithConfigurationPropertyList:containingPreferences:] + 0
3   CoreFoundation                      0x00007fff49e388dc -[__NSArrayM insertObject:atIndex:] + 1216
4   WebKit                              0x000000010eaa8dc4 _ZN3IPC6decodeERNS_7DecoderERN3WTF9RetainPtrIPK9__CFArrayEE + 172
5   WebKit                              0x000000010eaa8ae9 _ZN3IPC6decodeERNS_7DecoderERN3WTF9RetainPtrIPKvEE + 82
6   WebKit                              0x000000010ec5d9e9 _ZN6WebKit19SecItemResponseData6decodeERN3IPC7DecoderE + 77
7   WebKit                              0x000000010ec5e0bc _ZN3IPC7DecoderrsIN6WebKit19SecItemResponseDataELPv0EEERS0_RN3WTF8OptionalIT_EE + 34
8   WebKit                              0x000000010ec5e051 _ZN3IPC16TupleDecoderImplIN6WebKit19SecItemResponseDataEJEE6decodeERNS_7DecoderE + 41
9   WebKit                              0x000000010ec5def8 _ZN3IPC7DecoderrsINSt3__15tupleIJN6WebKit19SecItemResponseDataEEEELPv0EEERS0_RN3WTF8OptionalIT_EE + 34
10  WebKit                              0x000000010ec5de40 _ZZN3IPC10Connection13sendWithReplyIN8Messages16SecItemShimProxy14SecItemRequestEEEvOT_yRN3WTF18FunctionDispatcherEONS7_8FunctionIFvNS7_8OptionalINS_10CodingTypeINS5_5ReplyEE4TypeEEEEEEENKUlNSt3__110unique_ptrINS_7DecoderENSK_14default_deleteISM_EEEEE_clESP_ + 46
11  WebKit                              0x000000010ec5ddec _ZN3WTF8FunctionIFvNSt3__110unique_ptrIN3IPC7DecoderENS1_14default_deleteIS4_EEEEEE15CallableWrapperIZNS3_10Connection13sendWithReplyIN8Messages16SecItemShimProxy14SecItemRequestEEEvOT_yRNS_18FunctionDispatcherEONS0_IFvNS_8OptionalINS3_10CodingTypeINSG_5ReplyEE4TypeEEEEEEEUlS7_E_E4callES7_ + 38
12  WebKit                              0x000000010eabd325 _ZNK3WTF8FunctionIFvNSt3__110unique_ptrIN3IPC7DecoderENS1_14default_deleteIS4_EEEEEEclES7_ + 41
13  WebKit                              0x000000010eabe107 _ZN3WTF8FunctionIFvvEE15CallableWrapperIZN3IPC10Connection24processIncomingSyncReplyENSt3__110unique_ptrINS4_7DecoderENS6_14default_deleteIS8_EEEEE3$_8E4callEv + 67
Comment 1 Chris Dumez 2019-05-30 13:25:03 PDT
<rdar://problem/50408046>
Comment 2 Chris Dumez 2019-05-30 13:30:29 PDT
Created attachment 370975 [details]
Patch
Comment 3 Alex Christensen 2019-05-30 16:23:30 PDT
Comment on attachment 370975 [details]
Patch

may as well
Comment 4 WebKit Commit Bot 2019-05-30 17:00:14 PDT
Comment on attachment 370975 [details]
Patch

Clearing flags on attachment: 370975

Committed r245911: <https://trac.webkit.org/changeset/245911>
Comment 5 WebKit Commit Bot 2019-05-30 17:00:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 mitz 2019-05-30 17:32:27 PDT
Does it really make sense for the length of the decoded array to be different from the length of the array that was encoded?
Comment 7 Chris Dumez 2019-05-31 08:50:33 PDT
(In reply to mitz from comment #6)
> Does it really make sense for the length of the decoded array to be
> different from the length of the array that was encoded?

The issue is that we're not always able to encode elements, or decode them.

E.g. The SecIdentityRef encoder will not encode a key if copyPersistentRef() return null. The SecIdentityRef decoder, will not construct a SecIdentityRef if not key was encoded, or if the decoding process does not have ProcessPrivilege::CanAccessCredentials privilege. Yet decode() will return true in those cases.

In theory, the SecCertificateRef decoder can also return true while not constructing a SecCertificateRef, if SecCertificateCreateWithData() returns null.