1 files changed, 33 insertions, 13 deletions

diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc
index 25d387cc3a..5bf7dfe69c 100644
--- a/p2p/base/stun_request.cc
+++ b/p2p/base/stun_request.cc
@@ -57,6 +57,10 @@ void StunRequestManager::Send(StunRequest* request) {
 void StunRequestManager::SendDelayed(StunRequest* request, int delay) {
   RTC_DCHECK_RUN_ON(thread_);
   RTC_DCHECK_EQ(this, request->manager());
+  RTC_DCHECK(!request->AuthenticationRequired() ||
+             request->msg()->integrity() !=
+                 StunMessage::IntegrityStatus::kNotSet)
+      << "Sending request w/o integrity!";
   auto [iter, was_inserted] =
       requests_.emplace(request->id(), absl::WrapUnique(request));
   RTC_DCHECK(was_inserted);
@@ -104,15 +108,23 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
   StunRequest* request = iter->second.get();
 
   // Now that we know the request, we can see if the response is
-  // integrity-protected or not.
-  // For some tests, the message integrity is not set in the request.
-  // Complain, and then don't check.
+  // integrity-protected or not. Some requests explicitly disables
+  // integrity checks using SetAuthenticationRequired.
+  // TODO(chromium:1177125): Remove below!
+  // And we suspect that for some tests, the message integrity is not set in the
+  // request. Complain, and then don't check.
   bool skip_integrity_checking =
       (request->msg()->integrity() == StunMessage::IntegrityStatus::kNotSet);
-  if (skip_integrity_checking) {
+  if (!request->AuthenticationRequired()) {
+    // This is a STUN_BINDING to from stun_port.cc or
+    // the initial (unauthenticated) TURN_ALLOCATE_REQUEST.
+  } else if (skip_integrity_checking) {
+    // TODO(chromium:1177125): Remove below!
     // This indicates lazy test writing (not adding integrity attribute).
     // Complain, but only in debug mode (while developing).
-    RTC_DLOG(LS_ERROR)
+    RTC_LOG(LS_ERROR)
+        << "CheckResponse called on a passwordless request. Fix test!";
+    RTC_DCHECK(false)
         << "CheckResponse called on a passwordless request. Fix test!";
   } else {
     if (msg->integrity() == StunMessage::IntegrityStatus::kNotSet) {
@@ -133,31 +145,39 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
     }
   }
 
-  bool success = true;
-
   if (!msg->GetNonComprehendedAttributes().empty()) {
     // If a response contains unknown comprehension-required attributes, it's
     // simply discarded and the transaction is considered failed. See RFC5389
     // sections 7.3.3 and 7.3.4.
     RTC_LOG(LS_ERROR) << ": Discarding response due to unknown "
                          "comprehension-required attribute.";
-    success = false;
+    requests_.erase(iter);
+    return false;
   } else if (msg->type() == GetStunSuccessResponseType(request->type())) {
     if (!msg->IntegrityOk() && !skip_integrity_checking) {
       return false;
     }
-    request->OnResponse(msg);
+    // Erase element from hash before calling callback. This ensures
+    // that the callback can modify the StunRequestManager any way it
+    // sees fit.
+    std::unique_ptr<StunRequest> owned_request = std::move(iter->second);
+    requests_.erase(iter);
+    owned_request->OnResponse(msg);
+    return true;
   } else if (msg->type() == GetStunErrorResponseType(request->type())) {
-    request->OnErrorResponse(msg);
+    // Erase element from hash before calling callback. This ensures
+    // that the callback can modify the StunRequestManager any way it
+    // sees fit.
+    std::unique_ptr<StunRequest> owned_request = std::move(iter->second);
+    requests_.erase(iter);
+    owned_request->OnErrorResponse(msg);
+    return true;
   } else {
     RTC_LOG(LS_ERROR) << "Received response with wrong type: " << msg->type()
                       << " (expecting "
                       << GetStunSuccessResponseType(request->type()) << ")";
     return false;
   }
-
-  requests_.erase(iter);
-  return success;
 }
 
 bool StunRequestManager::empty() const {