From 13f9c62ec85cb8d9c1bbe6abee2e8678fb7c4e52 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Fri, 29 Apr 2022 16:38:32 +0200 Subject: [PATCH] Fix comparator bugs which are not compliant to strict weak ordering. See a full explanation of the problem on this blog [1] post about changing std::sort in LLVM and relative issues uncovered. The CompareNetwork function was violating the 4th rule of "strict weak ordering" (Transitivity of incomparability: x == y and y == z imply x == z, where x == y means x < y and y < x are both false). [1] - https://danlark.org/2022/04/20/changing-stdsort-at-googles-scale-and-beyond/ Bug: None Change-Id: I7e893f0a30da31403766284823f75c45c4db91c3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251681 Reviewed-by: Tomas Gunnarsson Reviewed-by: Niels Moller Reviewed-by: Harald Alvestrand Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/main@{#36711} --- rtc_base/network.cc | 25 +++++----- rtc_base/network.h | 5 ++ rtc_base/network_unittest.cc | 89 ++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 11 deletions(-) diff --git a/rtc_base/network.cc b/rtc_base/network.cc index 7a1266c8c0..c6442b1b63 100644 --- a/rtc_base/network.cc +++ b/rtc_base/network.cc @@ -63,16 +63,6 @@ struct AddressList { std::vector ips; }; -bool CompareNetworks(const std::unique_ptr& a, - const std::unique_ptr& b) { - if (a->prefix_length() == b->prefix_length()) { - if (a->name() == b->name()) { - return a->prefix() < b->prefix(); - } - } - return a->name() < b->name(); -} - bool SortNetworks(const Network* a, const Network* b) { // Network types will be preferred above everything else while sorting // Networks. @@ -195,6 +185,19 @@ const char kPublicIPv4Host[] = "8.8.8.8"; const char kPublicIPv6Host[] = "2001:4860:4860::8888"; const int kPublicPort = 53; // DNS port. +namespace webrtc_network_internal { +bool CompareNetworks(const std::unique_ptr& a, + const std::unique_ptr& b) { + if (a->prefix_length() != b->prefix_length()) { + return a->prefix_length() < b->prefix_length(); + } + if (a->name() != b->name()) { + return a->name() < b->name(); + } + return a->prefix() < b->prefix(); +} +} // namespace webrtc_network_internal + std::string MakeNetworkKey(absl::string_view name, const IPAddress& prefix, int prefix_length) { @@ -337,7 +340,7 @@ void NetworkManagerBase::MergeNetworkList( // AddressList in this map will track IP addresses for all Networks // with the same key. std::map consolidated_address_list; - absl::c_sort(new_networks, CompareNetworks); + absl::c_sort(new_networks, rtc::webrtc_network_internal::CompareNetworks); // First, build a set of network-keys to the ipaddresses. for (auto& network : new_networks) { bool might_add_to_merged_list = false; diff --git a/rtc_base/network.h b/rtc_base/network.h index e801022d7c..ef6c4f06be 100644 --- a/rtc_base/network.h +++ b/rtc_base/network.h @@ -53,6 +53,11 @@ class Thread; // By default, ignore loopback interfaces on the host. const int kDefaultNetworkIgnoreMask = ADAPTER_TYPE_LOOPBACK; +namespace webrtc_network_internal { +bool CompareNetworks(const std::unique_ptr& a, + const std::unique_ptr& b); +} // namespace webrtc_network_internal + // Makes a string key for this network. Used in the network manager's maps. // Network objects are keyed on interface name, network prefix and the // length of that prefix. diff --git a/rtc_base/network_unittest.cc b/rtc_base/network_unittest.cc index 8f60de5563..bfb30b2c93 100644 --- a/rtc_base/network_unittest.cc +++ b/rtc_base/network_unittest.cc @@ -1533,4 +1533,93 @@ TEST_F(NetworkTest, HardcodedVpn) { EXPECT_FALSE(NetworkManagerBase::IsVpnMacAddress(nullptr)); } +TEST(CompareNetworks, IrreflexivityTest) { + // x < x is false + auto network = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network, network)); +} + +TEST(CompareNetworks, AsymmetryTest) { + // x < y and y < x cannot be both true + auto network_a = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_b = std::make_unique( + "test_eth1", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_a, network_b)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_b, network_a)); + + auto network_c = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345500U), 24); + auto network_d = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_c, network_d)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_d, network_c)); +} + +TEST(CompareNetworks, TransitivityTest) { + // x < y and y < z imply x < z + auto network_a = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_b = std::make_unique( + "test_eth1", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_c = std::make_unique( + "test_eth2", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_a, network_b)); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_b, network_c)); + + auto network_d = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_e = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345700U), 24); + auto network_f = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345800U), 24); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_d, network_e)); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_e, network_f)); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_d, network_f)); + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_a, network_c)); +} + +TEST(CompareNetworks, TransitivityOfIncomparabilityTest) { + // x == y and y == z imply x == z, + // where x == y means x < y and y < x are both false + auto network_a = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 23); + auto network_b = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_c = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345700U), 24); + + // network_a < network_b + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_a, network_b)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_b, network_a)); + + // network_b < network_c + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_b, network_c)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_c, network_b)); + + // network_a < network_c + EXPECT_TRUE(webrtc_network_internal::CompareNetworks(network_a, network_c)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_c, network_a)); + + auto network_d = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_e = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + auto network_f = std::make_unique( + "test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); + + // network_d == network_e + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_d, network_e)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_e, network_d)); + + // network_e == network_f + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_e, network_f)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_f, network_e)); + + // network_d == network_f + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_d, network_f)); + EXPECT_FALSE(webrtc_network_internal::CompareNetworks(network_f, network_d)); +} + } // namespace rtc