From af2930a69882e2e29307d4ffae42504965bc328a Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 31 Jan 2022 14:11:39 +0100 Subject: [PATCH] Avoid dangling pointers in a few Connection related classes. Bug: webrtc:11988 Change-Id: I2db1281983396366b91666a1c2bbbcae434ed625 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/249949 Reviewed-by: Niels Moller Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35858} --- p2p/base/basic_ice_controller.cc | 2 ++ p2p/base/p2p_transport_channel.cc | 34 ++++++++++++++++++++++++------- p2p/base/p2p_transport_channel.h | 1 + p2p/base/port.cc | 13 +++++++++++- p2p/base/port.h | 2 ++ p2p/base/turn_port.cc | 8 +------- p2p/base/turn_port.h | 1 - 7 files changed, 45 insertions(+), 16 deletions(-) diff --git a/p2p/base/basic_ice_controller.cc b/p2p/base/basic_ice_controller.cc index 81fb324d1f..9025fbe2a7 100644 --- a/p2p/base/basic_ice_controller.cc +++ b/p2p/base/basic_ice_controller.cc @@ -83,6 +83,8 @@ void BasicIceController::OnConnectionDestroyed(const Connection* connection) { pinged_connections_.erase(connection); unpinged_connections_.erase(connection); connections_.erase(absl::c_find(connections_, connection)); + if (selected_connection_ == connection) + selected_connection_ = nullptr; } bool BasicIceController::HasPingableConnection() const { diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index c530af19bd..9fe9cac948 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -228,6 +228,7 @@ P2PTransportChannel::~P2PTransportChannel() { RTC_DCHECK_RUN_ON(network_thread_); std::vector copy(connections().begin(), connections().end()); for (Connection* con : copy) { + con->SignalDestroyed.disconnect(this); con->Destroy(); } resolvers_.clear(); @@ -1674,7 +1675,11 @@ void P2PTransportChannel::UpdateConnectionStates() { // We need to copy the list of connections since some may delete themselves // when we call UpdateState. - for (Connection* c : connections()) { + // NOTE: We copy the connections() vector in case `UpdateState` triggers the + // Connection to be destroyed (which will cause a callback that alters + // the connections() vector). + std::vector copy(connections().begin(), connections().end()); + for (Connection* c : copy) { c->UpdateState(now); } } @@ -1985,12 +1990,31 @@ void P2PTransportChannel::MaybeStopPortAllocatorSessions() { } } +// RTC_RUN_ON(network_thread_) +void P2PTransportChannel::OnSelectedConnectionDestroyed() { + RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one."; + IceControllerEvent reason = IceControllerEvent::SELECTED_CONNECTION_DESTROYED; + SwitchSelectedConnection(nullptr, reason); + RequestSortAndStateUpdate(reason); +} + // If all connections timed out, delete them all. void P2PTransportChannel::HandleAllTimedOut() { RTC_DCHECK_RUN_ON(network_thread_); - for (Connection* connection : connections()) { + bool update_selected_connection = false; + std::vector copy(connections().begin(), connections().end()); + for (Connection* connection : copy) { + if (selected_connection_ == connection) { + selected_connection_ = nullptr; + update_selected_connection = true; + } + connection->SignalDestroyed.disconnect(this); + ice_controller_->OnConnectionDestroyed(connection); connection->Destroy(); } + + if (update_selected_connection) + OnSelectedConnectionDestroyed(); } bool P2PTransportChannel::ReadyToSend(Connection* connection) const { @@ -2124,11 +2148,7 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) { // we can just set selected to nullptr and re-choose a best assuming that // there was no selected connection. if (selected_connection_ == connection) { - RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one."; - IceControllerEvent reason = - IceControllerEvent::SELECTED_CONNECTION_DESTROYED; - SwitchSelectedConnection(nullptr, reason); - RequestSortAndStateUpdate(reason); + OnSelectedConnectionDestroyed(); } else { // If a non-selected connection was destroyed, we don't need to re-sort but // we do need to update state, because we could be switching to "failed" or diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 174cbc23ce..d06234603c 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -273,6 +273,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { void UpdateState(); void HandleAllTimedOut(); void MaybeStopPortAllocatorSessions(); + void OnSelectedConnectionDestroyed() RTC_RUN_ON(network_thread_); // ComputeIceTransportState computes the RTCIceTransportState as described in // https://w3c.github.io/webrtc-pc/#dom-rtcicetransportstate. ComputeState diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 51297c46c6..c616658fba 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -194,8 +194,10 @@ Port::~Port() { ++iter; } - for (uint32_t i = 0; i < list.size(); i++) + for (uint32_t i = 0; i < list.size(); i++) { + list[i]->SignalDestroyed.disconnect(this); delete list[i]; + } } const std::string& Port::Type() const { @@ -606,6 +608,15 @@ rtc::DiffServCodePoint Port::StunDscpValue() const { return rtc::DSCP_NO_CHANGE; } +void Port::DestroyAllConnections() { + RTC_DCHECK_RUN_ON(thread_); + for (auto kv : connections_) { + kv.second->SignalDestroyed.disconnect(this); + kv.second->Destroy(); + } + connections_.clear(); +} + void Port::set_timeout_delay(int delay) { RTC_DCHECK_RUN_ON(thread_); // Although this method is meant to only be used by tests, some downstream diff --git a/p2p/base/port.h b/p2p/base/port.h index 991872902a..1ec82f7049 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -435,6 +435,8 @@ class Port : public PortInterface, // Extra work to be done in subclasses when a connection is destroyed. virtual void HandleConnectionDestroyed(Connection* conn) {} + void DestroyAllConnections(); + void CopyPortInformationToPacketInfo(rtc::PacketInfo* info) const; MdnsNameRegistrationStatus mdns_name_registration_status() const { diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index 07c1060432..e622b62982 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -922,9 +922,7 @@ void TurnPort::Close() { // Stop the port from creating new connections. state_ = STATE_DISCONNECTED; // Delete all existing connections; stop sending data. - for (auto kv : connections()) { - kv.second->Destroy(); - } + DestroyAllConnections(); SignalTurnPortClosed(this); } @@ -1272,10 +1270,6 @@ void TurnPort::HandleConnectionDestroyed(Connection* conn) { const rtc::SocketAddress& remote_address = conn->remote_candidate().address(); TurnEntry* entry = FindEntry(remote_address); RTC_DCHECK(entry != NULL); - ScheduleEntryDestruction(entry); -} - -void TurnPort::ScheduleEntryDestruction(TurnEntry* entry) { RTC_DCHECK(!entry->destruction_timestamp().has_value()); int64_t timestamp = rtc::TimeMillis(); entry->set_destruction_timestamp(timestamp); diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index 7b8e3b9af9..891af7605a 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -361,7 +361,6 @@ class TurnPort : public Port { // Destroys the entry only if `timestamp` matches the destruction timestamp // in `entry`. void DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp); - void ScheduleEntryDestruction(TurnEntry* entry); // Marks the connection with remote address `address` failed and // pruned (a.k.a. write-timed-out). Returns true if a connection is found.