diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index bea98cf029..fbbd85344b 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -536,7 +536,7 @@ void Connection::HandleBindingRequest(IceMessage* msg) { msg->reduced_transaction_id()); // This is a validated stun request from remote peer. - port_->SendBindingResponse(msg, remote_addr); + SendBindingResponse(msg); // If it timed out on writing check, start up again if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) { @@ -587,6 +587,72 @@ void Connection::HandleBindingRequest(IceMessage* msg) { } } +void Connection::SendBindingResponse(const StunMessage* request) { + RTC_DCHECK(request->type() == STUN_BINDING_REQUEST); + + // Where I send the response. + const rtc::SocketAddress& addr = remote_candidate_.address(); + + // Retrieve the username from the request. + const StunByteStringAttribute* username_attr = + request->GetByteString(STUN_ATTR_USERNAME); + RTC_DCHECK(username_attr != NULL); + if (username_attr == NULL) { + // No valid username, skip the response. + return; + } + + // Fill in the response message. + StunMessage response; + response.SetType(STUN_BINDING_RESPONSE); + response.SetTransactionID(request->transaction_id()); + const StunUInt32Attribute* retransmit_attr = + request->GetUInt32(STUN_ATTR_RETRANSMIT_COUNT); + if (retransmit_attr) { + // Inherit the incoming retransmit value in the response so the other side + // can see our view of lost pings. + response.AddAttribute(std::make_unique( + STUN_ATTR_RETRANSMIT_COUNT, retransmit_attr->value())); + + if (retransmit_attr->value() > CONNECTION_WRITE_CONNECT_FAILURES) { + RTC_LOG(LS_INFO) + << ToString() + << ": Received a remote ping with high retransmit count: " + << retransmit_attr->value(); + } + } + + response.AddAttribute(std::make_unique( + STUN_ATTR_XOR_MAPPED_ADDRESS, addr)); + response.AddMessageIntegrity(local_candidate().password()); + response.AddFingerprint(); + + // Send the response message. + rtc::ByteBufferWriter buf; + response.Write(&buf); + rtc::PacketOptions options(port_->StunDscpValue()); + options.info_signaled_after_sent.packet_type = + rtc::PacketType::kIceConnectivityCheckResponse; + auto err = port_->SendTo(buf.Data(), buf.Length(), addr, options, false); + if (err < 0) { + RTC_LOG(LS_ERROR) << ToString() + << ": Failed to send STUN ping response, to=" + << addr.ToSensitiveString() << ", err=" << err + << ", id=" << rtc::hex_encode(response.transaction_id()); + } else { + // Log at LS_INFO if we send a stun ping response on an unwritable + // connection. + rtc::LoggingSeverity sev = (!writable()) ? rtc::LS_INFO : rtc::LS_VERBOSE; + RTC_LOG_V(sev) << ToString() << ": Sent STUN ping response, to=" + << addr.ToSensitiveString() + << ", id=" << rtc::hex_encode(response.transaction_id()); + + stats_.sent_ping_responses++; + LogCandidatePairEvent(webrtc::IceCandidatePairEventType::kCheckResponseSent, + request->reduced_transaction_id()); + } +} + void Connection::OnReadyToSend() { SignalReadyToSend(this); } diff --git a/p2p/base/connection.h b/p2p/base/connection.h index bc37429cda..39066f406f 100644 --- a/p2p/base/connection.h +++ b/p2p/base/connection.h @@ -184,13 +184,6 @@ class Connection : public CandidatePairInterface, // a nomination value. The controlling agent gets its |acked_nomination_| set // when receiving a response to a nominating ping. bool nominated() const { return acked_nomination_ || remote_nomination_; } - // Public for unit tests. - void set_remote_nomination(uint32_t remote_nomination) { - remote_nomination_ = remote_nomination; - } - // Public for unit tests. - uint32_t acked_nomination() const { return acked_nomination_; } - void set_remote_ice_mode(IceMode mode) { remote_ice_mode_ = mode; } int receiving_timeout() const; @@ -300,13 +293,23 @@ class Connection : public CandidatePairInterface, // Check if we sent |val| pings without receving a response. bool TooManyOutstandingPings(const absl::optional& val) const; + void SetIceFieldTrials(const IceFieldTrials* field_trials); + const rtc::EventBasedExponentialMovingAverage& GetRttEstimate() const { + return rtt_estimate_; + } + + void SendBindingResponse(const StunMessage* request); + // An accessor for unit tests. Port* PortForTest() { return port_; } const Port* PortForTest() const { return port_; } - void SetIceFieldTrials(const IceFieldTrials* field_trials); - const rtc::EventBasedExponentialMovingAverage& GetRttEstimate() const { - return rtt_estimate_; + // Public for unit tests. + uint32_t acked_nomination() const { return acked_nomination_; } + + // Public for unit tests. + void set_remote_nomination(uint32_t remote_nomination) { + remote_nomination_ = remote_nomination; } protected: diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h index ab61c802fb..4fafb542b3 100644 --- a/p2p/base/fake_port_allocator.h +++ b/p2p/base/fake_port_allocator.h @@ -48,15 +48,6 @@ class TestUDPPort : public UDPPort { } return port; } - void SendBindingResponse(StunMessage* request, - const rtc::SocketAddress& addr) override { - UDPPort::SendBindingResponse(request, addr); - sent_binding_response_ = true; - } - bool sent_binding_response() { return sent_binding_response_; } - void set_sent_binding_response(bool response) { - sent_binding_response_ = response; - } protected: TestUDPPort(rtc::Thread* thread, @@ -77,8 +68,6 @@ class TestUDPPort : public UDPPort { password, origin, emit_localhost_for_anyaddress) {} - - bool sent_binding_response_ = false; }; // A FakePortAllocatorSession can be used with either a real or fake socket diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 9cc58accbd..ac28d46748 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -3808,11 +3808,10 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { &request, kIceUfrag[1], false); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); ASSERT_TRUE(conn1 != nullptr); - EXPECT_TRUE(port->sent_binding_response()); + EXPECT_EQ(conn1->stats().sent_ping_responses, 1u); EXPECT_NE(conn1, ch.selected_connection()); conn1->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout); - port->set_sent_binding_response(false); // Another connection is nominated via use_candidate. ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1)); @@ -3833,10 +3832,9 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { &request, kIceUfrag[1], false); Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); ASSERT_TRUE(conn3 != nullptr); - EXPECT_TRUE(port->sent_binding_response()); + EXPECT_EQ(conn3->stats().sent_ping_responses, 1u); conn3->ReceivedPingResponse(LOW_RTT, "id"); // Become writable. EXPECT_EQ(conn2, ch.selected_connection()); - port->set_sent_binding_response(false); // However if the request contains use_candidate attribute, it will be // selected as the selected connection. @@ -3846,7 +3844,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { &request, kIceUfrag[1], false); Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4); ASSERT_TRUE(conn4 != nullptr); - EXPECT_TRUE(port->sent_binding_response()); + EXPECT_EQ(conn4->stats().sent_ping_responses, 1u); // conn4 is not the selected connection yet because it is not writable. EXPECT_EQ(conn2, ch.selected_connection()); conn4->ReceivedPingResponse(LOW_RTT, "id"); // Become writable. @@ -3854,14 +3852,14 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { // Test that the request from an unknown address contains a ufrag from an old // generation. - port->set_sent_binding_response(false); + // port->set_sent_binding_response(false); ch.SetRemoteIceParameters(kIceParams[2]); ch.SetRemoteIceParameters(kIceParams[3]); port->SignalUnknownAddress(port, rtc::SocketAddress("5.5.5.5", 5), PROTO_UDP, &request, kIceUfrag[2], false); Connection* conn5 = WaitForConnectionTo(&ch, "5.5.5.5", 5); ASSERT_TRUE(conn5 != nullptr); - EXPECT_TRUE(port->sent_binding_response()); + EXPECT_EQ(conn5->stats().sent_ping_responses, 1u); EXPECT_EQ(kIcePwd[2], conn5->remote_candidate().password()); } diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 742c15dbf8..b92f1226c0 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -661,73 +661,6 @@ bool Port::CanHandleIncomingPacketsFrom(const rtc::SocketAddress&) const { return false; } -void Port::SendBindingResponse(StunMessage* request, - const rtc::SocketAddress& addr) { - RTC_DCHECK(request->type() == STUN_BINDING_REQUEST); - - // Retrieve the username from the request. - const StunByteStringAttribute* username_attr = - request->GetByteString(STUN_ATTR_USERNAME); - RTC_DCHECK(username_attr != NULL); - if (username_attr == NULL) { - // No valid username, skip the response. - return; - } - - // Fill in the response message. - StunMessage response; - response.SetType(STUN_BINDING_RESPONSE); - response.SetTransactionID(request->transaction_id()); - const StunUInt32Attribute* retransmit_attr = - request->GetUInt32(STUN_ATTR_RETRANSMIT_COUNT); - if (retransmit_attr) { - // Inherit the incoming retransmit value in the response so the other side - // can see our view of lost pings. - response.AddAttribute(std::make_unique( - STUN_ATTR_RETRANSMIT_COUNT, retransmit_attr->value())); - - if (retransmit_attr->value() > CONNECTION_WRITE_CONNECT_FAILURES) { - RTC_LOG(LS_INFO) - << ToString() - << ": Received a remote ping with high retransmit count: " - << retransmit_attr->value(); - } - } - - response.AddAttribute(std::make_unique( - STUN_ATTR_XOR_MAPPED_ADDRESS, addr)); - response.AddMessageIntegrity(password_); - response.AddFingerprint(); - - // Send the response message. - rtc::ByteBufferWriter buf; - response.Write(&buf); - rtc::PacketOptions options(StunDscpValue()); - options.info_signaled_after_sent.packet_type = - rtc::PacketType::kIceConnectivityCheckResponse; - auto err = SendTo(buf.Data(), buf.Length(), addr, options, false); - if (err < 0) { - RTC_LOG(LS_ERROR) << ToString() - << ": Failed to send STUN ping response, to=" - << addr.ToSensitiveString() << ", err=" << err - << ", id=" << rtc::hex_encode(response.transaction_id()); - } else { - // Log at LS_INFO if we send a stun ping response on an unwritable - // connection. - Connection* conn = GetConnection(addr); - rtc::LoggingSeverity sev = - (conn && !conn->writable()) ? rtc::LS_INFO : rtc::LS_VERBOSE; - RTC_LOG_V(sev) << ToString() << ": Sent STUN ping response, to=" - << addr.ToSensitiveString() - << ", id=" << rtc::hex_encode(response.transaction_id()); - - conn->stats_.sent_ping_responses++; - conn->LogCandidatePairEvent( - webrtc::IceCandidatePairEventType::kCheckResponseSent, - request->reduced_transaction_id()); - } -} - void Port::SendBindingErrorResponse(StunMessage* request, const rtc::SocketAddress& addr, int error_code, diff --git a/p2p/base/port.h b/p2p/base/port.h index d6099222fa..84340e831a 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -287,11 +287,7 @@ class Port : public PortInterface, virtual bool CanHandleIncomingPacketsFrom( const rtc::SocketAddress& remote_addr) const; - // Sends a response message (normal or error) to the given request. One of - // these methods should be called as a response to SignalUnknownAddress. - // NOTE: You MUST call CreateConnection BEFORE SendBindingResponse. - void SendBindingResponse(StunMessage* request, - const rtc::SocketAddress& addr) override; + // Sends a response error to the given request. void SendBindingErrorResponse(StunMessage* request, const rtc::SocketAddress& addr, int error_code, diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h index 24f2e2afa4..39eae18a0d 100644 --- a/p2p/base/port_interface.h +++ b/p2p/base/port_interface.h @@ -105,9 +105,6 @@ class PortInterface { // Sends a response message (normal or error) to the given request. One of // these methods should be called as a response to SignalUnknownAddress. - // NOTE: You MUST call CreateConnection BEFORE SendBindingResponse. - virtual void SendBindingResponse(StunMessage* request, - const rtc::SocketAddress& addr) = 0; virtual void SendBindingErrorResponse(StunMessage* request, const rtc::SocketAddress& addr, int error_code, diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index ec2a872acd..55ff5be5ad 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -302,7 +302,7 @@ class TestChannel : public sigslot::has_slots<> { c.set_address(remote_address_); conn_ = port_->CreateConnection(c, Port::ORIGIN_MESSAGE); conn_->SignalDestroyed.connect(this, &TestChannel::OnDestroyed); - port_->SendBindingResponse(remote_request_.get(), remote_address_); + conn_->SendBindingResponse(remote_request_.get()); remote_request_.reset(); } void Ping() { Ping(0); } @@ -2618,11 +2618,10 @@ TEST_F(PortTest, TestIceLiteConnectivity) { // NOTE: Ideally we should't create connection at this stage from lite // port, as it should be done only after receiving ping with USE_CANDIDATE. // But we need a connection to send a response message. - ice_lite_port->CreateConnection(ice_full_port_ptr->Candidates()[0], - cricket::Port::ORIGIN_MESSAGE); + auto* con = ice_lite_port->CreateConnection( + ice_full_port_ptr->Candidates()[0], cricket::Port::ORIGIN_MESSAGE); std::unique_ptr request = CopyStunMessage(*msg); - ice_lite_port->SendBindingResponse( - request.get(), ice_full_port_ptr->Candidates()[0].address()); + con->SendBindingResponse(request.get()); // Feeding the respone message from litemode to the full mode connection. ch1.conn()->OnReadPacket(ice_lite_port->last_stun_buf()->data(),