From 0d63943b583b1844b864e54f1560b87d70655a7b Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 00:31:14 +0100 Subject: [PATCH 01/13] Track connection half-open state --- src/network/connection.cpp | 22 +++++- src/network/connection.h | 18 ++++- src/network/connectionthreads.cpp | 118 +++++++++++++++++++++--------- src/network/connectionthreads.h | 1 + 4 files changed, 119 insertions(+), 40 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index b324ca68f36a..7b8bb91663e9 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -49,7 +49,7 @@ namespace con // TODO: Clean this up. #define LOG(a) a -#define PING_TIMEOUT 5.0 +#define PING_TIMEOUT 5.0f u16 BufferedPacket::getSeqnum() const { @@ -548,6 +548,15 @@ ConnectionCommandPtr ConnectionCommand::disconnect_peer(session_t peer_id) return c; } +ConnectionCommandPtr ConnectionCommand::resend_one(session_t peer_id) +{ + auto c = create(CONNCMD_RESEND_ONE); + c->peer_id = peer_id; + c->channelnum = 0; // must be same as createPeer + c->reliable = true; + return c; +} + ConnectionCommandPtr ConnectionCommand::send(session_t peer_id, u8 channelnum, NetworkPacket *pkt, bool reliable) { @@ -984,12 +993,12 @@ void UDPPeer::reportRTT(float rtt) bool UDPPeer::Ping(float dtime,SharedBuffer& data) { m_ping_timer += dtime; - if (m_ping_timer >= PING_TIMEOUT) + if (!isHalfOpen() && m_ping_timer >= PING_TIMEOUT) { // Create and send PING packet writeU8(&data[0], PACKET_TYPE_CONTROL); writeU8(&data[1], CONTROLTYPE_PING); - m_ping_timer = 0.0; + m_ping_timer = 0.0f; return true; } return false; @@ -1608,6 +1617,12 @@ void Connection::DisconnectPeer(session_t peer_id) putCommand(ConnectionCommand::disconnect_peer(peer_id)); } +void Connection::doResendOne(session_t peer_id) +{ + assert(peer_id != PEER_ID_INEXISTENT); + putCommand(ConnectionCommand::resend_one(peer_id)); +} + void Connection::sendAck(session_t peer_id, u8 channelnum, u16 seqnum) { assert(channelnum < CHANNEL_COUNT); // Pre-condition @@ -1634,6 +1649,7 @@ UDPPeer* Connection::createServerPeer(Address& address) } UDPPeer *peer = new UDPPeer(PEER_ID_SERVER, address, this); + peer->SetFullyOpen(); { MutexAutoLock lock(m_peers_mutex); diff --git a/src/network/connection.h b/src/network/connection.h index 3bd944c00efc..e8451e3d5b05 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -313,7 +313,8 @@ enum ConnectionCommandType{ CONNCMD_SEND, CONNCMD_SEND_TO_ALL, CONCMD_ACK, - CONCMD_CREATE_PEER + CONCMD_CREATE_PEER, + CONNCMD_RESEND_ONE }; struct ConnectionCommand; @@ -336,6 +337,7 @@ struct ConnectionCommand static ConnectionCommandPtr connect(Address address); static ConnectionCommandPtr disconnect(); static ConnectionCommandPtr disconnect_peer(session_t peer_id); + static ConnectionCommandPtr resend_one(session_t peer_id); static ConnectionCommandPtr send(session_t peer_id, u8 channelnum, NetworkPacket *pkt, bool reliable); static ConnectionCommandPtr ack(session_t peer_id, u8 channelnum, const Buffer &data); static ConnectionCommandPtr createPeer(session_t peer_id, const Buffer &data); @@ -520,6 +522,9 @@ class Peer { void ResetTimeout() {MutexAutoLock lock(m_exclusive_access_mutex); m_timeout_counter = 0.0; }; + bool isHalfOpen() const { return m_half_open; } + void SetFullyOpen() { m_half_open = false; } + bool isTimedOut(float timeout); unsigned int m_increment_packets_remaining = 0; @@ -590,6 +595,15 @@ class Peer { rttstats m_rtt; float m_last_rtt = -1.0f; + /* + Until the peer has communicated with us using their assigned peer id + the connection is considered half-open. + During this time we inhibit re-sending any reliables or pings. This + is to avoid spending too many resources on a potential DoS attack + and to make sure Minetest servers are not useful for UDP amplificiation. + */ + bool m_half_open = true; + // current usage count unsigned int m_usage = 0; @@ -736,6 +750,8 @@ class Connection void SetPeerID(session_t id) { m_peer_id = id; } + void doResendOne(session_t peer_id); + void sendAck(session_t peer_id, u8 channelnum, u16 seqnum); std::vector getPeerIDs() diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index 452a55929de9..14323edb4d0e 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -46,11 +46,11 @@ namespace con #define WINDOW_SIZE 5 -static session_t readPeerId(const u8 *packetdata) +static inline session_t readPeerId(const u8 *packetdata) { return readU16(&packetdata[4]); } -static u8 readChannel(const u8 *packetdata) +static inline u8 readChannel(const u8 *packetdata) { return readU8(&packetdata[6]); } @@ -220,29 +220,22 @@ void ConnectionSendThread::runTimeouts(float dtime) channel.UpdatePacketLossCounter(timed_outs.size()); g_profiler->graphAdd("packets_lost", timed_outs.size()); - m_iteration_packets_avaialble -= timed_outs.size(); - - for (const auto &k : timed_outs) { - u8 channelnum = readChannel(k->data); - u16 seqnum = k->getSeqnum(); - - channel.UpdateBytesLost(k->size()); - - LOG(derr_con << m_connection->getDesc() - << "RE-SENDING timed-out RELIABLE to " - << k->address.serializeString() - << "(t/o=" << resend_timeout << "): " - << "count=" << k->resend_count - << ", channel=" << ((int) channelnum & 0xff) - << ", seqnum=" << seqnum - << std::endl); - - rawSend(k.get()); - - // do not handle rtt here as we can't decide if this packet was - // lost or really takes more time to transmit + // Note that this only happens during connection setup, it would + // break badly otherwise. + if (peer->isHalfOpen()) { + if (!timed_outs.empty()) { + dout_con << m_connection->getDesc() << + "Skipping re-send of " << timed_outs.size() << + " timed-out reliables to peer_id " << udpPeer->id + << " (half-open)." << std::endl; + } + continue; } + m_iteration_packets_avaialble -= timed_outs.size(); + for (const auto &k : timed_outs) + resendReliable(channel, k.get(), resend_timeout); + channel.UpdateTimers(dtime); } @@ -270,8 +263,36 @@ void ConnectionSendThread::runTimeouts(float dtime) } } +void ConnectionSendThread::resendReliable(Channel &channel, const BufferedPacket *k, float resend_timeout) +{ + assert(k); + u8 channelnum = readChannel(k->data); + u16 seqnum = k->getSeqnum(); + + channel.UpdateBytesLost(k->size()); + + derr_con << m_connection->getDesc() + << "RE-SENDING timed-out RELIABLE to " + << k->address.serializeString(); + if (resend_timeout >= 0) + derr_con << "(t/o=" << resend_timeout << "): "; + else + derr_con << "(force): "; + derr_con + << "count=" << k->resend_count + << ", channel=" << ((int) channelnum & 0xff) + << ", seqnum=" << seqnum + << std::endl; + + rawSend(k); + + // do not handle rtt here as we can't decide if this packet was + // lost or really takes more time to transmit +} + void ConnectionSendThread::rawSend(const BufferedPacket *p) { + assert(p); try { m_connection->m_udpSocket.Send(p->address, p->data, p->size()); LOG(dout_con << m_connection->getDesc() @@ -398,6 +419,23 @@ void ConnectionSendThread::processReliableCommand(ConnectionCommandPtr &c) } return; + case CONNCMD_RESEND_ONE: { + LOG(dout_con << m_connection->getDesc() + << "UDP processing reliable CONNCMD_RESEND_ONE" << std::endl); + + PeerHelper peer = m_connection->getPeerNoEx(c->peer_id); + if (!peer) + return; + Channel &channel = dynamic_cast(&peer)->channels[c->channelnum]; + + auto timed_outs = channel.outgoing_reliables_sent.getTimedOuts(0, 1); + + if (!timed_outs.empty()) + resendReliable(channel, timed_outs.front().get(), -1); + + return; + } + case CONNCMD_SERVE: case CONNCMD_CONNECT: case CONNCMD_DISCONNECT: @@ -457,6 +495,7 @@ void ConnectionSendThread::processNonReliableCommand(ConnectionCommandPtr &c_ptr sendAsPacket(c.peer_id, c.channelnum, c.data, true); return; case CONCMD_CREATE_PEER: + case CONNCMD_RESEND_ONE: FATAL_ERROR("Got command that should be reliable as unreliable command"); default: LOG(dout_con << m_connection->getDesc() @@ -918,20 +957,24 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, return; } - /* Try to identify peer by sender address (may happen on join) */ - if (peer_id == PEER_ID_INEXISTENT) { - peer_id = m_connection->lookupPeer(sender); - // We do not have to remind the peer of its - // peer id as the CONTROLTYPE_SET_PEER_ID - // command was sent reliably. - } + const bool knew_peer_id = peer_id != PEER_ID_INEXISTENT; + + if (!m_connection->ConnectedToServer()) { + // Try to identify peer by sender address + if (peer_id == PEER_ID_INEXISTENT) { + peer_id = m_connection->lookupPeer(sender); + if (peer_id != PEER_ID_INEXISTENT) { + /* During join it can happen that the CONTROLTYPE_SET_PEER_ID + * packet is lost. Since resends are not active at this stage + * we need to remind the peer manually. */ + m_connection->doResendOne(peer_id); + } + } - if (peer_id == PEER_ID_INEXISTENT) { - /* Ignore it if we are a client */ - if (m_connection->ConnectedToServer()) - return; - /* The peer was not found in our lists. Add it. */ - peer_id = m_connection->createPeer(sender, MTP_MINETEST_RELIABLE_UDP, 0); + // Someone new is trying to talk to us. Add them. + if (peer_id == PEER_ID_INEXISTENT) { + peer_id = m_connection->createPeer(sender, MTP_MINETEST_RELIABLE_UDP, 0); + } } PeerHelper peer = m_connection->getPeerNoEx(peer_id); @@ -959,6 +1002,9 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, return; } + if (knew_peer_id) + peer->SetFullyOpen(); + peer->ResetTimeout(); Channel *channel = nullptr; diff --git a/src/network/connectionthreads.h b/src/network/connectionthreads.h index c2e2dae12322..594891a2b93c 100644 --- a/src/network/connectionthreads.h +++ b/src/network/connectionthreads.h @@ -70,6 +70,7 @@ class ConnectionSendThread : public Thread private: void runTimeouts(float dtime); + void resendReliable(Channel &channel, const BufferedPacket *k, float resend_timeout); void rawSend(const BufferedPacket *p); bool rawSendAsPacket(session_t peer_id, u8 channelnum, const SharedBuffer &data, bool reliable); From fecbc76264f805c162b9c0a9146753d457d0bbf8 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 00:43:47 +0100 Subject: [PATCH 02/13] Assign peer IDs randomly --- src/network/connection.cpp | 25 ++++++++----------------- src/network/connection.h | 2 -- src/unittest/test_connection.cpp | 6 +++--- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index 7b8bb91663e9..2c6bde456914 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -1551,29 +1551,22 @@ u16 Connection::createPeer(Address& sender, MTProtocols protocol, int fd) { // Somebody wants to make a new connection - // Get a unique peer id (2 or higher) - session_t peer_id_new = m_next_remote_peer_id; - u16 overflow = MAX_UDP_PEERS; + // Get a unique peer id + const session_t minimum = 2; + const session_t overflow = MAX_UDP_PEERS; /* Find an unused peer id */ + MutexAutoLock lock(m_peers_mutex); - bool out_of_ids = false; - for(;;) { - // Check if exists + session_t peer_id_new; + for (int tries = 0; tries < 100; tries++) { + peer_id_new = myrand_range(minimum, overflow - 1); if (m_peers.find(peer_id_new) == m_peers.end()) - - break; - // Check for overflow - if (peer_id_new == overflow) { - out_of_ids = true; break; - } - peer_id_new++; } - - if (out_of_ids) { + if (m_peers.find(peer_id_new) != m_peers.end()) { errorstream << getDesc() << " ran out of peer ids" << std::endl; return PEER_ID_INEXISTENT; } @@ -1585,8 +1578,6 @@ u16 Connection::createPeer(Address& sender, MTProtocols protocol, int fd) m_peers[peer->id] = peer; m_peer_ids.push_back(peer->id); - m_next_remote_peer_id = (peer_id_new +1 ) % MAX_UDP_PEERS; - LOG(dout_con << getDesc() << "createPeer(): giving peer_id=" << peer_id_new << std::endl); diff --git a/src/network/connection.h b/src/network/connection.h index e8451e3d5b05..e23cf954c8bc 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -793,8 +793,6 @@ class Connection u32 m_bc_receive_timeout = 0; bool m_shutting_down = false; - - session_t m_next_remote_peer_id = 2; }; } // namespace diff --git a/src/unittest/test_connection.cpp b/src/unittest/test_connection.cpp index 04fea90d6a09..ce7464fec200 100644 --- a/src/unittest/test_connection.cpp +++ b/src/unittest/test_connection.cpp @@ -245,7 +245,7 @@ void TestConnection::testConnectSendReceive() UASSERT(hand_client.last_id == 1); // Server should have the client UASSERT(hand_server.count == 1); - UASSERT(hand_server.last_id == 2); + UASSERT(hand_server.last_id >= 2); //sleep_ms(50); @@ -300,7 +300,7 @@ void TestConnection::testConnectSendReceive() UASSERT(memcmp(*sentdata, *recvdata, recvdata.getSize()) == 0); } - session_t peer_id_client = 2; + const session_t peer_id_client = hand_server.last_id; /* Send a large packet */ @@ -374,5 +374,5 @@ void TestConnection::testConnectSendReceive() UASSERT(hand_client.count == 1); UASSERT(hand_client.last_id == 1); UASSERT(hand_server.count == 1); - UASSERT(hand_server.last_id == 2); + UASSERT(hand_server.last_id >= 2); } From 442925545f9a1fd2b238c4957b15fe89dd43aaed Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 09:40:13 +0100 Subject: [PATCH 03/13] Use fixed, lower timeout for half-open connections --- src/network/connection.h | 1 + src/network/connectionthreads.cpp | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/network/connection.h b/src/network/connection.h index e23cf954c8bc..edecb1df3447 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -653,6 +653,7 @@ class UDPPeer : public Peer void setResendTimeout(float timeout) { MutexAutoLock lock(m_exclusive_access_mutex); resend_timeout = timeout; } + bool Ping(float dtime,SharedBuffer& data); Channel channels[CHANNEL_COUNT]; diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index 14323edb4d0e..96a89b1b5b0c 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -193,7 +193,11 @@ void ConnectionSendThread::runTimeouts(float dtime) /* Check peer timeout */ - if (peer->isTimedOut(m_timeout)) { + // When the connection is half-open give the peer less time. + // Note that this time is also fixed since the timeout is not reset in half-open state. + const float peer_timeout = peer->isHalfOpen() ? + MYMAX(5.0f, m_timeout / 4) : m_timeout; + if (peer->isTimedOut(peer_timeout)) { infostream << m_connection->getDesc() << "RunTimeouts(): Peer " << peer->id << " has timed out." @@ -208,7 +212,7 @@ void ConnectionSendThread::runTimeouts(float dtime) for (Channel &channel : udpPeer->channels) { // Remove timed out incomplete unreliable split packets - channel.incoming_splits.removeUnreliableTimedOuts(dtime, m_timeout); + channel.incoming_splits.removeUnreliableTimedOuts(dtime, peer_timeout); // Increment reliable packet times channel.outgoing_reliables_sent.incrementTimeouts(dtime); @@ -243,11 +247,7 @@ void ConnectionSendThread::runTimeouts(float dtime) if (udpPeer->Ping(dtime, data)) { LOG(dout_con << m_connection->getDesc() << "Sending ping for peer_id: " << udpPeer->id << std::endl); - /* this may fail if there ain't a sequence number left */ - if (!rawSendAsPacket(udpPeer->id, 0, data, true)) { - //retrigger with reduced ping interval - udpPeer->Ping(4.0, data); - } + rawSendAsPacket(udpPeer->id, 0, data, true); } udpPeer->RunCommandQueues(m_max_packet_size, @@ -1002,10 +1002,11 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, return; } - if (knew_peer_id) + if (knew_peer_id) { peer->SetFullyOpen(); - - peer->ResetTimeout(); + // Setup phase has a fixed timeout + peer->ResetTimeout(); + } Channel *channel = nullptr; if (dynamic_cast(&peer)) { From fa906b0b16157105f465b1fa656938a81345abe1 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 10:19:49 +0100 Subject: [PATCH 04/13] Scale resend timeout exponentially --- src/constants.h | 5 ----- src/network/connection.cpp | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/constants.h b/src/constants.h index 1ebfc75036bf..b4a2c7767e56 100644 --- a/src/constants.h +++ b/src/constants.h @@ -42,11 +42,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #define CONNECTION_TIMEOUT 30 -#define RESEND_TIMEOUT_MIN 0.1 -#define RESEND_TIMEOUT_MAX 3.0 -// resend_timeout = avg_rtt * this -#define RESEND_TIMEOUT_FACTOR 4 - /* Server */ diff --git a/src/network/connection.cpp b/src/network/connection.cpp index 2c6bde456914..dada76f83f1e 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -51,6 +51,15 @@ namespace con #define PING_TIMEOUT 5.0f +// exponent base +#define RESEND_SCALE_BASE 1.5f + +// since spacing is exponential the numbers here shouldn't be too high +// (it's okay to start out quick) +#define RESEND_TIMEOUT_MIN 0.1f +#define RESEND_TIMEOUT_MAX 2.0f +#define RESEND_TIMEOUT_FACTOR 2 + u16 BufferedPacket::getSeqnum() const { if (size() < BASE_HEADER_SIZE + 3) @@ -354,7 +363,10 @@ std::list> MutexAutoLock listlock(m_list_mutex); std::list> timed_outs; for (auto &packet : m_list) { - if (packet->time < timeout) + // resend time scales exponentially with each cycle + const float pkt_timeout = timeout * powf(RESEND_SCALE_BASE, packet->resend_count); + + if (packet->time < pkt_timeout) continue; // caller will resend packet so reset time and increase counter @@ -980,14 +992,14 @@ void UDPPeer::reportRTT(float rtt) } RTTStatistics(rtt,"rudp",MAX_RELIABLE_WINDOW_SIZE*10); + // use this value to decide the resend timeout float timeout = getStat(AVG_RTT) * RESEND_TIMEOUT_FACTOR; if (timeout < RESEND_TIMEOUT_MIN) timeout = RESEND_TIMEOUT_MIN; if (timeout > RESEND_TIMEOUT_MAX) timeout = RESEND_TIMEOUT_MAX; - MutexAutoLock usage_lock(m_exclusive_access_mutex); - resend_timeout = timeout; + setResendTimeout(timeout); } bool UDPPeer::Ping(float dtime,SharedBuffer& data) From 03863f14ae869ef7b3b30cb4cb8881f54a4f0d83 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 11:03:19 +0100 Subject: [PATCH 05/13] Minor code corrections --- src/network/connection.cpp | 36 +++++++++++++++++-------------- src/network/connection.h | 12 +++++------ src/network/connectionthreads.cpp | 24 +++++++++------------ 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index dada76f83f1e..5bc22db74760 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -194,7 +194,7 @@ u32 ReliablePacketBuffer::size() return m_list.size(); } -RPBSearchResult ReliablePacketBuffer::findPacketNoLock(u16 seqnum) +ReliablePacketBuffer::FindResult ReliablePacketBuffer::findPacketNoLock(u16 seqnum) { for (auto it = m_list.begin(); it != m_list.end(); ++it) { if ((*it)->getSeqnum() == seqnum) @@ -232,7 +232,7 @@ BufferedPacketPtr ReliablePacketBuffer::popFirst() BufferedPacketPtr ReliablePacketBuffer::popSeqnum(u16 seqnum) { MutexAutoLock listlock(m_list_mutex); - RPBSearchResult r = findPacketNoLock(seqnum); + auto r = findPacketNoLock(seqnum); if (r == m_list.end()) { LOG(dout_con<<"Sequence number: " << seqnum << " not found in reliable buffer"<getSeqnum(), i->size(), i->address.serializeString().c_str()); - fprintf(stderr, "New: seqnum: %05d size: %04zu, address: %s\n", + warningstream << buf; + snprintf(buf, sizeof(buf), + "New: seqnum: %05d size: %04zu, address: %s\n", p.getSeqnum(), p.size(), p.address.serializeString().c_str()); + warningstream << buf << std::flush; throw IncomingDataCorruption("duplicated packet isn't same as original one"); } } @@ -357,11 +363,11 @@ void ReliablePacketBuffer::incrementTimeouts(float dtime) } } -std::list> +std::vector> ReliablePacketBuffer::getTimedOuts(float timeout, u32 max_packets) { MutexAutoLock listlock(m_list_mutex); - std::list> timed_outs; + std::vector> timed_outs; for (auto &packet : m_list) { // resend time scales exponentially with each cycle const float pkt_timeout = timeout * powf(RESEND_SCALE_BASE, packet->resend_count); @@ -504,9 +510,9 @@ SharedBuffer IncomingSplitBuffer::insert(BufferedPacketPtr &p_ptr, bool reli void IncomingSplitBuffer::removeUnreliableTimedOuts(float dtime, float timeout) { + MutexAutoLock listlock(m_map_mutex); std::vector remove_queue; { - MutexAutoLock listlock(m_map_mutex); for (const auto &i : m_buf) { IncomingSplitPacket *p = i.second; // Reliable ones are not removed by timeout @@ -518,10 +524,10 @@ void IncomingSplitBuffer::removeUnreliableTimedOuts(float dtime, float timeout) } } for (u16 j : remove_queue) { - MutexAutoLock listlock(m_map_mutex); LOG(dout_con<<"NOTE: Removing timed out unreliable split packet"<second; + m_buf.erase(it); } } @@ -967,7 +973,7 @@ void Peer::Drop() delete this; } -UDPPeer::UDPPeer(u16 a_id, Address a_address, Connection* connection) : +UDPPeer::UDPPeer(session_t a_id, Address a_address, Connection* connection) : Peer(a_address,a_id,connection) { for (Channel &channel : channels) @@ -1134,8 +1140,6 @@ bool UDPPeer::processReliableSendCommand( FATAL_ERROR_IF(!successfully_put_back_sequence_number, "error"); } - // DO NOT REMOVE n_queued! It avoids a deadlock of async locked - // 'log_message_mutex' and 'm_list_mutex'. u32 n_queued = chan.outgoing_reliables_sent.size(); LOG(dout_con<getDesc() @@ -1339,7 +1343,7 @@ PeerHelper Connection::getPeerNoEx(session_t peer_id) } /* find peer_id for address */ -u16 Connection::lookupPeer(Address& sender) +session_t Connection::lookupPeer(const Address& sender) { MutexAutoLock peerlock(m_peers_mutex); std::map::iterator j; @@ -1559,7 +1563,7 @@ float Connection::getLocalStat(rate_stat_type type) return retval; } -u16 Connection::createPeer(Address& sender, MTProtocols protocol, int fd) +session_t Connection::createPeer(Address& sender, MTProtocols protocol, int fd) { // Somebody wants to make a new connection diff --git a/src/network/connection.h b/src/network/connection.h index edecb1df3447..6704cecddf32 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -250,8 +250,6 @@ struct IncomingSplitPacket for fast access to the smallest one. */ -typedef std::list::iterator RPBSearchResult; - class ReliablePacketBuffer { public: @@ -264,7 +262,7 @@ class ReliablePacketBuffer void insert(BufferedPacketPtr &p_ptr, u16 next_expected); void incrementTimeouts(float dtime); - std::list> getTimedOuts(float timeout, u32 max_packets); + std::vector> getTimedOuts(float timeout, u32 max_packets); void print(); bool empty(); @@ -272,7 +270,9 @@ class ReliablePacketBuffer private: - RPBSearchResult findPacketNoLock(u16 seqnum); + typedef std::list::iterator FindResult; + + FindResult findPacketNoLock(u16 seqnum); std::list m_list; @@ -743,9 +743,9 @@ class Connection protected: PeerHelper getPeerNoEx(session_t peer_id); - u16 lookupPeer(Address& sender); + session_t lookupPeer(const Address& sender); - u16 createPeer(Address& sender, MTProtocols protocol, int fd); + session_t createPeer(Address& sender, MTProtocols protocol, int fd); UDPPeer* createServerPeer(Address& sender); bool deletePeer(session_t peer_id, bool timeout); diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index 96a89b1b5b0c..b5128a893d16 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -44,8 +44,6 @@ namespace con // TODO: Clean this up. #define LOG(a) a -#define WINDOW_SIZE 5 - static inline session_t readPeerId(const u8 *packetdata) { return readU16(&packetdata[4]); @@ -519,9 +517,9 @@ void ConnectionSendThread::serve(Address bind_address) void ConnectionSendThread::connect(Address address) { - LOG(dout_con << m_connection->getDesc() << " connecting to " - << address.serializeString() - << ":" << address.getPort() << std::endl); + dout_con << m_connection->getDesc() << " connecting to "; + address.print(dout_con); + dout_con << std::endl; UDPPeer *peer = m_connection->createServerPeer(address); @@ -529,11 +527,10 @@ void ConnectionSendThread::connect(Address address) m_connection->putEvent(ConnectionEvent::peerAdded(peer->id, peer->address)); Address bind_addr; - if (address.isIPv6()) - bind_addr.setAddress((IPv6AddressBytes *) NULL); + bind_addr.setAddress(static_cast(nullptr)); else - bind_addr.setAddress(0, 0, 0, 0); + bind_addr.setAddress(static_cast(0)); m_connection->m_udpSocket.Bind(bind_addr); @@ -951,9 +948,9 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, session_t peer_id = readPeerId(*packetdata); u8 channelnum = readChannel(*packetdata); - if (channelnum > CHANNEL_COUNT - 1) { + if (channelnum >= CHANNEL_COUNT) { LOG(derr_con << m_connection->getDesc() - << "Receive(): Invalid channel " << (u32)channelnum << std::endl); + << "Receive(): Invalid channel " << (int)channelnum << std::endl); return; } @@ -1008,15 +1005,14 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, peer->ResetTimeout(); } - Channel *channel = nullptr; - if (dynamic_cast(&peer)) { - channel = &dynamic_cast(&peer)->channels[channelnum]; - } else { + auto *udpPeer = dynamic_cast(&peer); + if (!udpPeer) { LOG(derr_con << m_connection->getDesc() << "Receive(): peer_id=" << peer_id << " isn't an UDPPeer?!" " Ignoring." << std::endl); return; } + Channel *channel = &udpPeer->channels[channelnum]; channel->UpdateBytesReceived(received_size); From dd9acab5e0d02df3c0788744eba5d00bbedfc7e7 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 11:13:43 +0100 Subject: [PATCH 06/13] Send initial dummy packet as empty No functional change and no compatibility implicatons but this better matches what is documented everywhere. --- src/network/networkpacket.cpp | 6 ++++++ src/unittest/test_connection.cpp | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/network/networkpacket.cpp b/src/network/networkpacket.cpp index 2867c12a5977..c99b987cf29e 100644 --- a/src/network/networkpacket.cpp +++ b/src/network/networkpacket.cpp @@ -552,6 +552,12 @@ NetworkPacket& NetworkPacket::operator<<(video::SColor src) Buffer NetworkPacket::oldForgePacket() { + // this is the dummy packet used to first contact the server + if (m_command == 0) { + assert(m_datasize == 0); + return Buffer(); + } + Buffer sb(m_datasize + 2); writeU16(&sb[0], m_command); if (m_datasize > 0) diff --git a/src/unittest/test_connection.cpp b/src/unittest/test_connection.cpp index ce7464fec200..ea280f56b3f6 100644 --- a/src/unittest/test_connection.cpp +++ b/src/unittest/test_connection.cpp @@ -277,8 +277,8 @@ void TestConnection::testConnectSendReceive() Simple send-receive test */ { - NetworkPacket pkt; - pkt.putRawPacket((u8*) "Hello World !", 14, 0); + NetworkPacket pkt(0x4b, 0); + pkt.putRawString("Hello World !", 14); auto sentdata = pkt.oldForgePacket(); @@ -306,9 +306,9 @@ void TestConnection::testConnectSendReceive() */ { const int datasize = 30000; - NetworkPacket pkt(0, datasize); + NetworkPacket pkt(0xff, datasize); for (u16 i=0; i(i/4); } infostream << "Sending data (size=" << datasize << "):"; From e44c75cd67721dbd4e5c63e43da3ba8bc60a826e Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 11:46:32 +0100 Subject: [PATCH 07/13] Remove weird command procession limit it was set to 1 too, wtf?! --- src/network/connection.cpp | 5 +---- src/network/connection.h | 1 - src/network/connectionthreads.cpp | 4 +--- src/network/connectionthreads.h | 1 - 4 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index 5bc22db74760..f75364f5ca5c 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -1158,16 +1158,13 @@ bool UDPPeer::processReliableSendCommand( void UDPPeer::RunCommandQueues( unsigned int max_packet_size, - unsigned int maxcommands, unsigned int maxtransfer) { for (Channel &channel : channels) { - unsigned int commands_processed = 0; if ((!channel.queued_commands.empty()) && - (channel.queued_reliables.size() < maxtransfer) && - (commands_processed < maxcommands)) { + (channel.queued_reliables.size() < maxtransfer)) { try { ConnectionCommandPtr c = channel.queued_commands.front(); diff --git a/src/network/connection.h b/src/network/connection.h index 6704cecddf32..d5664e8bb5a1 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -645,7 +645,6 @@ class UDPPeer : public Peer void RunCommandQueues( unsigned int max_packet_size, - unsigned int maxcommands, unsigned int maxtransfer); float getResendTimeout() diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index b5128a893d16..afe897918aa4 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -248,9 +248,7 @@ void ConnectionSendThread::runTimeouts(float dtime) rawSendAsPacket(udpPeer->id, 0, data, true); } - udpPeer->RunCommandQueues(m_max_packet_size, - m_max_commands_per_iteration, - m_max_packets_requeued); + udpPeer->RunCommandQueues(m_max_packet_size, m_max_packets_requeued); } // Remove timed out peers diff --git a/src/network/connectionthreads.h b/src/network/connectionthreads.h index 594891a2b93c..d8e083351171 100644 --- a/src/network/connectionthreads.h +++ b/src/network/connectionthreads.h @@ -102,7 +102,6 @@ class ConnectionSendThread : public Thread Semaphore m_send_sleep_semaphore; unsigned int m_iteration_packets_avaialble; - unsigned int m_max_commands_per_iteration = 1; unsigned int m_max_data_packets_per_iteration; unsigned int m_max_packets_requeued = 256; }; From 984eded84bd468d19062b7a6021bd8092205d438 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 12:24:14 +0100 Subject: [PATCH 08/13] Time out when reliables can't be delivered If one of the channels stalls for whatever reason we can't pretend the connection is fine. --- src/network/connection.cpp | 50 ++++++++++++++++++++++++++----- src/network/connection.h | 8 +++-- src/network/connectionthreads.cpp | 13 ++++---- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index f75364f5ca5c..e3d8d39994b7 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -363,8 +363,19 @@ void ReliablePacketBuffer::incrementTimeouts(float dtime) } } +u32 ReliablePacketBuffer::getTimedOuts(float timeout) +{ + MutexAutoLock listlock(m_list_mutex); + u32 count = 0; + for (auto &packet : m_list) { + if (packet->totaltime >= timeout) + count++; + } + return count; +} + std::vector> - ReliablePacketBuffer::getTimedOuts(float timeout, u32 max_packets) + ReliablePacketBuffer::getResend(float timeout, u32 max_packets) { MutexAutoLock listlock(m_list_mutex); std::vector> timed_outs; @@ -939,17 +950,22 @@ void Peer::RTTStatistics(float rtt, const std::string &profiler_id, m_last_rtt = rtt; } -bool Peer::isTimedOut(float timeout) +bool Peer::isTimedOut(float timeout, std::string &reason) { MutexAutoLock lock(m_exclusive_access_mutex); - u64 current_time = porting::getTimeMs(); - float dtime = CALC_DTIME(m_last_timeout_check,current_time); - m_last_timeout_check = current_time; - - m_timeout_counter += dtime; + { + u64 current_time = porting::getTimeMs(); + float dtime = CALC_DTIME(m_last_timeout_check, current_time); + m_last_timeout_check = current_time; + m_timeout_counter += dtime; + } + if (m_timeout_counter > timeout) { + reason = "timeout counter"; + return true; + } - return m_timeout_counter > timeout; + return false; } void Peer::Drop() @@ -980,6 +996,24 @@ UDPPeer::UDPPeer(session_t a_id, Address a_address, Connection* connection) : channel.setWindowSize(START_RELIABLE_WINDOW_SIZE); } +bool UDPPeer::isTimedOut(float timeout, std::string &reason) +{ + if (Peer::isTimedOut(timeout, reason)) + return true; + + MutexAutoLock lock(m_exclusive_access_mutex); + + for (int i = 0; i < CHANNEL_COUNT; i++) { + Channel &channel = channels[i]; + if (channel.outgoing_reliables_sent.getTimedOuts(timeout) > 0) { + reason = "outgoing reliables channel=" + itos(i); + return true; + } + } + + return false; +} + bool UDPPeer::getAddress(MTProtocols type,Address& toset) { if ((type == MTP_UDP) || (type == MTP_MINETEST_RELIABLE_UDP) || (type == MTP_PRIMARY)) diff --git a/src/network/connection.h b/src/network/connection.h index d5664e8bb5a1..a14ebb9e93c8 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -262,7 +262,9 @@ class ReliablePacketBuffer void insert(BufferedPacketPtr &p_ptr, u16 next_expected); void incrementTimeouts(float dtime); - std::vector> getTimedOuts(float timeout, u32 max_packets); + u32 getTimedOuts(float timeout); + // timeout relative to last resend + std::vector> getResend(float timeout, u32 max_packets); void print(); bool empty(); @@ -525,7 +527,7 @@ class Peer { bool isHalfOpen() const { return m_half_open; } void SetFullyOpen() { m_half_open = false; } - bool isTimedOut(float timeout); + virtual bool isTimedOut(float timeout, std::string &reason); unsigned int m_increment_packets_remaining = 0; @@ -636,6 +638,8 @@ class UDPPeer : public Peer SharedBuffer addSplitPacket(u8 channel, BufferedPacketPtr &toadd, bool reliable); + bool isTimedOut(float timeout, std::string &reason) override; + protected: /* Calculates avg_rtt and resend_timeout. diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index afe897918aa4..970b7d1ed8b7 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -195,10 +195,11 @@ void ConnectionSendThread::runTimeouts(float dtime) // Note that this time is also fixed since the timeout is not reset in half-open state. const float peer_timeout = peer->isHalfOpen() ? MYMAX(5.0f, m_timeout / 4) : m_timeout; - if (peer->isTimedOut(peer_timeout)) { + std::string reason; + if (peer->isTimedOut(peer_timeout, reason)) { infostream << m_connection->getDesc() << "RunTimeouts(): Peer " << peer->id - << " has timed out." + << " has timed out (" << reason << ")" << std::endl; // Add peer to the list timeouted_peers.push_back(peer->id); @@ -216,7 +217,7 @@ void ConnectionSendThread::runTimeouts(float dtime) channel.outgoing_reliables_sent.incrementTimeouts(dtime); // Re-send timed out outgoing reliables - auto timed_outs = channel.outgoing_reliables_sent.getTimedOuts(resend_timeout, + auto timed_outs = channel.outgoing_reliables_sent.getResend(resend_timeout, (m_max_data_packets_per_iteration / numpeers)); channel.UpdatePacketLossCounter(timed_outs.size()); @@ -424,10 +425,10 @@ void ConnectionSendThread::processReliableCommand(ConnectionCommandPtr &c) return; Channel &channel = dynamic_cast(&peer)->channels[c->channelnum]; - auto timed_outs = channel.outgoing_reliables_sent.getTimedOuts(0, 1); + auto list = channel.outgoing_reliables_sent.getResend(0, 1); - if (!timed_outs.empty()) - resendReliable(channel, timed_outs.front().get(), -1); + if (!list.empty()) + resendReliable(channel, list.front().get(), -1); return; } From dd37c54328b0eaec695585614bd651944aff3c7d Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 13:54:50 +0100 Subject: [PATCH 09/13] Do not allocate packet quota to half-open connections --- src/network/connection.cpp | 15 ++++++++++++ src/network/connection.h | 2 ++ src/network/connectionthreads.cpp | 38 ++++++++++++++++--------------- src/network/connectionthreads.h | 4 ++-- 4 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/network/connection.cpp b/src/network/connection.cpp index e3d8d39994b7..8c9f1cbac7f5 100644 --- a/src/network/connection.cpp +++ b/src/network/connection.cpp @@ -1397,6 +1397,21 @@ session_t Connection::lookupPeer(const Address& sender) return PEER_ID_INEXISTENT; } +u32 Connection::getActiveCount() +{ + MutexAutoLock peerlock(m_peers_mutex); + u32 count = 0; + for (auto &it : m_peers) { + Peer *peer = it.second; + if (peer->isPendingDeletion()) + continue; + if (peer->isHalfOpen()) + continue; + count++; + } + return count; +} + bool Connection::deletePeer(session_t peer_id, bool timeout) { Peer *peer = 0; diff --git a/src/network/connection.h b/src/network/connection.h index a14ebb9e93c8..5dc97441a66a 100644 --- a/src/network/connection.h +++ b/src/network/connection.h @@ -764,6 +764,8 @@ class Connection return m_peer_ids; } + u32 getActiveCount(); + UDPSocket m_udpSocket; // Command queue: user -> SendThread MutexedQueue m_command_queue; diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index 970b7d1ed8b7..d99169160ead 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -86,8 +86,6 @@ void *ConnectionSendThread::run() BEGIN_DEBUG_EXCEPTION_HANDLER PROFILE(ScopeProfiler sp(g_profiler, ThreadIdentifier.str(), SPT_AVG)); - m_iteration_packets_avaialble = m_max_data_packets_per_iteration; - /* wait for trigger or timeout */ m_send_sleep_semaphore.wait(50); @@ -99,8 +97,16 @@ void *ConnectionSendThread::run() curtime = porting::getTimeMs(); float dtime = CALC_DTIME(lasttime, curtime); + m_iteration_packets_avaialble = m_max_data_packets_per_iteration; + const auto &calculate_quota = [&] () -> u32 { + u32 numpeers = m_connection->getActiveCount(); + if (numpeers > 0) + return MYMAX(1, m_iteration_packets_avaialble / numpeers); + return m_iteration_packets_avaialble; + }; + /* first resend timed-out packets */ - runTimeouts(dtime); + runTimeouts(dtime, calculate_quota()); if (m_iteration_packets_avaialble == 0) { LOG(warningstream << m_connection->getDesc() << " Packet quota used up after re-sending packets, " @@ -119,7 +125,7 @@ void *ConnectionSendThread::run() } /* send queued packets */ - sendPackets(dtime); + sendPackets(dtime, calculate_quota()); END_DEBUG_EXCEPTION_HANDLER } @@ -160,17 +166,12 @@ bool ConnectionSendThread::packetsQueued() return false; } -void ConnectionSendThread::runTimeouts(float dtime) +void ConnectionSendThread::runTimeouts(float dtime, u32 peer_packet_quota) { std::vector timeouted_peers; std::vector peerIds = m_connection->getPeerIDs(); - const u32 numpeers = m_connection->m_peers.size(); - - if (numpeers == 0) - return; - - for (session_t &peerId : peerIds) { + for (const session_t peerId : peerIds) { PeerHelper peer = m_connection->getPeerNoEx(peerId); if (!peer) @@ -217,8 +218,8 @@ void ConnectionSendThread::runTimeouts(float dtime) channel.outgoing_reliables_sent.incrementTimeouts(dtime); // Re-send timed out outgoing reliables - auto timed_outs = channel.outgoing_reliables_sent.getResend(resend_timeout, - (m_max_data_packets_per_iteration / numpeers)); + auto timed_outs = channel.outgoing_reliables_sent.getResend( + resend_timeout, peer_packet_quota); channel.UpdatePacketLossCounter(timed_outs.size()); g_profiler->graphAdd("packets_lost", timed_outs.size()); @@ -235,7 +236,11 @@ void ConnectionSendThread::runTimeouts(float dtime) continue; } - m_iteration_packets_avaialble -= timed_outs.size(); + if (m_iteration_packets_avaialble > timed_outs.size()) + m_iteration_packets_avaialble -= timed_outs.size(); + else + m_iteration_packets_avaialble = 0; + for (const auto &k : timed_outs) resendReliable(channel, k.get(), resend_timeout); @@ -643,15 +648,12 @@ void ConnectionSendThread::sendToAllReliable(ConnectionCommandPtr &c) } } -void ConnectionSendThread::sendPackets(float dtime) +void ConnectionSendThread::sendPackets(float dtime, u32 peer_packet_quota) { std::vector peerIds = m_connection->getPeerIDs(); std::vector pendingDisconnect; std::map pending_unreliable; - const unsigned int peer_packet_quota = m_iteration_packets_avaialble - / MYMAX(peerIds.size(), 1); - for (session_t peerId : peerIds) { PeerHelper peer = m_connection->getPeerNoEx(peerId); //peer may have been removed diff --git a/src/network/connectionthreads.h b/src/network/connectionthreads.h index d8e083351171..3ba45b3b8e4c 100644 --- a/src/network/connectionthreads.h +++ b/src/network/connectionthreads.h @@ -69,7 +69,7 @@ class ConnectionSendThread : public Thread void setPeerTimeout(float peer_timeout) { m_timeout = peer_timeout; } private: - void runTimeouts(float dtime); + void runTimeouts(float dtime, u32 peer_packet_quota); void resendReliable(Channel &channel, const BufferedPacket *k, float resend_timeout); void rawSend(const BufferedPacket *p); bool rawSendAsPacket(session_t peer_id, u8 channelnum, @@ -86,7 +86,7 @@ class ConnectionSendThread : public Thread void sendToAll(u8 channelnum, const SharedBuffer &data); void sendToAllReliable(ConnectionCommandPtr &c); - void sendPackets(float dtime); + void sendPackets(float dtime, u32 peer_packet_quota); void sendAsPacket(session_t peer_id, u8 channelnum, const SharedBuffer &data, bool ack = false); From 433461d1655c8cf1238c2af0ae5f27ce46b3ea9b Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 15:12:20 +0100 Subject: [PATCH 10/13] Rate-limit client connection attempts --- src/network/connectionthreads.cpp | 16 +++++++++++++++- src/network/connectionthreads.h | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index d99169160ead..03771b33f158 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -44,6 +44,8 @@ namespace con // TODO: Clean this up. #define LOG(a) a +#define MAX_NEW_PEERS_PER_SEC 30 + static inline session_t readPeerId(const u8 *packetdata) { return readU16(&packetdata[4]); @@ -971,7 +973,19 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, // Someone new is trying to talk to us. Add them. if (peer_id == PEER_ID_INEXISTENT) { - peer_id = m_connection->createPeer(sender, MTP_MINETEST_RELIABLE_UDP, 0); + auto &l = m_new_peer_ratelimit; + l.tick(); + if (++l.counter > MAX_NEW_PEERS_PER_SEC) { + if (!l.logged) { + warningstream << m_connection->getDesc() + << "Receive(): More than " << MAX_NEW_PEERS_PER_SEC + << " new clients within 1s. Throttling." << std::endl; + } + l.logged = true; + // We simply drop the packet, the client can try again. + } else { + peer_id = m_connection->createPeer(sender, MTP_MINETEST_RELIABLE_UDP, 0); + } } } diff --git a/src/network/connectionthreads.h b/src/network/connectionthreads.h index 3ba45b3b8e4c..7e0d44373fa9 100644 --- a/src/network/connectionthreads.h +++ b/src/network/connectionthreads.h @@ -162,8 +162,25 @@ class ConnectionReceiveThread : public Thread bool reliable); }; + struct RateLimitHelper { + u64 time = 0; + int counter = 0; + bool logged = false; + + void tick() { + u64 now = porting::getTimeS(); + if (time != now) { + time = now; + counter = 0; + logged = false; + } + } + }; + static const PacketTypeHandler packetTypeRouter[PACKET_TYPE_MAX]; Connection *m_connection = nullptr; + + RateLimitHelper m_new_peer_ratelimit; }; } From 930dcbf1f12ac6c00cf6715c0c078fe22c457887 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 5 Jan 2024 16:27:06 +0100 Subject: [PATCH 11/13] Make server disconnect lingering clients --- src/clientiface.cpp | 33 ++++++++++++++++++++++++++++++--- src/clientiface.h | 7 +++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/clientiface.cpp b/src/clientiface.cpp index 4484b08d5921..aac0aa84521e 100644 --- a/src/clientiface.cpp +++ b/src/clientiface.cpp @@ -38,8 +38,8 @@ const char *ClientInterface::statenames[] = { "Disconnecting", "Denied", "Created", - "AwaitingInit2", "HelloSent", + "AwaitingInit2", "InitDone", "DefinitionsSent", "Active", @@ -647,8 +647,7 @@ u64 RemoteClient::uptime() const ClientInterface::ClientInterface(const std::shared_ptr & con) : m_con(con), - m_env(NULL), - m_print_info_timer(0.0f) + m_env(nullptr) { } @@ -706,6 +705,34 @@ void ClientInterface::step(float dtime) m_print_info_timer = 0.0f; UpdatePlayerList(); } + + m_check_linger_timer += dtime; + if (m_check_linger_timer < 1.0f) + return; + m_check_linger_timer = 0; + + RecursiveMutexAutoLock clientslock(m_clients_mutex); + for (const auto &it : m_clients) { + auto state = it.second->getState(); + if (state >= CS_HelloSent) + continue; + if (it.second->uptime() <= LINGER_TIMEOUT) + continue; + // CS_Created means nobody has even noticed the client is there + // (this is before on_prejoinplayer runs) + // CS_Invalid should not happen + // -> log those as warning, the rest as info + std::ostream &os = state == CS_Created || state == CS_Invalid ? + warningstream : infostream; + try { + Address addr = m_con->GetPeerAddress(it.second->peer_id); + os << "Disconnecting lingering client from " + << addr.serializeString() << " (state=" + << state2Name(state) << ")" << std::endl; + m_con->DisconnectPeer(it.second->peer_id); + } catch (con::PeerNotFoundException &e) { + } + } } void ClientInterface::UpdatePlayerList() diff --git a/src/clientiface.h b/src/clientiface.h index c531d743a156..c659bafd4252 100644 --- a/src/clientiface.h +++ b/src/clientiface.h @@ -181,8 +181,8 @@ enum ClientState CS_Disconnecting, CS_Denied, CS_Created, - CS_AwaitingInit2, CS_HelloSent, + CS_AwaitingInit2, CS_InitDone, CS_DefinitionsSent, CS_Active, @@ -550,7 +550,10 @@ class ClientInterface { // Environment ServerEnvironment *m_env; - float m_print_info_timer; + float m_print_info_timer = 0; + float m_check_linger_timer = 0; static const char *statenames[]; + + static constexpr int LINGER_TIMEOUT = 10; }; From bad960a83e12006306654a5cd1c99e819286f97a Mon Sep 17 00:00:00 2001 From: sfan5 Date: Sat, 6 Jan 2024 21:39:06 +0100 Subject: [PATCH 12/13] Require client to consistently use peer ID --- src/network/connectionthreads.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/network/connectionthreads.cpp b/src/network/connectionthreads.cpp index 03771b33f158..ed2bcd0b6ce1 100644 --- a/src/network/connectionthreads.cpp +++ b/src/network/connectionthreads.cpp @@ -1018,6 +1018,13 @@ void ConnectionReceiveThread::receive(SharedBuffer &packetdata, peer->SetFullyOpen(); // Setup phase has a fixed timeout peer->ResetTimeout(); + } else if (!peer->isHalfOpen()) { + // If the peer talks to us without a peer ID when it has done so + // before something is definitely fishy. + LOG(derr_con << m_connection->getDesc() + << " Peer " << peer_id << " sending without peer id?!" + " Ignoring." << std::endl); + return; } auto *udpPeer = dynamic_cast(&peer); From cb5ac4df48504d357f6c3c4efcb55619bde31313 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Mon, 15 Jan 2024 22:34:27 +0100 Subject: [PATCH 13/13] Sanitize lang_code and full_version received from client fixes #14262 --- src/clientiface.cpp | 26 ++++++++++++++++++++++---- src/clientiface.h | 12 +++--------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/clientiface.cpp b/src/clientiface.cpp index aac0aa84521e..6563c404c646 100644 --- a/src/clientiface.cpp +++ b/src/clientiface.cpp @@ -33,6 +33,18 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "util/srp.h" #include "face_position_cache.h" +static std::string string_sanitize_ascii(const std::string &s, u32 max_length) +{ + std::string out; + for (char c : s) { + if (out.size() >= max_length) + break; + if (c > 32 && c < 127) + out.push_back(c); + } + return out; +} + const char *ClientInterface::statenames[] = { "Invalid", "Disconnecting", @@ -46,8 +58,6 @@ const char *ClientInterface::statenames[] = { "SudoMode", }; - - std::string ClientInterface::state2Name(ClientState state) { return statenames[state]; @@ -639,9 +649,17 @@ void RemoteClient::resetChosenMech() chosen_mech = AUTH_MECHANISM_NONE; } -u64 RemoteClient::uptime() const +void RemoteClient::setVersionInfo(u8 major, u8 minor, u8 patch, const std::string &full) +{ + m_version_major = major; + m_version_minor = minor; + m_version_patch = patch; + m_full_version = string_sanitize_ascii(full, 64); +} + +void RemoteClient::setLangCode(const std::string &code) { - return porting::getTimeS() - m_connection_time; + m_lang_code = string_sanitize_ascii(code, 12); } ClientInterface::ClientInterface(const std::shared_ptr & con) diff --git a/src/clientiface.h b/src/clientiface.h index c659bafd4252..4d63ca81528b 100644 --- a/src/clientiface.h +++ b/src/clientiface.h @@ -329,16 +329,10 @@ class RemoteClient { serialization_version = m_pending_serialization_version; } /* get uptime */ - u64 uptime() const; + u64 uptime() const { return porting::getTimeS() - m_connection_time; } /* set version information */ - void setVersionInfo(u8 major, u8 minor, u8 patch, const std::string &full) - { - m_version_major = major; - m_version_minor = minor; - m_version_patch = patch; - m_full_version = full; - } + void setVersionInfo(u8 major, u8 minor, u8 patch, const std::string &full); /* read version information */ u8 getMajor() const { return m_version_major; } @@ -346,7 +340,7 @@ class RemoteClient u8 getPatch() const { return m_version_patch; } const std::string &getFullVer() const { return m_full_version; } - void setLangCode(const std::string &code) { m_lang_code = code; } + void setLangCode(const std::string &code); const std::string &getLangCode() const { return m_lang_code; } void setCachedAddress(const Address &addr) { m_addr = addr; }