From 36792d4337bf2296ba38e4e62b17255e41930ce5 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 13 Oct 2022 14:19:25 -0300 Subject: [PATCH] Fix multi-field < ordering Lots and lots of places in the code had broken < operators because they are returning something like: foo < other.foo or bar < other.bar; but this breaks both the strict weak ordering requirements that are required for the "Compare" requirement for things like std::map/set/priority_queue. For example: a = {.foo=1, .bar=3} b = {.foo=3, .bar=1} does not have an ordering over a and b (both `a < b` and `b < a` are satisfied at the same time). This needs to be instead something like: foo < other.foo or (foo == other.foo and bar < other.bar) but that's a bit clunkier, and it is easier to use std::tie for tuple's built-in < comparison which does the right thing: std::tie(foo, bar) < std::tie(other.foo, other.bar) (Initially I noticed this in SockAddr/sockaddr_in6, but upon further investigation this extends to the major of multi-field `operator<`'s.) This fixes it by using std::tie (or something similar) everywhere we are doing multi-field inequalities. --- llarp/dns/srv_data.hpp | 14 +++-- llarp/iwp/linklayer.cpp | 2 +- llarp/iwp/linklayer.hpp | 2 +- llarp/iwp/message_buffer.hpp | 5 +- llarp/link/server.cpp | 2 +- llarp/link/server.hpp | 8 ++- llarp/net/address_info.cpp | 2 +- llarp/net/ip_range.hpp | 4 +- llarp/net/net.hpp | 116 +++++++++++++++++++---------------- llarp/net/sock_addr.cpp | 6 +- llarp/net/traffic_policy.hpp | 6 +- llarp/net/uint128.hpp | 8 +-- llarp/router_version.hpp | 2 +- llarp/service/intro.hpp | 4 +- llarp/vpn/platform.hpp | 2 +- 15 files changed, 98 insertions(+), 85 deletions(-) diff --git a/llarp/dns/srv_data.hpp b/llarp/dns/srv_data.hpp index f61606322..58281ab8d 100644 --- a/llarp/dns/srv_data.hpp +++ b/llarp/dns/srv_data.hpp @@ -10,7 +10,7 @@ namespace llarp::dns { - typedef std::tuple SRVTuple; + using SRVTuple = std::tuple; struct SRVData { @@ -38,19 +38,23 @@ namespace llarp::dns SRVTuple toTuple() const; + auto + toTupleRef() const + { + return std::tie(service_proto, priority, weight, port, target); + } + /// so we can put SRVData in a std::set bool operator<(const SRVData& other) const { - return service_proto < other.service_proto or priority < other.priority - or weight < other.weight or port < other.port or target < other.target; + return toTupleRef() < other.toTupleRef(); } bool operator==(const SRVData& other) const { - return service_proto == other.service_proto and priority == other.priority - and weight == other.weight and port == other.port and target == other.target; + return toTupleRef() == other.toTupleRef(); } bool diff --git a/llarp/iwp/linklayer.cpp b/llarp/iwp/linklayer.cpp index 03e2259eb..6fb807058 100644 --- a/llarp/iwp/linklayer.cpp +++ b/llarp/iwp/linklayer.cpp @@ -26,7 +26,7 @@ namespace llarp::iwp , m_Inbound{allowInbound} {} - const char* + std::string_view LinkLayer::Name() const { return "iwp"; diff --git a/llarp/iwp/linklayer.hpp b/llarp/iwp/linklayer.hpp index 6d58b046c..82504edc7 100644 --- a/llarp/iwp/linklayer.hpp +++ b/llarp/iwp/linklayer.hpp @@ -35,7 +35,7 @@ namespace llarp::iwp std::shared_ptr NewOutboundSession(const RouterContact& rc, const AddressInfo& ai) override; - const char* + std::string_view Name() const override; uint16_t diff --git a/llarp/iwp/message_buffer.hpp b/llarp/iwp/message_buffer.hpp index 3ea352064..f0dae604c 100644 --- a/llarp/iwp/message_buffer.hpp +++ b/llarp/iwp/message_buffer.hpp @@ -53,11 +53,12 @@ namespace llarp uint16_t m_ResendPriority; bool - operator<(const OutboundMessage& msg) const + operator<(const OutboundMessage& other) const { // yes, the first order is reversed as higher means more important // second part is for queue order - return msg.m_ResendPriority < m_ResendPriority or m_MsgID < msg.m_MsgID; + int prioA = -m_ResendPriority, prioB = -other.m_ResendPriority; + return std::tie(prioA, m_MsgID) < std::tie(prioB, other.m_MsgID); } ILinkSession::Packet_t diff --git a/llarp/link/server.cpp b/llarp/link/server.cpp index 58924e4ad..72eaa7ff9 100644 --- a/llarp/link/server.cpp +++ b/llarp/link/server.cpp @@ -249,7 +249,7 @@ namespace llarp bool ILinkLayer::PickAddress(const RouterContact& rc, llarp::AddressInfo& picked) const { - std::string OurDialect = Name(); + auto OurDialect = Name(); for (const auto& addr : rc.addrs) { if (addr.dialect == OurDialect) diff --git a/llarp/link/server.hpp b/llarp/link/server.hpp index 0e7c86c89..f9aca894b 100644 --- a/llarp/link/server.hpp +++ b/llarp/link/server.hpp @@ -135,7 +135,7 @@ namespace llarp virtual void Stop(); - virtual const char* + virtual std::string_view Name() const = 0; util::StatusObject @@ -179,7 +179,7 @@ namespace llarp bool IsCompatable(const llarp::RouterContact& other) const { - const std::string us = Name(); + const auto us = Name(); for (const auto& ai : other.addrs) if (ai.dialect == us) return true; @@ -207,7 +207,9 @@ namespace llarp bool operator<(const ILinkLayer& other) const { - return Rank() < other.Rank() || Name() < other.Name() || m_ourAddr < other.m_ourAddr; + auto rankA = Rank(), rankB = other.Rank(); + auto nameA = Name(), nameB = other.Name(); + return std::tie(rankA, nameA, m_ourAddr) < std::tie(rankB, nameB, other.m_ourAddr); } /// called by link session to remove a pending session who is timed out diff --git a/llarp/net/address_info.cpp b/llarp/net/address_info.cpp index fd5424632..f39b1d4c9 100644 --- a/llarp/net/address_info.cpp +++ b/llarp/net/address_info.cpp @@ -23,7 +23,7 @@ namespace llarp bool operator<(const AddressInfo& lhs, const AddressInfo& rhs) { - return lhs.rank < rhs.rank || lhs.ip < rhs.ip || lhs.port < rhs.port; + return std::tie(lhs.rank, lhs.ip, lhs.port) < std::tie(rhs.rank, rhs.ip, rhs.port); } std::variant diff --git a/llarp/net/ip_range.hpp b/llarp/net/ip_range.hpp index 9772793aa..a4e9b0be3 100644 --- a/llarp/net/ip_range.hpp +++ b/llarp/net/ip_range.hpp @@ -117,8 +117,8 @@ namespace llarp bool operator<(const IPRange& other) const { - return (this->addr & this->netmask_bits) < (other.addr & other.netmask_bits) - || this->netmask_bits < other.netmask_bits; + auto maskedA = addr & netmask_bits, maskedB = other.addr & other.netmask_bits; + return std::tie(maskedA, netmask_bits) < std::tie(maskedB, other.netmask_bits); } bool diff --git a/llarp/net/net.hpp b/llarp/net/net.hpp index e54a7ee8e..b3a7af8e4 100644 --- a/llarp/net/net.hpp +++ b/llarp/net/net.hpp @@ -34,62 +34,16 @@ namespace llarp { - inline bool - operator==(const in_addr& a, const in_addr& b) + inline int + cmp(const in_addr& a, const in_addr& b) { - return memcmp(&a, &b, sizeof(in_addr)) == 0; + return memcmp(&a, &b, sizeof(in_addr)); } - inline bool - operator==(const in6_addr& a, const in6_addr& b) + inline int + cmp(const in6_addr& a, const in6_addr& b) { - return memcmp(&a, &b, sizeof(in6_addr)) == 0; - } - - inline bool - operator==(const sockaddr_in& a, const sockaddr_in& b) - { - return a.sin_port == b.sin_port and a.sin_addr.s_addr == b.sin_addr.s_addr; - } - - inline bool - operator==(const sockaddr_in6& a, const sockaddr_in6& b) - { - return a.sin6_port == b.sin6_port and a.sin6_addr == b.sin6_addr; - } - - inline bool - operator==(const sockaddr& a, const sockaddr& b) - { - if (a.sa_family != b.sa_family) - return false; - switch (a.sa_family) - { - case AF_INET: - return reinterpret_cast(a) == reinterpret_cast(b); - case AF_INET6: - return reinterpret_cast(a) == reinterpret_cast(b); - default: - return false; - } - } - - inline bool - operator<(const in_addr& a, const in_addr& b) - { - return memcmp(&a, &b, sizeof(in_addr)) < 0; - } - - inline bool - operator<(const in6_addr& a, const in6_addr& b) - { - return memcmp(&a, &b, sizeof(in6_addr)) < 0; - } - - inline bool - operator<(const sockaddr_in6& a, const sockaddr_in6& b) - { - return a.sin6_addr < b.sin6_addr or a.sin6_port < b.sin6_port; + return memcmp(&a, &b, sizeof(in6_addr)); } namespace net @@ -261,3 +215,61 @@ namespace llarp } // namespace net } // namespace llarp + +inline bool +operator==(const in_addr& a, const in_addr& b) +{ + return llarp::cmp(a, b) == 0; +} + +inline bool +operator==(const in6_addr& a, const in6_addr& b) +{ + return llarp::cmp(a, b) == 0; +} + +inline bool +operator==(const sockaddr_in& a, const sockaddr_in& b) +{ + return a.sin_port == b.sin_port and a.sin_addr.s_addr == b.sin_addr.s_addr; +} + +inline bool +operator==(const sockaddr_in6& a, const sockaddr_in6& b) +{ + return a.sin6_port == b.sin6_port and a.sin6_addr == b.sin6_addr; +} + +inline bool +operator==(const sockaddr& a, const sockaddr& b) +{ + if (a.sa_family != b.sa_family) + return false; + switch (a.sa_family) + { + case AF_INET: + return reinterpret_cast(a) == reinterpret_cast(b); + case AF_INET6: + return reinterpret_cast(a) == reinterpret_cast(b); + default: + return false; + } +} + +inline bool +operator<(const in_addr& a, const in_addr& b) +{ + return llarp::cmp(a, b) < 0; +} + +inline bool +operator<(const in6_addr& a, const in6_addr& b) +{ + return llarp::cmp(a, b) < 0; +} + +inline bool +operator<(const sockaddr_in6& a, const sockaddr_in6& b) +{ + return std::tie(a.sin6_addr, a.sin6_port) < std::tie(b.sin6_addr, b.sin6_port); +} diff --git a/llarp/net/sock_addr.cpp b/llarp/net/sock_addr.cpp index f42feb928..8fdf457f1 100644 --- a/llarp/net/sock_addr.cpp +++ b/llarp/net/sock_addr.cpp @@ -206,15 +206,13 @@ namespace llarp bool SockAddr::operator<(const SockAddr& other) const { - return (m_addr.sin6_addr < other.m_addr.sin6_addr) - or (m_addr.sin6_port < other.m_addr.sin6_port); + return m_addr < other.m_addr; } bool SockAddr::operator==(const SockAddr& other) const { - return m_addr.sin6_addr == other.m_addr.sin6_addr - and m_addr.sin6_port == other.m_addr.sin6_port; + return m_addr == other.m_addr; } huint128_t diff --git a/llarp/net/traffic_policy.hpp b/llarp/net/traffic_policy.hpp index 1368d05d4..a34456535 100644 --- a/llarp/net/traffic_policy.hpp +++ b/llarp/net/traffic_policy.hpp @@ -33,11 +33,7 @@ namespace llarp::net bool operator<(const ProtocolInfo& other) const { - if (port and other.port) - { - return protocol < other.protocol or *port < *other.port; - } - return protocol < other.protocol; + return std::tie(protocol, port) < std::tie(other.protocol, other.port); } ProtocolInfo() = default; diff --git a/llarp/net/uint128.hpp b/llarp/net/uint128.hpp index 4bc39f53c..dde7c83af 100644 --- a/llarp/net/uint128.hpp +++ b/llarp/net/uint128.hpp @@ -141,25 +141,25 @@ namespace llarp constexpr bool operator<(const uint128_t& b) const { - return upper < b.upper || (upper == b.upper && lower < b.lower); + return std::tie(upper, lower) < std::tie(b.upper, b.lower); } constexpr bool operator<=(const uint128_t& b) const { - return upper < b.upper || (upper == b.upper && lower <= b.lower); + return std::tie(upper, lower) <= std::tie(b.upper, b.lower); } constexpr bool operator>(const uint128_t& b) const { - return upper > b.upper || (upper == b.upper && lower > b.lower); + return std::tie(upper, lower) > std::tie(b.upper, b.lower); } constexpr bool operator>=(const uint128_t& b) const { - return upper > b.upper || (upper == b.upper && lower >= b.lower); + return std::tie(upper, lower) >= std::tie(b.upper, b.lower); } constexpr uint128_t& diff --git a/llarp/router_version.hpp b/llarp/router_version.hpp index 30429483b..a03cc02fc 100644 --- a/llarp/router_version.hpp +++ b/llarp/router_version.hpp @@ -41,7 +41,7 @@ namespace llarp bool operator<(const RouterVersion& other) const { - return m_ProtoVersion < other.m_ProtoVersion || m_Version < other.m_Version; + return std::tie(m_ProtoVersion, m_Version) < std::tie(other.m_ProtoVersion, other.m_Version); } bool diff --git a/llarp/service/intro.hpp b/llarp/service/intro.hpp index c1c431eb0..bb659a4d4 100644 --- a/llarp/service/intro.hpp +++ b/llarp/service/intro.hpp @@ -55,8 +55,8 @@ namespace llarp bool operator<(const Introduction& other) const { - return expiresAt < other.expiresAt || pathID < other.pathID || router < other.router - || version < other.version || latency < other.latency; + return std::tie(expiresAt, pathID, router, version, latency) + < std::tie(other.expiresAt, other.pathID, other.router, other.version, other.latency); } bool diff --git a/llarp/vpn/platform.hpp b/llarp/vpn/platform.hpp index 6d0b18063..ebba6dee8 100644 --- a/llarp/vpn/platform.hpp +++ b/llarp/vpn/platform.hpp @@ -25,7 +25,7 @@ namespace llarp::vpn bool operator<(const InterfaceAddress& other) const { - return range < other.range or fam < other.fam; + return std::tie(range, fam) < std::tie(other.range, other.fam); } };