From 8e1be37febbda6ac8c40ca3868556aa9d67f6bf7 Mon Sep 17 00:00:00 2001 From: mathieui Date: Tue, 26 Jan 2016 20:49:54 +0100 Subject: [PATCH 1/4] NetPlayServer: in-class initialization --- Source/Core/Core/NetPlayServer.cpp | 11 ----------- Source/Core/Core/NetPlayServer.h | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/Source/Core/Core/NetPlayServer.cpp b/Source/Core/Core/NetPlayServer.cpp index 692beb20f1..a77ba7e489 100644 --- a/Source/Core/Core/NetPlayServer.cpp +++ b/Source/Core/Core/NetPlayServer.cpp @@ -58,17 +58,6 @@ NetPlayServer::~NetPlayServer() // called from ---GUI--- thread NetPlayServer::NetPlayServer(const u16 port, bool traversal, const std::string& centralServer, u16 centralPort) - : is_connected(false) - , m_is_running(false) - , m_do_loop(false) - , m_ping_key(0) - , m_update_pings(false) - , m_current_game(0) - , m_target_buffer_size(0) - , m_selected_game("") - , m_server(nullptr) - , m_traversal_client(nullptr) - , m_dialog(nullptr) { //--use server time if (enet_initialize() != 0) diff --git a/Source/Core/Core/NetPlayServer.h b/Source/Core/Core/NetPlayServer.h index cc6cb62820..05a5984f2a 100644 --- a/Source/Core/Core/NetPlayServer.h +++ b/Source/Core/Core/NetPlayServer.h @@ -50,7 +50,7 @@ public: std::unordered_set GetInterfaceSet(); std::string GetInterfaceHost(const std::string& inter); - bool is_connected; + bool is_connected = false; #ifdef USE_UPNP void TryPortmapping(u16 port); @@ -90,13 +90,13 @@ private: NetSettings m_settings; - bool m_is_running; - bool m_do_loop; + bool m_is_running = false; + bool m_do_loop = false; Common::Timer m_ping_timer; - u32 m_ping_key; - bool m_update_pings; - u32 m_current_game; - unsigned int m_target_buffer_size; + u32 m_ping_key = 0; + bool m_update_pings = false; + u32 m_current_game = 0; + unsigned int m_target_buffer_size = 0; PadMappingArray m_pad_map; PadMappingArray m_wiimote_map; @@ -117,9 +117,9 @@ private: std::thread m_thread; Common::FifoQueue, false> m_async_queue; - ENetHost* m_server; - TraversalClient* m_traversal_client; - NetPlayUI* m_dialog; + ENetHost* m_server = nullptr; + TraversalClient* m_traversal_client = nullptr; + NetPlayUI* m_dialog = nullptr; #ifdef USE_UPNP static void mapPortThread(const u16 port); From 1c808a2835ff959dbd6685756853389c2fa3b63b Mon Sep 17 00:00:00 2001 From: mathieui Date: Tue, 26 Jan 2016 21:03:35 +0100 Subject: [PATCH 2/4] NetPlayServer: Possible out-of-bounds access on invalid input if a pad or wiimote number was outside bounds (e.g. 42353543232), it would still have been read from the array, which could lead to inappropriate consequences, like a segfault. --- Source/Core/Core/NetPlayServer.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/NetPlayServer.cpp b/Source/Core/Core/NetPlayServer.cpp index a77ba7e489..49bc559d06 100644 --- a/Source/Core/Core/NetPlayServer.cpp +++ b/Source/Core/Core/NetPlayServer.cpp @@ -504,8 +504,10 @@ unsigned int NetPlayServer::OnData(sf::Packet& packet, Client& player) // If the data is not from the correct player, // then disconnect them. - if (m_pad_map[map] != player.pid) + if (m_pad_map.at(map) != player.pid) + { return 1; + } // Relay to clients sf::Packet spac; @@ -531,7 +533,7 @@ unsigned int NetPlayServer::OnData(sf::Packet& packet, Client& player) // If the data is not from the correct player, // then disconnect them. - if (m_wiimote_map[map] != player.pid) + if (m_wiimote_map.at(map) != player.pid) { return 1; } From 34ae512d913f9c50eebef806957f3766c4e8dbb5 Mon Sep 17 00:00:00 2001 From: mathieui Date: Tue, 26 Jan 2016 21:32:16 +0100 Subject: [PATCH 3/4] NetPlayServer: Make pad data unpacking nicer to read --- Source/Core/Core/NetPlayServer.cpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/NetPlayServer.cpp b/Source/Core/Core/NetPlayServer.cpp index 49bc559d06..bb449c4c76 100644 --- a/Source/Core/Core/NetPlayServer.cpp +++ b/Source/Core/Core/NetPlayServer.cpp @@ -500,7 +500,16 @@ unsigned int NetPlayServer::OnData(sf::Packet& packet, Client& player) PadMapping map = 0; GCPadStatus pad; - packet >> map >> pad.button >> pad.analogA >> pad.analogB >> pad.stickX >> pad.stickY >> pad.substickX >> pad.substickY >> pad.triggerLeft >> pad.triggerRight; + packet >> map + >> pad.button + >> pad.analogA + >> pad.analogB + >> pad.stickX + >> pad.stickY + >> pad.substickX + >> pad.substickY + >> pad.triggerLeft + >> pad.triggerRight; // If the data is not from the correct player, // then disconnect them. @@ -512,7 +521,16 @@ unsigned int NetPlayServer::OnData(sf::Packet& packet, Client& player) // Relay to clients sf::Packet spac; spac << (MessageId)NP_MSG_PAD_DATA; - spac << map << pad.button << pad.analogA << pad.analogB << pad.stickX << pad.stickY << pad.substickX << pad.substickY << pad.triggerLeft << pad.triggerRight; + spac << map + << pad.button + << pad.analogA + << pad.analogB + << pad.stickX + << pad.stickY + << pad.substickX + << pad.substickY + << pad.triggerLeft + << pad.triggerRight; SendToClients(spac, player.pid); } From 8ce919194823c61d7c283ea4543faa45d6387283 Mon Sep 17 00:00:00 2001 From: mathieui Date: Tue, 26 Jan 2016 21:37:42 +0100 Subject: [PATCH 4/4] NetPlayServer: Make SendAsyncToClients use an unique_ptr --- Source/Core/Core/NetPlayServer.cpp | 24 ++++++++++++------------ Source/Core/Core/NetPlayServer.h | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/Source/Core/Core/NetPlayServer.cpp b/Source/Core/Core/NetPlayServer.cpp index bb449c4c76..e351c70bb0 100644 --- a/Source/Core/Core/NetPlayServer.cpp +++ b/Source/Core/Core/NetPlayServer.cpp @@ -450,18 +450,18 @@ void NetPlayServer::AdjustPadBufferSize(unsigned int size) m_target_buffer_size = size; // tell clients to change buffer size - sf::Packet* spac = new sf::Packet; - *spac << (MessageId)NP_MSG_PAD_BUFFER; - *spac << (u32)m_target_buffer_size; + auto spac = std::make_unique(); + *spac << static_cast(NP_MSG_PAD_BUFFER); + *spac << static_cast(m_target_buffer_size); - SendAsyncToClients(spac); + SendAsyncToClients(std::move(spac)); } -void NetPlayServer::SendAsyncToClients(sf::Packet* packet) +void NetPlayServer::SendAsyncToClients(std::unique_ptr packet) { { std::lock_guard lkq(m_crit.async_queue_write); - m_async_queue.Push(std::unique_ptr(packet)); + m_async_queue.Push(std::move(packet)); } ENetUtil::WakeupThread(m_server); } @@ -674,12 +674,12 @@ void NetPlayServer::OnTraversalStateChanged() // called from ---GUI--- thread void NetPlayServer::SendChatMessage(const std::string& msg) { - sf::Packet* spac = new sf::Packet; + auto spac = std::make_unique(); *spac << (MessageId)NP_MSG_CHAT_MESSAGE; *spac << (PlayerId)0; // server id always 0 *spac << msg; - SendAsyncToClients(spac); + SendAsyncToClients(std::move(spac)); } // called from ---GUI--- thread @@ -690,11 +690,11 @@ bool NetPlayServer::ChangeGame(const std::string &game) m_selected_game = game; // send changed game to clients - sf::Packet* spac = new sf::Packet; + auto spac = std::make_unique(); *spac << (MessageId)NP_MSG_CHANGE_GAME; *spac << game; - SendAsyncToClients(spac); + SendAsyncToClients(std::move(spac)); return true; } @@ -719,7 +719,7 @@ bool NetPlayServer::StartGame() g_netplay_initial_gctime = Common::Timer::GetLocalTimeSinceJan1970(); // tell clients to start game - sf::Packet* spac = new sf::Packet; + auto spac = std::make_unique(); *spac << (MessageId)NP_MSG_START_GAME; *spac << m_current_game; *spac << m_settings.m_CPUthread; @@ -738,7 +738,7 @@ bool NetPlayServer::StartGame() *spac << (u32)g_netplay_initial_gctime; *spac << (u32)(g_netplay_initial_gctime >> 32); - SendAsyncToClients(spac); + SendAsyncToClients(std::move(spac)); m_is_running = true; diff --git a/Source/Core/Core/NetPlayServer.h b/Source/Core/Core/NetPlayServer.h index 05a5984f2a..f85383c120 100644 --- a/Source/Core/Core/NetPlayServer.h +++ b/Source/Core/Core/NetPlayServer.h @@ -22,7 +22,7 @@ class NetPlayServer : public TraversalClientClient { public: void ThreadFunc(); - void SendAsyncToClients(sf::Packet* packet); + void SendAsyncToClients(std::unique_ptr packet); NetPlayServer(const u16 port, bool traversal, const std::string& centralServer, u16 centralPort); ~NetPlayServer();