Potential null-dereference under NetworkRTCProvider::resolvedName(): Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000010) [ 0] 0x000000019250582c WebKit`WebKit::NetworkRTCProvider::resolvedName(__CFHost*, CFHostInfoType, CFStreamError const*, void*) [inlined] WTF::Ref<IPC::Connection>::get() const at Ref.h:138:45 0x000000019250581c: b.ne 0xcd7d8 ; <+164> at NetworkRTCProvider.cpp:199 0x0000000192505820: ldr x8, [sp, #0x28] 0x0000000192505824: ldr x9, [x8, #0x8] 0x0000000192505828: ldr x9, [x9, #0x40] -> 0x000000019250582c: ldr x0, [x9, #0x10] 0x0000000192505830: add x9, sp, #0x10 ; =0x10 0x0000000192505834: str x9, [sp, #0x8] 0x0000000192505838: ldr x2, [x8] 0x000000019250583c: add x1, sp, #0x8 ; =0x8 [ 0] 0x000000019250582c WebKit`WebKit::NetworkRTCProvider::resolvedName(__CFHost*, CFHostInfoType, CFStreamError const*, void*) [inlined] WebKit::NetworkConnectionToWebProcess::connection() at NetworkConnectionToWebProcess.h:59 55 public: 56 static Ref<NetworkConnectionToWebProcess> create(IPC::Connection::Identifier); 57 virtual ~NetworkConnectionToWebProcess(); 58 -> 59 IPC::Connection& connection() { return m_connection.get(); } 60 61 void didCleanupResourceLoader(NetworkResourceLoader&); 62 63 bool captureExtraNetworkLoadMetricsEnabled() const { return m_captureExtraNetworkLoadMetricsEnabled; } [ 0] 0x000000019250582c WebKit`WebKit::NetworkRTCProvider::resolvedName(__CFHost*, CFHostInfoType, CFStreamError const*, void*) + 248 at NetworkRTCProvider.cpp:204 200 auto* address = reinterpret_cast<const struct sockaddr_in*>(CFDataGetBytePtr(data)); 201 addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address->sin_addr))); 202 } 203 ASSERT(resolver->rtcProvider.m_connection); -> 204 resolver->rtcProvider.m_connection->connection().send(Messages::WebRTCResolver::SetResolvedAddress(addresses), resolver->identifier); 205 } 206 207 struct NetworkMessageData : public rtc::MessageData { 208 NetworkMessageData(Ref<NetworkRTCProvider>&& rtcProvider, Function<void()>&& callback) [ 1] 0x00000001925057e7 WebKit`WebKit::NetworkRTCProvider::resolvedName(__CFHost*, CFHostInfoType, CFStreamError const*, void*) + 179 at NetworkRTCProvider.cpp:200:69 196 addresses.reserveInitialCapacity(count); 197 198 for (size_t index = 0; index < count; ++index) { 199 CFDataRef data = (CFDataRef)CFArrayGetValueAtIndex(resolvedAddresses, index); -> 200 auto* address = reinterpret_cast<const struct sockaddr_in*>(CFDataGetBytePtr(data)); 201 addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address->sin_addr))); 202 } 203 ASSERT(resolver->rtcProvider.m_connection); 204 resolver->rtcProvider.m_connection->connection().send(Messages::WebRTCResolver::SetResolvedAddress(addresses), resolver->identifier); [ 2] 0x00000001896ae99f CFNetwork`ClassicHostDelegate::hostInfoCallback(HostBase*, __CFString const*, __CFError*) + 227 at HostBase.cpp:75:3 [ 3] 0x00000001896ae31b CFNetwork`HostBase::invokeCallback(__CFString const*) + 203 at HostBase.cpp:230:6 [ 4] 0x00000001896ada83 CFNetwork`DispatchHost::processPending() + 119 at DispatchHost.cpp:412:4 [ 5] 0x000000018a12d19f CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 23 at CFRunLoop.c:1960:9 [ 6] 0x000000018a12ca83 CoreFoundation`__CFRunLoopDoSources0 + 451 at CFRunLoop.c:2025:25 [ 7] 0x000000018a12a57b CoreFoundation`__CFRunLoopRun + 831 at CFRunLoop.c:2842:41 [ 8] 0x000000018a04503b CoreFoundation`CFRunLoopRunSpecific + 435 at CFRunLoop.c:3148:11 [ 9] 0x000000018bd61f9f Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 303 at NSRunLoop.m:367:5 [ 10] 0x000000018bdb5e0f Foundation`-[NSRunLoop(NSRunLoop) run] + 87 at NSRunLoop.m:389:12 [ 11] 0x00000001ada469eb libxpc.dylib`_xpc_objc_main + 451 at main.m:198:3 [ 12] 0x00000001ada4884f libxpc.dylib`xpc_main + 163 at init.c:1460:2 [ 13] 0x00000001010e759b com.apple.WebKit.Networking`main + 379 [ 14] 0x00000001ad7d7d1b libdyld.dylib`start + 3
<rdar://problem/32597868>
Created attachment 315436 [details] Patch
Comment on attachment 315436 [details] Patch Might want to move back cancel DNS outside of the destructor.
Created attachment 315446 [details] Patch
Comment on attachment 315446 [details] Patch Clearing flags on attachment: 315446 Committed r219514: <http://trac.webkit.org/changeset/219514>
All reviewed patches have been landed. Closing bug.
Comment on attachment 315446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315446&action=review > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189 > + ASSERT(identifier); > + if (auto resolver = m_resolvers.take(identifier)) I understand why you added this assertion, after all the hash table implementation here can’t handle a key of "0". But HashMap already does this assertion, so it’s not necessary to add one at this level before calling take. Specifically, in the case of this call, the assertion is in HashTable::checkKey, called by HashTable::inlineLookup. And it checks not just for 0 but also for the deleted value, which is also needed in this case. Maybe you added this because you didn’t know HashMap would take care of asserting? Or maybe you added this assertion to make the issue clearer to someone reading the source code? Or maybe the HashTable assertion is tricky to understand when it fires and this one is easier to understand? I did notice a problem in HashTable::find and HashTable::contains; if the hash table is empty then it won’t check the key, which is not good. We want the key checked in that case. I think we can probably just remove the m_table null checks from those functions; lookup already has a null check. Maybe the earlier check is an important performance optimization? Or more likely it is just redundant code that doesn’t really significantly speed anything up and causes us to lose the key assertion.
(In reply to Darin Adler from comment #7) > Comment on attachment 315446 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315446&action=review > > > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189 > > + ASSERT(identifier); > > + if (auto resolver = m_resolvers.take(identifier)) > > I understand why you added this assertion, after all the hash table > implementation here can’t handle a key of "0". But HashMap already does this > assertion, so it’s not necessary to add one at this level before calling > take. Specifically, in the case of this call, the assertion is in > HashTable::checkKey, called by HashTable::inlineLookup. And it checks not > just for 0 but also for the deleted value, which is also needed in this case. > > Maybe you added this because you didn’t know HashMap would take care of > asserting? Or maybe you added this assertion to make the issue clearer to > someone reading the source code? Or maybe the HashTable assertion is tricky > to understand when it fires and this one is easier to understand? > > I did notice a problem in HashTable::find and HashTable::contains; if the > hash table is empty then it won’t check the key, which is not good. We want > the key checked in that case. I think we can probably just remove the > m_table null checks from those functions; lookup already has a null check. > Maybe the earlier check is an important performance optimization? Or more > likely it is just redundant code that doesn’t really significantly speed > anything up and causes us to lose the key assertion. I did not know HashTable already had this assertion. We have been burned by this recently which is why I added the assertion. I can drop it though given the HashMap implementation.
(In reply to Chris Dumez from comment #8) > We have been burned by this recently which is why I added the assertion. Makes me wonder if the HashTable assertion is working. > > I did notice a problem in HashTable::find and HashTable::contains; if the > > hash table is empty then it won’t check the key, which is not good. We want > > the key checked in that case. I think we can probably just remove the > > m_table null checks from those functions; lookup already has a null check. > > Maybe the earlier check is an important performance optimization? Or more > > likely it is just redundant code that doesn’t really significantly speed > > anything up and causes us to lose the key assertion. I guess I should follow up on this at some point.