admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "p2p: separate ICE tie breaker and foundation seed"

Browse Source 
This reverts commit d99499abbae94793a02944a1f28f7015816447f5.

Reason for revert: Breaks downstream projects and I can also repro locally when running the rtc_unittest test target (it does however pass in isolation indicating test cleanup/setup needs to be fixed)

Original change's description:
> p2p: separate ICE tie breaker and foundation seed
>
> BUG=webrtc:14626
>
> Change-Id: I189a708192c9cef0b50c3fcbe798b30376d3b547
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/338982
> Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#41806}

Bug: webrtc:14626
Change-Id: If45f8a33395c562c9388b3d3748e8566efa87ecb
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/341081
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Jeremy Leconte <jleconte@google.com>
Commit-Queue: Christoffer Dewerin <jansson@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Owners-Override: Christoffer Dewerin <jansson@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41812}
This commit is contained in:
Christoffer Dewerin 2024-02-26 14:38:44 +00:00 committed by WebRTC LUCI CQ
parent 14613639bf
commit 7098d110e1
10 changed files with 16 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
api/candidate.cc
View File

@ -263,7 +263,7 @@ Candidate Candidate::ToSanitizedCopy(bool use_hostname_address,
}
void Candidate::ComputeFoundation(const rtc::SocketAddress& base_address,
uint64_t seed) {
uint64_t tie_breaker) {
// https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3
// The foundation is an identifier, scoped within a session. Two candidates
// MUST have the same foundation ID when all of the following are true:
@ -282,9 +282,14 @@ void Candidate::ComputeFoundation(const rtc::SocketAddress& base_address,
rtc::StringBuilder sb;
sb << type_name() << base_address.ipaddr().ToString() << protocol_
<< relay_protocol_;
// The random seed is passed to prevent a reverse CRC32 lookup of the address
// based on the foundation.
sb << rtc::ToString(seed);
// https://www.rfc-editor.org/rfc/rfc5245#section-5.2
// [...] it is possible for both agents to mistakenly believe they are
// controlled or controlling. To resolve this, each agent MUST select a random
// number, called the tie-breaker, uniformly distributed between 0 and (2**64)
// - 1 (that is, a 64-bit positive integer). This number is used in
// connectivity checks to detect and repair this case [...]
sb << rtc::ToString(tie_breaker);
foundation_ = rtc::ToString(rtc::ComputeCrc32(sb.Release()));
}

5
api/candidate.h
View File

@ -254,9 +254,10 @@ class RTC_EXPORT Candidate {
// then the foundation will be different. Two candidate pairs with
// the same foundation pairs are likely to have similar network
// characteristics. Foundations are used in the frozen algorithm.
// A session wide (peerconnection) seed is applied to the foundation,
// A session wide (peerconnection) tie-breaker is applied to the foundation,
// adds additional randomness and must be the same for all candidates.
void ComputeFoundation(const rtc::SocketAddress& base_address, uint64_t seed);
void ComputeFoundation(const rtc::SocketAddress& base_address,
uint64_t tie_breaker);
// https://www.rfc-editor.org/rfc/rfc5245#section-7.2.1.3
// Call to populate the foundation field for a new peer reflexive remote

1
p2p/base/fake_port_allocator.h
View File

@ -128,7 +128,6 @@ class FakePortAllocatorSession : public PortAllocatorSession {
field_trials_));
RTC_DCHECK(port_);
port_->SetIceTiebreaker(allocator_->ice_tiebreaker());
port_->SetFoundationSeed(allocator_->foundation_seed());
port_->SubscribePortDestroyed(
[this](PortInterface* port) { OnPortDestroyed(port); });
AddPort(port_.get());

12
p2p/base/port.cc
View File

@ -199,14 +199,6 @@ uint64_t Port::IceTiebreaker() const {
return tiebreaker_;
}
void Port::SetFoundationSeed(uint64_t foundation_seed) {
foundation_seed_ = foundation_seed;
}
uint64_t Port::FoundationSeed() const {
return foundation_seed_;
}
bool Port::SharedSocket() const {
return shared_socket_;
}
@ -263,8 +255,8 @@ void Port::AddAddress(const rtc::SocketAddress& address,
type, generation_, "", network_->id(), network_cost_);
// Set the relay protocol before computing the foundation field.
c.set_relay_protocol(relay_protocol);
// TODO(bugs.webrtc.org/14605): ensure FoundationSeed() is set.
c.ComputeFoundation(base_address, foundation_seed_);
// TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set.
c.ComputeFoundation(base_address, tiebreaker_);
c.set_priority(
c.GetPriority(type_preference, network_->preference(), relay_preference,

4
p2p/base/port.h
View File

@ -211,9 +211,6 @@ class RTC_EXPORT Port : public PortInterface, public sigslot::has_slots<> {
void SetIceTiebreaker(uint64_t tiebreaker) override;
uint64_t IceTiebreaker() const override;
void SetFoundationSeed(uint64_t foundation_seed) override;
uint64_t FoundationSeed() const override;
bool SharedSocket() const override;
void ResetSharedSocket() { shared_socket_ = false; }
@ -497,7 +494,6 @@ class RTC_EXPORT Port : public PortInterface, public sigslot::has_slots<> {
bool enable_port_packets_;
IceRole ice_role_;
uint64_t tiebreaker_;
uint64_t foundation_seed_;
bool shared_socket_;
// Information to use when going through a proxy.
std::string user_agent_;

3
p2p/base/port_allocator.cc
View File

@ -100,8 +100,7 @@ PortAllocator::PortAllocator()
step_delay_(kDefaultStepDelay),
allow_tcp_listen_(true),
candidate_filter_(CF_ALL),
tiebreaker_(rtc::CreateRandomId64()),
foundation_seed_(rtc::CreateRandomId64()) {
tiebreaker_(rtc::CreateRandomId64()) {
// The allocator will be attached to a thread in Initialize.
thread_checker_.Detach();
}

9
p2p/base/port_allocator.h
View File

@ -452,7 +452,6 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> {
Candidate SanitizeCandidate(const Candidate& c) const;
uint64_t ice_tiebreaker() const { return tiebreaker_; }
uint64_t foundation_seed() const { return foundation_seed_; }
uint32_t flags() const {
CheckRunOnValidThreadIfInitialized();
@ -665,14 +664,8 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> {
std::vector<std::unique_ptr<PortAllocatorSession>>::const_iterator
FindPooledSession(const IceParameters* ice_credentials = nullptr) const;
// ICE Tie-breaker as described in
// https://www.rfc-editor.org/rfc/rfc5245#section-5.2
// ICE tie breaker.
uint64_t tiebreaker_;
// A random value that is included in the foundation of all ICE candidates
// generated by ports derived from this allocator. See
// https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3
// Never sent over the network.
uint64_t foundation_seed_;
};
} // namespace cricket

5
p2p/base/port_interface.h
View File

@ -62,11 +62,6 @@ class PortInterface {
virtual void SetIceTiebreaker(uint64_t tiebreaker) = 0;
virtual uint64_t IceTiebreaker() const = 0;
// Methods to set/get the PortAllocator random value which is used to seed the
// foundation.
virtual void SetFoundationSeed(uint64_t foundation_seed) = 0;
virtual uint64_t FoundationSeed() const = 0;
virtual bool SharedSocket() const = 0;
virtual bool SupportsProtocol(absl::string_view protocol) const = 0;

5
p2p/base/port_unittest.cc
View File

@ -109,7 +109,6 @@ const uint32_t kDefaultPrflxPriority = ICE_TYPE_PREFERENCE_PRFLX << 24 |
constexpr int kTiebreaker1 = 11111;
constexpr int kTiebreaker2 = 22222;
constexpr int kTiebreakerDefault = 44444;
constexpr int kFoundationSeed = 42;
const char* data = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
@ -556,7 +555,6 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
username_, password_, true, absl::nullopt,
&field_trials_);
port->SetIceTiebreaker(kTiebreakerDefault);
port->SetFoundationSeed(kFoundationSeed);
return port;
}
@ -569,7 +567,6 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
MakeNetworkMultipleAddrs(global_addr, link_local_addr), 0, 0, username_,
password_, true, absl::nullopt, &field_trials_);
port->SetIceTiebreaker(kTiebreakerDefault);
port->SetFoundationSeed(kFoundationSeed);
return port;
}
std::unique_ptr<TCPPort> CreateTcpPort(const SocketAddress& addr) {
@ -580,7 +577,6 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
auto port = TCPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0,
username_, password_, true, &field_trials_);
port->SetIceTiebreaker(kTiebreakerDefault);
port->SetFoundationSeed(kFoundationSeed);
return port;
}
std::unique_ptr<StunPort> CreateStunPort(const SocketAddress& addr,
@ -591,7 +587,6 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
username_, password_, stun_servers,
absl::nullopt, &field_trials_);
port->SetIceTiebreaker(kTiebreakerDefault);
port->SetFoundationSeed(kFoundationSeed);
return port;
}
std::unique_ptr<Port> CreateRelayPort(const SocketAddress& addr,

4
p2p/client/basic_port_allocator.cc
View File

@ -1484,7 +1484,6 @@ void AllocationSequence::CreateUDPPorts() {
if (port) {
port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
port->SetFoundationSeed(session_->allocator()->foundation_seed());
// If shared socket is enabled, STUN candidate will be allocated by the
// UDPPort.
if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) {
@ -1521,7 +1520,6 @@ void AllocationSequence::CreateTCPPorts() {
session_->allocator()->field_trials());
if (port) {
port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
port->SetFoundationSeed(session_->allocator()->foundation_seed());
session_->AddAllocatedPort(port.release(), this);
// Since TCPPort is not created using shared socket, `port` will not be
// added to the dequeue.
@ -1552,7 +1550,6 @@ void AllocationSequence::CreateStunPorts() {
session_->allocator()->field_trials());
if (port) {
port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
port->SetFoundationSeed(session_->allocator()->foundation_seed());
session_->AddAllocatedPort(port.release(), this);
// Since StunPort is not created using shared socket, `port` will not be
// added to the dequeue.
@ -1656,7 +1653,6 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config,
}
RTC_DCHECK(port != NULL);
port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
port->SetFoundationSeed(session_->allocator()->foundation_seed());
session_->AddAllocatedPort(port.release(), this);
}
}