diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 1ef42cc76f..94e591128b 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -262,14 +262,17 @@ const Candidate& Connection::remote_candidate() const { } const rtc::Network* Connection::network() const { + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in network()"; return port()->Network(); } int Connection::generation() const { + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in generation()"; return port()->generation(); } uint64_t Connection::priority() const { + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in priority()"; if (!port_) return 0; @@ -806,13 +809,14 @@ void Connection::Prune() { void Connection::Destroy() { RTC_DCHECK_RUN_ON(network_thread_); - RTC_DCHECK(port_) << "Calling Destroy() twice?"; + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Destroy()"; if (port_) port_->DestroyConnection(this); } bool Connection::Shutdown() { RTC_DCHECK_RUN_ON(network_thread_); + RTC_DCHECK(port_) << ToDebugId() << ": Calling Shutdown() twice?"; if (!port_) return false; // already shut down. @@ -832,6 +836,9 @@ bool Connection::Shutdown() { // information required for logging needs access to `port_`. port_.reset(); + // Clear any pending requests (or responses). + requests_.Clear(); + return true; } @@ -846,6 +853,7 @@ void Connection::FailAndPrune() { // will be nulled. // In such a case, there's a chance that the Port object gets // deleted before the Connection object ends up being deleted. + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in FailAndPrune()"; if (!port_) return; @@ -882,6 +890,7 @@ void Connection::set_selected(bool selected) { void Connection::UpdateState(int64_t now) { RTC_DCHECK_RUN_ON(network_thread_); + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in UpdateState()"; if (!port_) return; @@ -958,6 +967,7 @@ int64_t Connection::last_ping_sent() const { void Connection::Ping(int64_t now, std::unique_ptr delta) { RTC_DCHECK_RUN_ON(network_thread_); + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Ping()"; if (!port_) return; @@ -1251,11 +1261,13 @@ std::string Connection::ToDebugId() const { uint32_t Connection::ComputeNetworkCost() const { // TODO(honghaiz): Will add rtt as part of the network cost. + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in ComputeNetworkCost()"; return port()->network_cost() + remote_candidate_.network_cost(); } std::string Connection::ToString() const { RTC_DCHECK_RUN_ON(network_thread_); + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in ToString()"; constexpr absl::string_view CONNECT_STATE_ABBREV[2] = { "-", // not connected (false) "C", // connected (true) @@ -1457,6 +1469,8 @@ void Connection::OnConnectionRequestResponse(StunRequest* request, void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request, StunMessage* response) { + RTC_DCHECK(port_) << ToDebugId() + << ": port_ null in OnConnectionRequestErrorResponse"; if (!port_) return; @@ -1610,6 +1624,8 @@ ConnectionInfo Connection::stats() { void Connection::MaybeUpdateLocalCandidate(StunRequest* request, StunMessage* response) { + RTC_DCHECK(port_) << ToDebugId() + << ": port_ null in MaybeUpdateLocalCandidate"; if (!port_) return; @@ -1754,6 +1770,7 @@ ProxyConnection::ProxyConnection(rtc::WeakPtr port, int ProxyConnection::Send(const void* data, size_t size, const rtc::PacketOptions& options) { + RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Send()"; if (!port_) return SOCKET_ERROR; diff --git a/p2p/base/connection.h b/p2p/base/connection.h index 8e439855fa..1ea9180e54 100644 --- a/p2p/base/connection.h +++ b/p2p/base/connection.h @@ -111,6 +111,7 @@ class RTC_EXPORT Connection : public CandidatePairInterface { bool connected() const; bool weak() const; bool active() const; + bool pending_delete() const { return !port_; } // A connection is dead if it can be safely deleted. bool dead(int64_t now) const; diff --git a/p2p/base/port.cc b/p2p/base/port.cc index a90ab632c7..afd998c3ab 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -188,8 +188,8 @@ void Port::Construct() { Port::~Port() { RTC_DCHECK_RUN_ON(thread_); - CancelPendingTasks(); DestroyAllConnections(); + CancelPendingTasks(); } const absl::string_view Port::Type() const { diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 25d387cc3a..15c1e6a269 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -133,31 +133,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 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 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 { diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc index 6831d9ffa2..2f88ab1fd3 100644 --- a/p2p/base/stun_request_unittest.cc +++ b/p2p/base/stun_request_unittest.cc @@ -54,15 +54,15 @@ class StunRequestTest : public ::testing::Test { request_count_++; } - void OnResponse(StunMessage* res) { + virtual void OnResponse(StunMessage* res) { response_ = res; success_ = true; } - void OnErrorResponse(StunMessage* res) { + virtual void OnErrorResponse(StunMessage* res) { response_ = res; failure_ = true; } - void OnTimeout() { timeout_ = true; } + virtual void OnTimeout() { timeout_ = true; } protected: rtc::AutoThread main_thread_; @@ -216,4 +216,42 @@ TEST_F(StunRequestTest, TestUnrecognizedComprehensionRequiredAttribute) { EXPECT_FALSE(timeout_); } +class StunRequestReentranceTest : public StunRequestTest { + public: + void OnResponse(StunMessage* res) override { + manager_.Clear(); + StunRequestTest::OnResponse(res); + } + void OnErrorResponse(StunMessage* res) override { + manager_.Clear(); + StunRequestTest::OnErrorResponse(res); + } +}; + +TEST_F(StunRequestReentranceTest, TestSuccess) { + auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr res = + request->CreateResponseMessage(STUN_BINDING_RESPONSE); + manager_.Send(request); + EXPECT_TRUE(manager_.CheckResponse(res.get())); + + EXPECT_TRUE(response_ == res.get()); + EXPECT_TRUE(success_); + EXPECT_FALSE(failure_); + EXPECT_FALSE(timeout_); +} + +TEST_F(StunRequestReentranceTest, TestError) { + auto* request = new StunRequestThunker(manager_, this); + std::unique_ptr res = + request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE); + manager_.Send(request); + EXPECT_TRUE(manager_.CheckResponse(res.get())); + + EXPECT_TRUE(response_ == res.get()); + EXPECT_FALSE(success_); + EXPECT_TRUE(failure_); + EXPECT_FALSE(timeout_); +} + } // namespace cricket