Skip to content

Commit 88933ed

Browse files
fix: address Copilot review comments and Windows build failure
- iovec/WSABUF: on Windows iov_base is CHAR* (not void*) and field order in WSABUF is reversed vs POSIX iovec, so replace void* casts with char* casts and switch aggregate-init to named-field assignment - Discovery: update peer name and subscriptions before copying join_info so the NetworkJoin callback receives complete peer data in the handshake-before-announce path - Discovery: set response_flags = SYN for newly discovered peers so the connection handshake starts immediately on first announce rather than waiting for a subsequent announce - Reliability: validate packet_count before measuring RTT so malformed or stale ACKs cannot poison the retransmission timeout estimator - NUClearNet: zero source before each recvfrom so stale bytes from a previous datagram of a different address family cannot corrupt the fragmentation assembly key
1 parent 86b78ea commit 88933ed

4 files changed

Lines changed: 32 additions & 26 deletions

File tree

src/nuclearnet/Discovery.cpp

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,9 @@
165165

166166
auto it = peers.find(source);
167167
if (it == peers.end()) {
168-
// New peer — record with announce_heard = true
169-
announce_result.is_new = true;
168+
// New peer — record with announce_heard = true and initiate handshake
169+
announce_result.is_new = true;
170+
announce_result.response_flags = SYN;
170171
PeerInfo info;
171172
info.name = name;
172173
info.address = source;
@@ -180,15 +181,6 @@
180181
auto& peer = it->second;
181182
peer.last_seen = now;
182183

183-
// Mark announce as heard (may trigger connection if data was already confirmed)
184-
if (!peer.announce_heard) {
185-
peer.announce_heard = true;
186-
if (peer.handshake == HandshakeState::CONFIRMED) {
187-
fire_join = true;
188-
join_info = peer;
189-
}
190-
}
191-
192184
// Update name if it was unknown (peer added via CONNECT before announce)
193185
if (peer.name.empty()) {
194186
peer.name = name;
@@ -200,6 +192,16 @@
200192
subs_changed = true;
201193
}
202194

195+
// Mark announce as heard (may trigger connection if data was already confirmed)
196+
// Name and subscriptions are updated first so join_info carries complete data
197+
if (!peer.announce_heard) {
198+
peer.announce_heard = true;
199+
if (peer.handshake == HandshakeState::CONFIRMED) {
200+
fire_join = true;
201+
join_info = peer;
202+
}
203+
}
204+
203205
// Determine retransmit flags based on handshake state
204206
switch (peer.handshake) {
205207
case HandshakeState::IDLE:

src/nuclearnet/NUClearNet.cpp

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -268,10 +268,11 @@ namespace network {
268268
header.flags = req.flags;
269269
header.hash = req.hash;
270270

271-
std::array<iovec, 2> iov{{
272-
{reinterpret_cast<void*>(&header), sizeof(DataPacket) - 1}, // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
273-
{const_cast<void*>(static_cast<const void*>(req.data.data())), req.data.size()}, // NOLINT(cppcoreguidelines-pro-type-const-cast)
274-
}};
271+
std::array<iovec, 2> iov{};
272+
iov[0].iov_base = reinterpret_cast<char*>(&header); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
273+
iov[0].iov_len = static_cast<decltype(iov[0].iov_len)>(sizeof(DataPacket) - 1);
274+
iov[1].iov_base = const_cast<char*>(reinterpret_cast<const char*>(req.data.data())); // NOLINT(cppcoreguidelines-pro-type-const-cast,cppcoreguidelines-pro-type-reinterpret-cast)
275+
iov[1].iov_len = static_cast<decltype(iov[1].iov_len)>(req.data.size());
275276

276277
send_iov(data_fd, req.target, iov.data(), static_cast<int>(iov.size()));
277278
}
@@ -320,10 +321,11 @@ namespace network {
320321
const std::size_t offset = static_cast<std::size_t>(i) * packet_mtu;
321322
const std::size_t frag_len = std::min(static_cast<std::size_t>(packet_mtu), length - offset);
322323

323-
std::array<iovec, 2> iov{{
324-
{reinterpret_cast<void*>(&header), sizeof(DataPacket) - 1}, // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
325-
{const_cast<void*>(static_cast<const void*>(payload + offset)), frag_len}, // NOLINT(cppcoreguidelines-pro-type-const-cast)
326-
}};
324+
std::array<iovec, 2> iov{};
325+
iov[0].iov_base = reinterpret_cast<char*>(&header); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)
326+
iov[0].iov_len = static_cast<decltype(iov[0].iov_len)>(sizeof(DataPacket) - 1);
327+
iov[1].iov_base = const_cast<char*>(reinterpret_cast<const char*>(payload + offset)); // NOLINT(cppcoreguidelines-pro-type-const-cast,cppcoreguidelines-pro-type-reinterpret-cast)
328+
iov[1].iov_len = static_cast<decltype(iov[1].iov_len)>(frag_len);
327329

328330
send_iov(data_fd, dest, iov.data(), static_cast<int>(iov.size()));
329331
}
@@ -440,6 +442,7 @@ namespace network {
440442
// For the announce socket, MSG_DONTWAIT provides the same behavior on POSIX
441443
socklen_t source_len = 0;
442444
auto recv = [&]() {
445+
source = {}; // Clear stale bytes so different address families don't corrupt the assembly key
443446
source_len = sizeof(source.storage);
444447
return ::recvfrom(fd,
445448
reinterpret_cast<char*>(buffer.data()),

src/nuclearnet/NUClearNet.hpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,8 +198,8 @@ namespace network {
198198
/// Send a single contiguous buffer to a target
199199
void send_buf(fd_t fd, const sock_t& target, const uint8_t* data, std::size_t length) {
200200
std::array<iovec, 1> iov{};
201-
iov[0].iov_base = const_cast<void*>(static_cast<const void*>(data)); // NOLINT(cppcoreguidelines-pro-type-const-cast)
202-
iov[0].iov_len = length;
201+
iov[0].iov_base = const_cast<char*>(reinterpret_cast<const char*>(data)); // NOLINT(cppcoreguidelines-pro-type-const-cast,cppcoreguidelines-pro-type-reinterpret-cast)
202+
iov[0].iov_len = static_cast<decltype(iov[0].iov_len)>(length);
203203
send_iov(fd, target, iov.data(), 1);
204204
}
205205

src/nuclearnet/Reliability.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,18 +74,19 @@
7474

7575
auto& tp = it->second;
7676

77+
// Validate that the ACK's packet_count matches our tracked packet before measuring RTT
78+
// to prevent malformed/stale ACKs from poisoning the estimator
79+
if (packet_count != tp.packet_count) {
80+
return;
81+
}
82+
7783
// Update RTT estimate based on time since last send
7884
auto rtt = now - tp.last_send;
7985
{
8086
const std::lock_guard<std::mutex> rtt_lock(rtt_mutex);
8187
rtt_estimators[source].measure(rtt);
8288
}
8389

84-
// Validate that the ACK's packet_count matches our tracked packet
85-
if (packet_count != tp.packet_count) {
86-
return;
87-
}
88-
8990
// Update acked bitset
9091
bool all_acked = true;
9192
for (uint16_t i = 0; i < packet_count && i < tp.acked.size(); ++i) {

0 commit comments

Comments
 (0)