From 5a1fc6098ed84aa82bc73afa2640840d17a0845f Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 19 May 2017 11:25:28 +0100 Subject: [PATCH 01/21] Attempts to prevent stupid stuff from happening --- src/d_clisrv.c | 7 ++++++- src/d_net.c | 9 ++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 939d53dec..87e51b447 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3936,6 +3936,9 @@ FILESTAMP while (HGetPacket()) { + if (doomcom->remotenode == -1) // ...this should have been ignored already + continue; // might come from PT_NODETIMEOUT somehow though + node = (SINT8)doomcom->remotenode; if (netbuffer->packettype == PT_CLIENTJOIN && server) @@ -3968,8 +3971,10 @@ FILESTAMP if (netbuffer->packettype == PT_PLAYERINFO) continue; // We do nothing with PLAYERINFO, that's for the MS browser. + if (node < 0 || node >= MAXNETNODES) // THIS SHOULDN'T EVEN BE POSSIBLE + ; //CONS_Printf("Received packet from node %d!\n", (int)node); // Packet received from someone already playing - if (nodeingame[node]) + else if (nodeingame[node]) HandlePacketFromPlayer(node); // Packet received from someone not playing else diff --git a/src/d_net.c b/src/d_net.c index 70cdc5f14..68720f5ee 100644 --- a/src/d_net.c +++ b/src/d_net.c @@ -711,6 +711,10 @@ void Net_CloseConnection(INT32 node) #else INT32 i; boolean forceclose = (node & FORCECLOSE) != 0; + + if (node == -1) + return; // nope, just ignore it + node &= ~FORCECLOSE; if (!node) @@ -718,7 +722,7 @@ void Net_CloseConnection(INT32 node) if (node < 0 || node >= MAXNETNODES) // prevent invalid nodes from crashing the game { - CONS_Alert(CONS_WARNING, M_GetText("Net_CloseConnection: invalid node %d detected!\n"), node); + //CONS_Alert(CONS_WARNING, M_GetText("Net_CloseConnection: invalid node %d detected!\n"), node); return; } @@ -1128,6 +1132,9 @@ boolean HGetPacket(void) doomcom->remotenode = 0; rebound_tail = (rebound_tail+1) % MAXREBOUND; + + if (doomcom->remotenode == -1) // wait hang on what? + return true; // there might still be packets from others though, so don't return false #ifdef DEBUGFILE if (debugfile) DebugPrintpacket("GETLOCAL"); From 1078a642dfee676cf87fcb023d9b8889477ae959 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 19 May 2017 11:52:50 +0100 Subject: [PATCH 02/21] Loop through rebound packets until you found a good one or ran out of them --- src/d_net.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/d_net.c b/src/d_net.c index 68720f5ee..8f11f60fc 100644 --- a/src/d_net.c +++ b/src/d_net.c @@ -1124,22 +1124,27 @@ boolean HGetPacket(void) // Get a packet from self if (rebound_tail != rebound_head) { - M_Memcpy(netbuffer, &reboundstore[rebound_tail], reboundsize[rebound_tail]); - doomcom->datalength = reboundsize[rebound_tail]; - if (netbuffer->packettype == PT_NODETIMEOUT) - doomcom->remotenode = netbuffer->u.textcmd[0]; - else - doomcom->remotenode = 0; + while (true) // loop until we found a valid packet, or we ran out of packets + { // provided MAXREBOUND is not all that large this shouldn't take too long + if (rebound_tail == rebound_head) + break; // just give up, none of them were any good somehow + M_Memcpy(netbuffer, &reboundstore[rebound_tail], reboundsize[rebound_tail]); + doomcom->datalength = reboundsize[rebound_tail]; + if (netbuffer->packettype == PT_NODETIMEOUT) + doomcom->remotenode = netbuffer->u.textcmd[0]; + else + doomcom->remotenode = 0; - rebound_tail = (rebound_tail+1) % MAXREBOUND; + rebound_tail = (rebound_tail+1) % MAXREBOUND; - if (doomcom->remotenode == -1) // wait hang on what? - return true; // there might still be packets from others though, so don't return false + if (doomcom->remotenode == -1) // wait hang on what? + continue; // ignore it, look for the next packet #ifdef DEBUGFILE - if (debugfile) - DebugPrintpacket("GETLOCAL"); + if (debugfile) + DebugPrintpacket("GETLOCAL"); #endif - return true; + return true; + } } if (!netgame) From 5aeb4b591949399676d0e5e3e358e539115f0e43 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Sat, 20 May 2017 21:29:05 +0100 Subject: [PATCH 03/21] Revert "Loop through rebound packets until you found a good one or ran out of them" On second thought, this was probably unnecessary anyway. This reverts commit 1078a642dfee676cf87fcb023d9b8889477ae959. --- src/d_net.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/d_net.c b/src/d_net.c index 8f11f60fc..68720f5ee 100644 --- a/src/d_net.c +++ b/src/d_net.c @@ -1124,27 +1124,22 @@ boolean HGetPacket(void) // Get a packet from self if (rebound_tail != rebound_head) { - while (true) // loop until we found a valid packet, or we ran out of packets - { // provided MAXREBOUND is not all that large this shouldn't take too long - if (rebound_tail == rebound_head) - break; // just give up, none of them were any good somehow - M_Memcpy(netbuffer, &reboundstore[rebound_tail], reboundsize[rebound_tail]); - doomcom->datalength = reboundsize[rebound_tail]; - if (netbuffer->packettype == PT_NODETIMEOUT) - doomcom->remotenode = netbuffer->u.textcmd[0]; - else - doomcom->remotenode = 0; + M_Memcpy(netbuffer, &reboundstore[rebound_tail], reboundsize[rebound_tail]); + doomcom->datalength = reboundsize[rebound_tail]; + if (netbuffer->packettype == PT_NODETIMEOUT) + doomcom->remotenode = netbuffer->u.textcmd[0]; + else + doomcom->remotenode = 0; - rebound_tail = (rebound_tail+1) % MAXREBOUND; + rebound_tail = (rebound_tail+1) % MAXREBOUND; - if (doomcom->remotenode == -1) // wait hang on what? - continue; // ignore it, look for the next packet + if (doomcom->remotenode == -1) // wait hang on what? + return true; // there might still be packets from others though, so don't return false #ifdef DEBUGFILE - if (debugfile) - DebugPrintpacket("GETLOCAL"); + if (debugfile) + DebugPrintpacket("GETLOCAL"); #endif - return true; - } + return true; } if (!netgame) From 247ed56e1790caa0a69f9f1027dfcb0d0fb5b8e8 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Mon, 22 May 2017 16:44:50 +0100 Subject: [PATCH 04/21] Don't allow nonsense PT_ASKINFOVIAMS packets, nor any at all if offline --- src/d_clisrv.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 87e51b447..098430230 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3405,13 +3405,23 @@ static void HandlePacketFromAwayNode(SINT8 node) switch (netbuffer->packettype) { case PT_ASKINFOVIAMS: + if (ms_RoomId < 0) // ignore if we're not actually on the MS right now + { + Net_CloseConnection(node); // and yes, close connection + break; + } if (server && serverrunning) { INT32 clientnode = I_NetMakeNode(netbuffer->u.msaskinfo.clientaddr); - SV_SendServerInfo(clientnode, (tic_t)LONG(netbuffer->u.msaskinfo.time)); - SV_SendPlayerInfo(clientnode); // Send extra info - Net_CloseConnection(clientnode); - // Don't close connection to MS. + if (clientnode != -1) + { + SV_SendServerInfo(clientnode, (tic_t)LONG(netbuffer->u.msaskinfo.time)); + SV_SendPlayerInfo(clientnode); // Send extra info + Net_CloseConnection(clientnode); + // Don't close connection to MS... + } + else + Net_CloseConnection(node); // ...unless the IP address is not valid } break; From 28444c12dd6d57ae43d08186a40dd5b5e8b43019 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Mon, 22 May 2017 16:54:39 +0100 Subject: [PATCH 05/21] Some more things I overlooked in this fix --- src/d_clisrv.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 098430230..17f0b7743 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3405,14 +3405,16 @@ static void HandlePacketFromAwayNode(SINT8 node) switch (netbuffer->packettype) { case PT_ASKINFOVIAMS: - if (ms_RoomId < 0) // ignore if we're not actually on the MS right now - { - Net_CloseConnection(node); // and yes, close connection - break; - } + if (server && serverrunning) { - INT32 clientnode = I_NetMakeNode(netbuffer->u.msaskinfo.clientaddr); + INT32 clientnode; + if (ms_RoomId < 0) // ignore if we're not actually on the MS right now + { + Net_CloseConnection(node); // and yes, close connection + return; + } + clientnode = I_NetMakeNode(netbuffer->u.msaskinfo.clientaddr); if (clientnode != -1) { SV_SendServerInfo(clientnode, (tic_t)LONG(netbuffer->u.msaskinfo.time)); @@ -3423,6 +3425,8 @@ static void HandlePacketFromAwayNode(SINT8 node) else Net_CloseConnection(node); // ...unless the IP address is not valid } + else + Net_CloseConnection(node); // you're not supposed to get it, so ignore it break; case PT_ASKINFO: From 3784d765e493d7820f7dd37b5d4c3c66ef7a3f8a Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Mon, 22 May 2017 20:47:38 +0100 Subject: [PATCH 06/21] Remove extra whitespace I added --- src/d_clisrv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 17f0b7743..36b03be63 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3405,7 +3405,6 @@ static void HandlePacketFromAwayNode(SINT8 node) switch (netbuffer->packettype) { case PT_ASKINFOVIAMS: - if (server && serverrunning) { INT32 clientnode; From c70839334e180869279c84f5d40360afdcc43bdc Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Mon, 22 May 2017 22:17:51 +0100 Subject: [PATCH 07/21] Added a bunch of missing checks to prevent non-server players from sending other non-server players stuff --- src/d_clisrv.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 36b03be63..c6b8bbd64 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3433,8 +3433,8 @@ static void HandlePacketFromAwayNode(SINT8 node) { SV_SendServerInfo(node, (tic_t)LONG(netbuffer->u.askinfo.time)); SV_SendPlayerInfo(node); // Send extra info - Net_CloseConnection(node); } + Net_CloseConnection(node); break; case PT_SERVERREFUSE: // Negative response of client join request @@ -3443,6 +3443,11 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } + if (node != servernode) // nope you're not the server + { + Net_CloseConnection(node); + break; + } if (cl_mode == CL_WAITJOINRESPONSE) { // Save the reason so it can be displayed after quitting the netgame @@ -3474,6 +3479,11 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } + if (node != servernode) // nope you're not the server + { + Net_CloseConnection(node); + break; + } /// \note how would this happen? and is it doing the right thing if it does? if (cl_mode != CL_WAITJOINRESPONSE) break; @@ -3537,11 +3547,20 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } - else - Got_Filetxpak(); + if (node != servernode) // nope you're not the server + { + Net_CloseConnection(node); + break; + } + Got_Filetxpak(); break; case PT_REQUESTFILE: + if (node != servernode) // nope you're not the server + { + Net_CloseConnection(node); + break; + } if (server) Got_RequestFilePak(node); break; @@ -3926,6 +3945,21 @@ FILESTAMP case PT_SERVERCFG: break; case PT_FILEFRAGMENT: + // Only accept PT_FILEFRAGMENT from the server. + if (node != servernode) + { + CONS_Alert(CONS_WARNING, M_GetText("%s received from non-host %d\n"), "PT_FILEFRAGMENT", node); + + if (server) + { + XBOXSTATIC UINT8 buf[2]; + buf[0] = (UINT8)node; + buf[1] = KICK_MSG_CON_FAIL; + SendNetXCmd(XD_KICK, &buf, 2); + } + + break; + } if (client) Got_Filetxpak(); break; From fdfd6e1c0bbce1c7c584a906a7b51f7907eba5bb Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Mon, 22 May 2017 22:20:08 +0100 Subject: [PATCH 08/21] Whoops don't do that for PT_REQUESTFILE --- src/d_clisrv.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index c6b8bbd64..762624612 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3556,11 +3556,6 @@ static void HandlePacketFromAwayNode(SINT8 node) break; case PT_REQUESTFILE: - if (node != servernode) // nope you're not the server - { - Net_CloseConnection(node); - break; - } if (server) Got_RequestFilePak(node); break; From 9e3bdc5ee229c061db3550eda3c4cf1c7e4a35fb Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Tue, 23 May 2017 16:13:42 +0100 Subject: [PATCH 09/21] Prevent bad PT_TEXTCMD/PT_TEXTCMD2 textcmd[0] sizes? --- src/d_clisrv.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 762624612..4700fcb88 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3737,6 +3737,15 @@ FILESTAMP tic_t tic = maketic; UINT8 *textcmd; + // ignore if the textcmd size var is actually larger than it should be + if (BASEPACKETSIZE + netbuffer->u.textcmd[0] > (size_t)doomcom->datalength) + { + DEBFILE(va("GetPacket: Bad Textcmd packet size! (expected %d, actual %d)\n", + BASEPACKETSIZE + netbuffer->u.textcmd[0], doomcom->datalength)); + Net_UnAcknowledgePacket(node); + break; + } + // check if tic that we are making isn't too large else we cannot send it :( // doomcom->numslots+1 "+1" since doomcom->numslots can change within this time and sent time j = software_MAXPACKETLENGTH From ff1cc07a1d20989c0271dc8c875a4d3b248aea94 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Tue, 23 May 2017 16:29:09 +0100 Subject: [PATCH 10/21] Implemented toaster's suggestion to make a macro here --- src/d_clisrv.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 4700fcb88..246642b98 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3402,6 +3402,14 @@ static void HandlePacketFromAwayNode(SINT8 node) if (node != servernode) DEBFILE(va("Received packet from unknown host %d\n", node)); +// macro for packets that should only be sent by the server +// if it is NOT from the server, bail out and close the connection! +#define SERVERONLY \ + if (node != servernode) \ + { \ + Net_CloseConnection(node); \ + break; \ + } switch (netbuffer->packettype) { case PT_ASKINFOVIAMS: @@ -3443,11 +3451,7 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } - if (node != servernode) // nope you're not the server - { - Net_CloseConnection(node); - break; - } + SERVERONLY if (cl_mode == CL_WAITJOINRESPONSE) { // Save the reason so it can be displayed after quitting the netgame @@ -3479,11 +3483,7 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } - if (node != servernode) // nope you're not the server - { - Net_CloseConnection(node); - break; - } + SERVERONLY /// \note how would this happen? and is it doing the right thing if it does? if (cl_mode != CL_WAITJOINRESPONSE) break; @@ -3547,11 +3547,7 @@ static void HandlePacketFromAwayNode(SINT8 node) Net_CloseConnection(node); break; } - if (node != servernode) // nope you're not the server - { - Net_CloseConnection(node); - break; - } + SERVERONLY Got_Filetxpak(); break; @@ -3580,6 +3576,7 @@ static void HandlePacketFromAwayNode(SINT8 node) break; // Ignore it } +#undef SERVERONLY } /** Handles a packet received from a node that is in game From 0b0b738a6f8af4ceb95c05de44f42da11b550408 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Tue, 23 May 2017 16:34:56 +0100 Subject: [PATCH 11/21] Don't allow non-servers to receive PT_RESYNCHGET --- src/d_clisrv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 246642b98..186c25e6a 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3609,6 +3609,8 @@ FILESTAMP { // -------------------------------------------- SERVER RECEIVE ---------- case PT_RESYNCHGET: + if (client) + break; SV_AcknowledgeResynchAck(netconsole, netbuffer->u.resynchgot); break; case PT_CLIENTCMD: From 7147e0fcafcdf89f65bb2e909e94ea575cdbc3d9 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Tue, 23 May 2017 17:35:32 +0100 Subject: [PATCH 12/21] Improve previous PT_TEXTCMD/PT_TEXTCMD2 check, add another one to ignore zero size textcmds! --- src/d_clisrv.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 186c25e6a..a42fcf1d1 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3736,11 +3736,23 @@ FILESTAMP tic_t tic = maketic; UINT8 *textcmd; - // ignore if the textcmd size var is actually larger than it should be - if (BASEPACKETSIZE + netbuffer->u.textcmd[0] > (size_t)doomcom->datalength) + // ignore if the textcmd has a reported size of zero + // this shouldn't be sent at all + if (!netbuffer->u.textcmd[0]) { - DEBFILE(va("GetPacket: Bad Textcmd packet size! (expected %d, actual %d)\n", - BASEPACKETSIZE + netbuffer->u.textcmd[0], doomcom->datalength)); + DEBFILE(va("GetPacket: Textcmd with size 0 detected! (node %u, player %d)\n", + node, netconsole)); + Net_UnAcknowledgePacket(node); + break; + } + + // ignore if the textcmd size var is actually larger than it should be + // BASEPACKETSIZE + 1 (for size) + textcmd[0] should == datalength + if (netbuffer->u.textcmd[0] > (size_t)doomcom->datalength-BASEPACKETSIZE-1) + { + DEBFILE(va("GetPacket: Bad Textcmd packet size! (expected %d, actual %d, node %u, player %d)\n", + netbuffer->u.textcmd[0], (size_t)doomcom->datalength-BASEPACKETSIZE-1, + node, netconsole)); Net_UnAcknowledgePacket(node); break; } From c92aaa74a41e3ad2477b564eaa9bfe086b518450 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Tue, 23 May 2017 18:46:34 +0100 Subject: [PATCH 13/21] I think PT_CLIENT2MIS was meant to do this too, probably --- src/d_clisrv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index a42fcf1d1..27fcd0672 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3677,7 +3677,8 @@ FILESTAMP } // Splitscreen cmd - if (netbuffer->packettype == PT_CLIENT2CMD && nodetoplayer2[node] >= 0) + if ((netbuffer->packettype == PT_CLIENT2CMD || netbuffer->packettype == PT_CLIENT2MIS) + && nodetoplayer2[node] >= 0) G_MoveTiccmd(&netcmds[maketic%BACKUPTICS][(UINT8)nodetoplayer2[node]], &netbuffer->u.client2pak.cmd2, 1); From 58236af6f73f535f5071d5fca84457b993239f5d Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Thu, 25 May 2017 16:55:59 +0100 Subject: [PATCH 14/21] Tweak to D_MapChange: if you failed to start a server, DON'T send a map change command --- src/d_netcmd.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/d_netcmd.c b/src/d_netcmd.c index 478772ed9..f23549315 100644 --- a/src/d_netcmd.c +++ b/src/d_netcmd.c @@ -1569,8 +1569,13 @@ void D_MapChange(INT32 mapnum, INT32 newgametype, boolean pultmode, boolean rese mapchangepending = 0; // spawn the server if needed // reset players if there is a new one - if (!(adminplayer == consoleplayer) && SV_SpawnServer()) - buf[0] &= ~(1<<1); + if (!(adminplayer == consoleplayer)) + { + if (SV_SpawnServer()) + buf[0] &= ~(1<<1); + if (!Playing()) // you failed to start a server somehow, so cancel the map change + return; + } // Kick bot from special stages if (botskin) From a23e9bc32b6357f5e366246dada0aa55c34c235b Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Thu, 25 May 2017 18:10:20 +0100 Subject: [PATCH 15/21] Fix size_t printing --- src/d_clisrv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 27fcd0672..c095d38a5 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3751,8 +3751,8 @@ FILESTAMP // BASEPACKETSIZE + 1 (for size) + textcmd[0] should == datalength if (netbuffer->u.textcmd[0] > (size_t)doomcom->datalength-BASEPACKETSIZE-1) { - DEBFILE(va("GetPacket: Bad Textcmd packet size! (expected %d, actual %d, node %u, player %d)\n", - netbuffer->u.textcmd[0], (size_t)doomcom->datalength-BASEPACKETSIZE-1, + DEBFILE(va("GetPacket: Bad Textcmd packet size! (expected %d, actual %s, node %u, player %d)\n", + netbuffer->u.textcmd[0], sizeu1((size_t)doomcom->datalength-BASEPACKETSIZE-1), node, netconsole)); Net_UnAcknowledgePacket(node); break; From aecc97ded33bbfd7e8e62aca333398a14b3c06d5 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 13:39:54 +0100 Subject: [PATCH 16/21] PT_REQUESTFILE checking: * if you sent it to a client rather than the server, game over, your connection is closed * if files that don't exist or are too large are requested are listed, game over, your connection is closed (they should have been checked on YOUR side beforehand, silly) * if the server has downloading disabled anyway, ...yeah, you get the idea Don't worry, I made sure Got_RequestFilePak cleaned up the full file request list for the node in case of failure --- src/d_clisrv.c | 7 +++++- src/d_netfil.c | 64 +++++++++++++++++++++++++++++++++++++++++++++----- src/d_netfil.h | 2 +- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index c095d38a5..2fc895c23 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -3553,7 +3553,12 @@ static void HandlePacketFromAwayNode(SINT8 node) case PT_REQUESTFILE: if (server) - Got_RequestFilePak(node); + { + if (!cv_downloading.value || !Got_RequestFilePak(node)) + Net_CloseConnection(node); // close connection if one of the requested files could not be sent, or you disabled downloading anyway + } + else + Net_CloseConnection(node); // nope break; case PT_NODETIMEOUT: diff --git a/src/d_netfil.c b/src/d_netfil.c index bf4e59878..777814cee 100644 --- a/src/d_netfil.c +++ b/src/d_netfil.c @@ -62,7 +62,9 @@ #include -static void SV_SendFile(INT32 node, const char *filename, UINT8 fileid); +// Prototypes +static boolean SV_SendFile(INT32 node, const char *filename, UINT8 fileid); +static void SV_RemoveFileSendList(INT32 node); // Sender structure typedef struct filetx_s @@ -303,7 +305,8 @@ boolean CL_SendRequestFile(void) } // get request filepak and put it on the send queue -void Got_RequestFilePak(INT32 node) +// returns false if a requested file was not found or cannot be sent +boolean Got_RequestFilePak(INT32 node) { char wad[MAX_WADPATH+1]; UINT8 *p = netbuffer->u.textcmd; @@ -314,8 +317,13 @@ void Got_RequestFilePak(INT32 node) if (id == 0xFF) break; READSTRINGN(p, wad, MAX_WADPATH); - SV_SendFile(node, wad, id); + if (!SV_SendFile(node, wad, id)) + { + SV_RemoveFileSendList(node); + return false; // don't read the rest of the files + } } + return true; // no problems with any files } /** Checks if the files needed aren't already loaded or on the disk @@ -480,7 +488,7 @@ static INT32 filestosend = 0; * \sa SV_SendRam * */ -static void SV_SendFile(INT32 node, const char *filename, UINT8 fileid) +static boolean SV_SendFile(INT32 node, const char *filename, UINT8 fileid) { filetx_t **q; // A pointer to the "next" field of the last file in the list filetx_t *p; // The new file request @@ -537,7 +545,7 @@ static void SV_SendFile(INT32 node, const char *filename, UINT8 fileid) free(p->id.filename); free(p); *q = NULL; - return; + return false; // cancel the rest of the requests } // Handle huge file requests (i.e. bigger than cv_maxsend.value KB) @@ -549,7 +557,7 @@ static void SV_SendFile(INT32 node, const char *filename, UINT8 fileid) free(p->id.filename); free(p); *q = NULL; - return; + return false; // cancel the rest of the requests } DEBFILE(va("Sending file %s (id=%d) to %d\n", filename, fileid, node)); @@ -557,6 +565,7 @@ static void SV_SendFile(INT32 node, const char *filename, UINT8 fileid) p->fileid = fileid; p->next = NULL; // End of list filestosend++; + return true; } /** Adds a memory block to the file list for a node @@ -598,6 +607,49 @@ void SV_SendRam(INT32 node, void *data, size_t size, freemethod_t freemethod, UI filestosend++; } +/** Removes all file requests for a node + * This is needed only if a PT_REQUESTFILE's content gave you something that shouldn't be there + * + * \param node The destination + * \sa Got_RequestFilePak + * + */ +static void SV_RemoveFileSendList(INT32 node) +{ + filetx_t *p = transfer[node].txlist; + + if (p == NULL) + return; // ...well, that was easy + + while (p) + { + // Free the file request according to the freemethod parameter used with SV_SendFile/Ram + switch (p->ram) + { + case SF_FILE: // It's a file, close it and free its filename + if (cv_noticedownload.value) + CONS_Printf("Cancelling file transfer for node %d\n", node); + if (transfer[node].currentfile) + fclose(transfer[node].currentfile); + free(p->id.filename); + break; + case SF_Z_RAM: // It's a memory block allocated with Z_Alloc or the likes, use Z_Free + Z_Free(p->id.ram); + break; + case SF_RAM: // It's a memory block allocated with malloc, use free + free(p->id.ram); + case SF_NOFREERAM: // Nothing to free + break; + } + // Remove the file request from the list + transfer[node].txlist = p->next; + free(p); + // Indicate that the transmission is over (if for some reason it had started) + transfer[node].currentfile = NULL; + filestosend--; + } +} + /** Stops sending a file for a node, and removes the file request from the list, * either because the file has been fully sent or because the node was disconnected * diff --git a/src/d_netfil.h b/src/d_netfil.h index c9085a5b0..b9b7b2f2e 100644 --- a/src/d_netfil.h +++ b/src/d_netfil.h @@ -69,7 +69,7 @@ boolean SV_SendingFile(INT32 node); boolean CL_CheckDownloadable(void); boolean CL_SendRequestFile(void); -void Got_RequestFilePak(INT32 node); +boolean Got_RequestFilePak(INT32 node); void SV_AbortSendFiles(INT32 node); void CloseNetFile(void); From 7979f84e258769cf230610a6cd7b4c0161479ba4 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 13:58:34 +0100 Subject: [PATCH 17/21] whoops --- src/d_netfil.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/d_netfil.c b/src/d_netfil.c index 777814cee..8704e05ce 100644 --- a/src/d_netfil.c +++ b/src/d_netfil.c @@ -644,6 +644,7 @@ static void SV_RemoveFileSendList(INT32 node) // Remove the file request from the list transfer[node].txlist = p->next; free(p); + p = transfer[node].txlist; // Indicate that the transmission is over (if for some reason it had started) transfer[node].currentfile = NULL; filestosend--; From 569af9f4c13b3124c7821689b612762d727274eb Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 14:19:18 +0100 Subject: [PATCH 18/21] I am dumb, SV_AbortSendFiles already does what SV_RemoveFileSendList was made to do --- src/d_netfil.c | 47 +---------------------------------------------- 1 file changed, 1 insertion(+), 46 deletions(-) diff --git a/src/d_netfil.c b/src/d_netfil.c index 8704e05ce..84e16aa1d 100644 --- a/src/d_netfil.c +++ b/src/d_netfil.c @@ -64,7 +64,6 @@ // Prototypes static boolean SV_SendFile(INT32 node, const char *filename, UINT8 fileid); -static void SV_RemoveFileSendList(INT32 node); // Sender structure typedef struct filetx_s @@ -319,7 +318,7 @@ boolean Got_RequestFilePak(INT32 node) READSTRINGN(p, wad, MAX_WADPATH); if (!SV_SendFile(node, wad, id)) { - SV_RemoveFileSendList(node); + SV_AbortSendFiles(node); return false; // don't read the rest of the files } } @@ -607,50 +606,6 @@ void SV_SendRam(INT32 node, void *data, size_t size, freemethod_t freemethod, UI filestosend++; } -/** Removes all file requests for a node - * This is needed only if a PT_REQUESTFILE's content gave you something that shouldn't be there - * - * \param node The destination - * \sa Got_RequestFilePak - * - */ -static void SV_RemoveFileSendList(INT32 node) -{ - filetx_t *p = transfer[node].txlist; - - if (p == NULL) - return; // ...well, that was easy - - while (p) - { - // Free the file request according to the freemethod parameter used with SV_SendFile/Ram - switch (p->ram) - { - case SF_FILE: // It's a file, close it and free its filename - if (cv_noticedownload.value) - CONS_Printf("Cancelling file transfer for node %d\n", node); - if (transfer[node].currentfile) - fclose(transfer[node].currentfile); - free(p->id.filename); - break; - case SF_Z_RAM: // It's a memory block allocated with Z_Alloc or the likes, use Z_Free - Z_Free(p->id.ram); - break; - case SF_RAM: // It's a memory block allocated with malloc, use free - free(p->id.ram); - case SF_NOFREERAM: // Nothing to free - break; - } - // Remove the file request from the list - transfer[node].txlist = p->next; - free(p); - p = transfer[node].txlist; - // Indicate that the transmission is over (if for some reason it had started) - transfer[node].currentfile = NULL; - filestosend--; - } -} - /** Stops sending a file for a node, and removes the file request from the list, * either because the file has been fully sent or because the node was disconnected * From e09270276ef445a36dae97b147c023b345f63eed Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 14:38:59 +0100 Subject: [PATCH 19/21] Display node's IP when printing the "sending file to node n" message, if noticedownload is turned on --- src/d_netfil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/d_netfil.c b/src/d_netfil.c index 84e16aa1d..2c463c0b5 100644 --- a/src/d_netfil.c +++ b/src/d_netfil.c @@ -495,7 +495,7 @@ static boolean SV_SendFile(INT32 node, const char *filename, UINT8 fileid) char wadfilename[MAX_WADPATH]; if (cv_noticedownload.value) - CONS_Printf("Sending file \"%s\" to node %d\n", filename, node); + CONS_Printf("Sending file \"%s\" to node %d (%s)\n", filename, node, I_GetNodeAddress(node)); // Find the last file in the list and set a pointer to its "next" field q = &transfer[node].txlist; From ab5835cd3b325b748a1b514bd6a825264779b982 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 15:26:00 +0100 Subject: [PATCH 20/21] Remove cruft from my initial days of fumbling with this branch textcmd[0] for PT_NODETIMEOUT can't hold anything < 0 anyway, and you'd probably have to really try to get >= MAXNETNODES --- src/d_clisrv.c | 7 +------ src/d_net.c | 2 -- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/d_clisrv.c b/src/d_clisrv.c index 2fc895c23..89418dde2 100644 --- a/src/d_clisrv.c +++ b/src/d_clisrv.c @@ -4004,9 +4004,6 @@ FILESTAMP while (HGetPacket()) { - if (doomcom->remotenode == -1) // ...this should have been ignored already - continue; // might come from PT_NODETIMEOUT somehow though - node = (SINT8)doomcom->remotenode; if (netbuffer->packettype == PT_CLIENTJOIN && server) @@ -4039,10 +4036,8 @@ FILESTAMP if (netbuffer->packettype == PT_PLAYERINFO) continue; // We do nothing with PLAYERINFO, that's for the MS browser. - if (node < 0 || node >= MAXNETNODES) // THIS SHOULDN'T EVEN BE POSSIBLE - ; //CONS_Printf("Received packet from node %d!\n", (int)node); // Packet received from someone already playing - else if (nodeingame[node]) + if (nodeingame[node]) HandlePacketFromPlayer(node); // Packet received from someone not playing else diff --git a/src/d_net.c b/src/d_net.c index 68720f5ee..b6914f817 100644 --- a/src/d_net.c +++ b/src/d_net.c @@ -1133,8 +1133,6 @@ boolean HGetPacket(void) rebound_tail = (rebound_tail+1) % MAXREBOUND; - if (doomcom->remotenode == -1) // wait hang on what? - return true; // there might still be packets from others though, so don't return false #ifdef DEBUGFILE if (debugfile) DebugPrintpacket("GETLOCAL"); From 4d1af86431b9b7d4f90b314811df7825ac467fe8 Mon Sep 17 00:00:00 2001 From: Monster Iestyn Date: Fri, 26 May 2017 15:30:26 +0100 Subject: [PATCH 21/21] Cleanup part 2, make ye old 2.1.18 warning a debugfile only message, and make the node == -1 have its own debugfile only message too Also get rid of a stray newline --- src/d_net.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/d_net.c b/src/d_net.c index b6914f817..50b6c8cf6 100644 --- a/src/d_net.c +++ b/src/d_net.c @@ -713,7 +713,10 @@ void Net_CloseConnection(INT32 node) boolean forceclose = (node & FORCECLOSE) != 0; if (node == -1) + { + DEBFILE(M_GetText("Net_CloseConnection: node -1 detected!\n")); return; // nope, just ignore it + } node &= ~FORCECLOSE; @@ -722,7 +725,7 @@ void Net_CloseConnection(INT32 node) if (node < 0 || node >= MAXNETNODES) // prevent invalid nodes from crashing the game { - //CONS_Alert(CONS_WARNING, M_GetText("Net_CloseConnection: invalid node %d detected!\n"), node); + DEBFILE(va(M_GetText("Net_CloseConnection: invalid node %d detected!\n"), node)); return; } @@ -1132,7 +1135,6 @@ boolean HGetPacket(void) doomcom->remotenode = 0; rebound_tail = (rebound_tail+1) % MAXREBOUND; - #ifdef DEBUGFILE if (debugfile) DebugPrintpacket("GETLOCAL");