Skip to content

Commit 6f27438

Browse files
jasnelljasnell
committed
src: do not change the address map QUIC uses
Pairing SocketAddress::Hash with a matching Equal was done by changing SocketAddress::Map, which is shared. SocketAddressLRU is built on Map, and QUIC's address validation is built on SocketAddressLRU, so that changed which peers QUIC treats as the same peer: two addresses differing only in sin6_flowinfo or sin_zero padding stopped being distinct entries. There was no defect there to justify it. operator== is stricter than Hash, not looser: equal keys still hash equal, so unordered_map's invariant held. The mismatch only matters for a table where two datagrams from one peer must find one entry, which is a session table, which is DTLS. Map goes back to what it was, byte for byte. Equal stays, and a PeerMap alias pairs the two for callers that want peer identity rather than sockaddr identity. DTLS's session table is the only one. The cctest keeps both behaviours pinned. Signed-off-by: James M Snell <[email protected]> Assisted-by: Opencode
1 parent f9a0ea4 commit 6f27438

3 files changed

Lines changed: 32 additions & 21 deletions

File tree

‎src/dtls/dtls_endpoint.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,10 @@ class DTLSEndpoint final : public HandleWrap {
156156
// single buffer per endpoint avoids a heap allocation on every packet.
157157
std::vector<char> recv_buf_;
158158

159-
// Session table: maps remote address -> session. SocketAddress::Map pairs
160-
// Hash with the matching Equal; the default std::equal_to would compare
161-
// padding bytes that Hash ignores.
162-
SocketAddress::Map<BaseObjectPtr<DTLSSession>> sessions_;
159+
// Session table: remote address -> session. PeerMap and not Map: two
160+
// datagrams from one peer must find one session, and operator== would treat
161+
// them as different peers over a sin6_flowinfo the sender is free to vary.
162+
SocketAddress::PeerMap<BaseObjectPtr<DTLSSession>> sessions_;
163163

164164
// Concurrent sessions per source IP, ignoring port, so one host cannot fill
165165
// the whole table. Entries are erased when their count reaches zero, so this

‎src/node_sockaddr.h‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,11 @@ class SocketAddress : public MemoryRetainer {
2727
size_t operator()(const SocketAddress& addr) const;
2828
};
2929

30-
// Compares the same bytes Hash hashes: family, port and address. Pair this
31-
// with Hash rather than relying on operator==, which memcmps the whole
32-
// sockaddr including sin_zero, sin6_flowinfo and sin6_scope_id, and so can
33-
// report unequal for two addresses that hash the same.
30+
// Compares what identifies a peer: family, port, address, and scope_id for
31+
// IPv6. operator== memcmps the whole sockaddr, so it also distinguishes
32+
// sin6_flowinfo, which is a QoS label a peer may vary between datagrams,
33+
// and sin_zero, which is padding. Pair this with Hash where two datagrams
34+
// from one peer have to reach one entry.
3435
struct Equal {
3536
bool operator()(const SocketAddress& a, const SocketAddress& b) const;
3637
};
@@ -146,7 +147,13 @@ class SocketAddress : public MemoryRetainer {
146147
SET_SELF_SIZE(SocketAddress)
147148

148149
template <typename T>
149-
using Map = std::unordered_map<SocketAddress, T, Hash, Equal>;
150+
using Map = std::unordered_map<SocketAddress, T, Hash>;
151+
152+
// As Map, but keyed on what identifies a peer rather than on every byte of
153+
// the sockaddr. Separate from Map because Map is what SocketAddressLRU
154+
// uses, and QUIC's address validation counts entries in one of those.
155+
template <typename T>
156+
using PeerMap = std::unordered_map<SocketAddress, T, Hash, Equal>;
150157

151158
template <typename T>
152159
using IpMap = std::unordered_map<SocketAddress, T, IpHash, IpEqual>;

‎test/cctest/test_sockaddr.cc‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,12 @@ TEST(SocketAddress, SocketAddress) {
4646
TEST(SocketAddress, HashAndEqualAgree) {
4747
// Hash covers family, port and address. operator== memcmps the whole
4848
// sockaddr, so it also sees sin_zero, which is padding the kernel is not
49-
// obliged to zero for us. Pairing Hash with the default std::equal_to
50-
// therefore let one peer occupy two entries of the same hash bucket.
49+
// obliged to zero for us. Two datagrams from one peer can therefore compare
50+
// unequal, which for a session table means one peer with two sessions.
51+
//
52+
// Not a broken container: operator== is stricter than Hash, so equal keys
53+
// still hash equal and unordered_map's invariant holds. Map keeps that
54+
// pairing. PeerMap is for callers that need one peer to mean one entry.
5155
sockaddr_storage s1, s2;
5256
SocketAddress::ToSockAddr(AF_INET, "10.0.0.1", 443, &s1);
5357
SocketAddress::ToSockAddr(AF_INET, "10.0.0.1", 443, &s2);
@@ -68,16 +72,16 @@ TEST(SocketAddress, HashAndEqualAgree) {
6872
// Equal agrees with Hash.
6973
CHECK(equal(addr1, addr2));
7074

71-
// The old pairing, Hash with the default std::equal_to, is what
72-
// SocketAddress::Map used to expand to. It splits this single peer across
73-
// two entries of one bucket -- the defect being fixed.
74-
std::unordered_map<SocketAddress, size_t, SocketAddress::Hash> unfixed;
75-
unfixed[addr1]++;
76-
unfixed[addr2]++;
77-
CHECK_EQ(unfixed.size(), 2);
75+
// Map pairs Hash with the default std::equal_to, which splits this single
76+
// peer across two entries of one bucket. Unchanged deliberately: it is what
77+
// SocketAddressLRU uses, and QUIC counts entries in one of those.
78+
SocketAddress::Map<size_t> split;
79+
split[addr1]++;
80+
split[addr2]++;
81+
CHECK_EQ(split.size(), 2);
7882

79-
// The regression that matters: one key, not two.
80-
SocketAddress::Map<size_t> map;
83+
// PeerMap is the one that treats them as the peer they are.
84+
SocketAddress::PeerMap<size_t> map;
8185
map[addr1]++;
8286
map[addr2]++;
8387
CHECK_EQ(map.size(), 1);
@@ -108,7 +112,7 @@ TEST(SocketAddress, HashAndEqualAgreeIPv6) {
108112
CHECK_EQ(SocketAddress::Hash()(addr1), SocketAddress::Hash()(addr2));
109113
CHECK(equal(addr1, addr2));
110114

111-
SocketAddress::Map<size_t> map;
115+
SocketAddress::PeerMap<size_t> map;
112116
map[addr1]++;
113117
map[addr2]++;
114118
CHECK_EQ(map.size(), 1);

0 commit comments

Comments
 (0)