From 1e688612ccd86cfe42d129118d90ae567502bdd3 Mon Sep 17 00:00:00 2001 From: Sameer Vijaykar Date: Wed, 10 Aug 2022 22:48:03 +0200 Subject: [PATCH] Fix use after free in UdpPort SendStunBindingRequest can cause a SocketAddress pointed to by the iterator to be deleted asynchronously before returning, causing `it` to be invalid when incrementing in the continuation step. Bug: webrtc:7309, webrtc:14131 Change-Id: I3f7d3d7c12935d9592ef3642679a821c58826df9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/270744 Commit-Queue: Sameer Vijaykar Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#37741} --- p2p/base/stun_port.cc | 9 +++++++-- p2p/base/stun_port.h | 3 +++ p2p/base/stun_port_unittest.cc | 11 ++--------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index 8074cbe7df..44e1243846 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc @@ -424,8 +424,13 @@ void UDPPort::SendStunBindingRequests() { RTC_DCHECK(request_manager_.empty()); for (ServerAddresses::const_iterator it = server_addresses_.begin(); - it != server_addresses_.end(); ++it) { - SendStunBindingRequest(*it); + it != server_addresses_.end();) { + // sending a STUN binding request may cause the current SocketAddress to be + // erased from the set, invalidating the loop iterator before it is + // incremented (even if the SocketAddress itself still exists). So make a + // copy of the loop iterator, which may be safely invalidated. + ServerAddresses::const_iterator addr = it++; + SendStunBindingRequest(*addr); } } diff --git a/p2p/base/stun_port.h b/p2p/base/stun_port.h index 7a3239ee95..364aed6d33 100644 --- a/p2p/base/stun_port.h +++ b/p2p/base/stun_port.h @@ -210,6 +210,9 @@ class UDPPort : public Port { void ResolveStunAddress(const rtc::SocketAddress& stun_addr); void OnResolveResult(const rtc::SocketAddress& input, int error); + // Send a STUN binding request to the given address. Calling this method may + // cause the set of known server addresses to be modified, eg. by replacing an + // unresolved server address with a resolved address. void SendStunBindingRequest(const rtc::SocketAddress& stun_addr); // Below methods handles binding request responses. diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index a5e13f9f69..86dd64e308 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc @@ -307,13 +307,7 @@ class StunPortWithMockDnsResolverTest : public StunPortTest { }; // Test that we can get an address from a STUN server specified by a hostname. -// Crashes on Linux, see webrtc:7416 -#if defined(WEBRTC_LINUX) || defined(WEBRTC_WIN) || defined(WEBRTC_MAC) -#define MAYBE_TestPrepareAddressHostname DISABLED_TestPrepareAddressHostname -#else -#define MAYBE_TestPrepareAddressHostname TestPrepareAddressHostname -#endif -TEST_F(StunPortWithMockDnsResolverTest, MAYBE_TestPrepareAddressHostname) { +TEST_F(StunPortWithMockDnsResolverTest, TestPrepareAddressHostname) { SetDnsResolverExpectations( [](webrtc::MockAsyncDnsResolver* resolver, webrtc::MockAsyncDnsResolverResult* resolver_result) { @@ -625,8 +619,7 @@ class StunIPv6PortTestWithMockDnsResolver : public StunIPv6PortTest { }; // Test that we can get an address from a STUN server specified by a hostname. -// Crashes on Linux, see webrtc:7416 -TEST_F(StunIPv6PortTestWithMockDnsResolver, MAYBE_TestPrepareAddressHostname) { +TEST_F(StunIPv6PortTestWithMockDnsResolver, TestPrepareAddressHostname) { SetDnsResolverExpectations( [](webrtc::MockAsyncDnsResolver* resolver, webrtc::MockAsyncDnsResolverResult* resolver_result) {