commit 1e688612ccd86cfe42d129118d90ae567502bdd3
author Sameer Vijaykar <samvi@google.com> Wed Aug 10 20:48:03 2022
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Aug 11 08:07:20 2022
tree ec2bc4cb1289c5009a3ed7ebf481b3be53e3682f
parent 6af2ac41ac9240d8d3cfcd47e4f57c5c25f168c9

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 <samvi@google.com>
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37741}

p2p/base/stun_port.cc

diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc
index 8074cbe..44e1243 100644
--- a/p2p/base/stun_port.cc
+++ b/p2p/base/stun_port.cc
@@ -424,8 +424,13 @@
   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);
   }
 }
 

p2p/base/stun_port.h

diff --git a/p2p/base/stun_port.h b/p2p/base/stun_port.h
index 7a3239e..364aed6 100644
--- a/p2p/base/stun_port.h
+++ b/p2p/base/stun_port.h
@@ -210,6 +210,9 @@
   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.

p2p/base/stun_port_unittest.cc

diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc
index a5e13f9..86dd64e 100644
--- a/p2p/base/stun_port_unittest.cc
+++ b/p2p/base/stun_port_unittest.cc
@@ -307,13 +307,7 @@
 };
 
 // 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 @@
 };
 
 // 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) {