From 546a58746967d10996075ca47a7198c1c375fdb2 Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Wed, 19 Jul 2017 12:47:01 -0600 Subject: server: Fixed mismatched new/delete; added proper shutdown procedure --- Source/cmConnection.cxx | 5 ----- Source/cmConnection.h | 7 ++++++- Source/cmPipeConnection.cxx | 13 ++++++++----- Source/cmServer.cxx | 23 ++++++++++------------- Source/cmServer.h | 1 + Source/cmServerConnection.cxx | 8 ++++---- 6 files changed, 29 insertions(+), 28 deletions(-) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index f3fc1ef..bc29e41 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -38,11 +38,6 @@ void cmEventBasedConnection::on_read(uv_stream_t* stream, ssize_t nread, delete[](buf->base); } -void cmEventBasedConnection::on_close_delete(uv_handle_t* handle) -{ - delete handle; -} - void cmEventBasedConnection::on_close(uv_handle_t* /*handle*/) { } diff --git a/Source/cmConnection.h b/Source/cmConnection.h index f9d50de..b1b51fe 100644 --- a/Source/cmConnection.h +++ b/Source/cmConnection.h @@ -100,7 +100,12 @@ public: uv_stream_t* WriteStream = nullptr; static void on_close(uv_handle_t* handle); - static void on_close_delete(uv_handle_t* handle); + + template + static void on_close_delete(uv_handle_t* handle) + { + delete reinterpret_cast(handle); + } protected: std::string RawReadBuffer; diff --git a/Source/cmPipeConnection.cxx b/Source/cmPipeConnection.cxx index b18a1d6..9e565f6 100644 --- a/Source/cmPipeConnection.cxx +++ b/Source/cmPipeConnection.cxx @@ -19,7 +19,8 @@ void cmPipeConnection::Connect(uv_stream_t* server) uv_pipe_init(this->Server->GetLoop(), rejectPipe, 0); uv_accept(server, reinterpret_cast(rejectPipe)); - uv_close(reinterpret_cast(rejectPipe), &on_close_delete); + uv_close(reinterpret_cast(rejectPipe), + &on_close_delete); return; } @@ -28,7 +29,8 @@ void cmPipeConnection::Connect(uv_stream_t* server) this->ClientPipe->data = static_cast(this); auto client = reinterpret_cast(this->ClientPipe); if (uv_accept(server, client) != 0) { - uv_close(reinterpret_cast(client), &on_close_delete); + uv_close(reinterpret_cast(client), + &on_close_delete); this->ClientPipe = nullptr; return; } @@ -65,15 +67,16 @@ bool cmPipeConnection::OnConnectionShuttingDown() { if (this->ClientPipe) { uv_close(reinterpret_cast(this->ClientPipe), - &on_close_delete); + &on_close_delete); this->WriteStream->data = nullptr; } - uv_close(reinterpret_cast(this->ServerPipe), &on_close_delete); + uv_close(reinterpret_cast(this->ServerPipe), + &on_close_delete); this->ClientPipe = nullptr; this->ServerPipe = nullptr; this->WriteStream = nullptr; this->ReadStream = nullptr; - return cmConnection::OnConnectionShuttingDown(); + return cmEventBasedConnection::OnConnectionShuttingDown(); } diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index c3e6811..f14e755 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -416,9 +416,18 @@ static void __start_thread(void* arg) server->Serve(&error); } +static void __shutdownThread(uv_async_t* arg) +{ + auto server = reinterpret_cast(arg->data); + on_walk_to_shutdown(reinterpret_cast(arg), nullptr); + server->StartShutDown(); +} + bool cmServerBase::StartServeThread() { ServeThreadRunning = true; + uv_async_init(&Loop, &this->ShutdownSignal, __shutdownThread); + this->ShutdownSignal.data = this; uv_thread_create(&ServeThread, __start_thread, this); return true; } @@ -464,8 +473,6 @@ void cmServerBase::OnDisconnect() void cmServerBase::OnServeStart() { - uv_signal_start(&this->SIGINTHandler, &on_signal, SIGINT); - uv_signal_start(&this->SIGHUPHandler, &on_signal, SIGHUP); } void cmServerBase::StartShutDown() @@ -485,11 +492,7 @@ void cmServerBase::StartShutDown() } Connections.clear(); - uv_stop(&Loop); - uv_walk(&Loop, on_walk_to_shutdown, nullptr); - - uv_run(&Loop, UV_RUN_DEFAULT); } bool cmServerBase::OnSignal(int signum) @@ -503,12 +506,6 @@ cmServerBase::cmServerBase(cmConnection* connection) { uv_loop_init(&Loop); - uv_signal_init(&Loop, &this->SIGINTHandler); - uv_signal_init(&Loop, &this->SIGHUPHandler); - - this->SIGINTHandler.data = this; - this->SIGHUPHandler.data = this; - AddNewConnection(connection); } @@ -516,7 +513,7 @@ cmServerBase::~cmServerBase() { if (ServeThreadRunning) { - StartShutDown(); + uv_async_send(&this->ShutdownSignal); uv_thread_join(&ServeThread); } diff --git a/Source/cmServer.h b/Source/cmServer.h index 9d8473d..93ac69e 100644 --- a/Source/cmServer.h +++ b/Source/cmServer.h @@ -66,6 +66,7 @@ protected: bool ServeThreadRunning = false; uv_thread_t ServeThread; + uv_async_t ShutdownSignal; uv_loop_t Loop; diff --git a/Source/cmServerConnection.cxx b/Source/cmServerConnection.cxx index 4891131..d6bf1a8 100644 --- a/Source/cmServerConnection.cxx +++ b/Source/cmServerConnection.cxx @@ -62,14 +62,14 @@ bool cmStdIoConnection::OnConnectionShuttingDown() if (usesTty) { uv_read_stop(reinterpret_cast(this->Input.tty)); uv_close(reinterpret_cast(this->Input.tty), - &on_close_delete); + &on_close_delete); uv_close(reinterpret_cast(this->Output.tty), - &on_close_delete); + &on_close_delete); } else { uv_close(reinterpret_cast(this->Input.pipe), - &on_close_delete); + &on_close_delete); uv_close(reinterpret_cast(this->Output.pipe), - &on_close_delete); + &on_close_delete); } return true; -- cgit v0.12 From 1a50cd8c683413154a700bf4ef17e621e8e89d7a Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Sun, 23 Jul 2017 11:54:42 -0600 Subject: server: Fixed minor memory leaks --- Source/cmServer.cxx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index f14e755..2638ec2 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -57,10 +57,6 @@ cmServer::cmServer(cmConnection* conn, bool supportExperimental) cmServer::~cmServer() { - if (!this->Protocol) { // Server was never fully started! - return; - } - for (cmServerProtocol* p : this->SupportedProtocols) { delete p; } @@ -110,6 +106,7 @@ void cmServer::ProcessRequest(cmConnection* connection, void cmServer::RegisterProtocol(cmServerProtocol* protocol) { if (protocol->IsExperimental() && !this->SupportExperimental) { + delete protocol; return; } auto version = protocol->ProtocolVersion(); -- cgit v0.12 From 6afc7f8828e5199395edfb10ac1a55891596907d Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Sun, 23 Jul 2017 12:25:05 -0600 Subject: server: Remove unused fields / functions --- Source/cmServer.cxx | 4 ---- Source/cmServer.h | 19 +------------------ 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index 2638ec2..14e1fd1 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -464,10 +464,6 @@ void cmServerBase::OnConnected(cmConnection*) { } -void cmServerBase::OnDisconnect() -{ -} - void cmServerBase::OnServeStart() { } diff --git a/Source/cmServer.h b/Source/cmServer.h index 93ac69e..cb7df10 100644 --- a/Source/cmServer.h +++ b/Source/cmServer.h @@ -37,13 +37,12 @@ public: * This should almost always be called by the given connections * directly. * - * @param connection The connectiont the request was received on + * @param connection The connection the request was received on * @param request The actual request */ virtual void ProcessRequest(cmConnection* connection, const std::string& request) = 0; virtual void OnConnected(cmConnection* connection); - virtual void OnDisconnect(); /*** * Start a dedicated thread. If this is used to start the server, it will @@ -141,22 +140,6 @@ private: cmServerProtocol* Protocol = nullptr; std::vector SupportedProtocols; - std::string DataBuffer; - std::string JsonData; - - typedef union - { - uv_tty_t tty; - uv_pipe_t pipe; - } InOutUnion; - - InOutUnion Input; - InOutUnion Output; - uv_stream_t* InputStream = nullptr; - uv_stream_t* OutputStream = nullptr; - - mutable bool Writing = false; - friend class cmServerProtocol; friend class cmServerRequest; }; -- cgit v0.12 From f8fd5a979cb4acd8a37632c29b453a426e798178 Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Fri, 21 Jul 2017 09:35:41 -0600 Subject: server: Made stdio connection accept different types of streams --- Source/cmConnection.cxx | 9 ++- Source/cmServerConnection.cxx | 148 +++++++++++++++++++++++++++++------------- Source/cmServerConnection.h | 12 +--- 3 files changed, 112 insertions(+), 57 deletions(-) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index bc29e41..6cf8e5b 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -144,9 +144,12 @@ bool cmConnection::OnServeStart(std::string* errString) bool cmEventBasedConnection::OnConnectionShuttingDown() { - this->WriteStream->data = nullptr; - this->ReadStream->data = nullptr; - + if (this->WriteStream) { + this->WriteStream->data = nullptr; + } + if (this->ReadStream) { + this->ReadStream->data = nullptr; + } this->ReadStream = nullptr; this->WriteStream = nullptr; return true; diff --git a/Source/cmServerConnection.cxx b/Source/cmServerConnection.cxx index d6bf1a8..dd14932 100644 --- a/Source/cmServerConnection.cxx +++ b/Source/cmServerConnection.cxx @@ -2,76 +2,136 @@ file Copyright.txt or https://cmake.org/licensing for details. */ #include "cmServerConnection.h" +#include "cmConfigure.h" #include "cmServer.h" #include "cmServerDictionary.h" +#ifdef _WIN32 +#include "io.h" +#else +#include +#endif +#include cmStdIoConnection::cmStdIoConnection( cmConnectionBufferStrategy* bufferStrategy) : cmEventBasedConnection(bufferStrategy) - , Input() - , Output() { } +void cmStdIoConnection::SetupStream(uv_stream_t*& stream, int file_id) +{ + assert(stream == nullptr); + switch (uv_guess_handle(file_id)) { + case UV_TTY: { + auto tty = new uv_tty_t(); + uv_tty_init(this->Server->GetLoop(), tty, file_id, file_id == 0); + uv_tty_set_mode(tty, UV_TTY_MODE_NORMAL); + stream = reinterpret_cast(tty); + break; + } + case UV_FILE: + if (file_id == 0) { + return; + } + // Intentional fallthrough; stdin can _not_ be treated as a named + // pipe, however stdout can be. + CM_FALLTHROUGH; + case UV_NAMED_PIPE: { + auto pipe = new uv_pipe_t(); + uv_pipe_init(this->Server->GetLoop(), pipe, 0); + uv_pipe_open(pipe, file_id); + stream = reinterpret_cast(pipe); + break; + } + default: + assert(false && "Unable to determine stream type"); + return; + } + stream->data = static_cast(this); +} + void cmStdIoConnection::SetServer(cmServerBase* s) { cmConnection::SetServer(s); - if (uv_guess_handle(1) == UV_TTY) { - usesTty = true; - - this->Input.tty = new uv_tty_t(); - uv_tty_init(this->Server->GetLoop(), this->Input.tty, 0, 1); - uv_tty_set_mode(this->Input.tty, UV_TTY_MODE_NORMAL); - this->Input.tty->data = static_cast(this); - this->ReadStream = reinterpret_cast(this->Input.tty); - - this->Output.tty = new uv_tty_t(); - uv_tty_init(this->Server->GetLoop(), this->Output.tty, 1, 0); - uv_tty_set_mode(this->Output.tty, UV_TTY_MODE_NORMAL); - this->Output.tty->data = static_cast(this); - this->WriteStream = reinterpret_cast(this->Output.tty); - } else { - usesTty = false; - - this->Input.pipe = new uv_pipe_t(); - uv_pipe_init(this->Server->GetLoop(), this->Input.pipe, 0); - uv_pipe_open(this->Input.pipe, 0); - this->Input.pipe->data = static_cast(this); - this->ReadStream = reinterpret_cast(this->Input.pipe); - - this->Output.pipe = new uv_pipe_t(); - uv_pipe_init(this->Server->GetLoop(), this->Output.pipe, 0); - uv_pipe_open(this->Output.pipe, 1); - this->Output.pipe->data = static_cast(this); - this->WriteStream = reinterpret_cast(this->Output.pipe); + SetupStream(this->ReadStream, 0); + SetupStream(this->WriteStream, 1); +} + +void shutdown_connection(uv_prepare_t* prepare) +{ + cmStdIoConnection* connection = + reinterpret_cast(prepare->data); + + if (!uv_is_closing(reinterpret_cast(prepare))) { + uv_close(reinterpret_cast(prepare), + &cmEventBasedConnection::on_close_delete); } + connection->OnDisconnect(0); } bool cmStdIoConnection::OnServeStart(std::string* pString) { - uv_read_start(this->ReadStream, on_alloc_buffer, on_read); Server->OnConnected(this); + if (this->ReadStream) { + uv_read_start(this->ReadStream, on_alloc_buffer, on_read); + } else if (uv_guess_handle(0) == UV_FILE) { + char buffer[1024]; + while (auto len = read(0, buffer, sizeof(buffer))) { + ReadData(std::string(buffer, buffer + len)); + } + + // We can't start the disconnect from here, add a prepare hook to do that + // for us + auto prepare = new uv_prepare_t(); + prepare->data = this; + uv_prepare_init(Server->GetLoop(), prepare); + uv_prepare_start(prepare, shutdown_connection); + } return cmConnection::OnServeStart(pString); } -bool cmStdIoConnection::OnConnectionShuttingDown() +void cmStdIoConnection::ShutdownStream(uv_stream_t*& stream) { - cmEventBasedConnection::OnConnectionShuttingDown(); + if (!stream) { + return; + } + switch (stream->type) { + case UV_TTY: { + assert(!uv_is_closing(reinterpret_cast(stream))); + if (!uv_is_closing(reinterpret_cast(stream))) { + uv_close(reinterpret_cast(stream), + &on_close_delete); + } + break; + } + case UV_FILE: + case UV_NAMED_PIPE: { + assert(!uv_is_closing(reinterpret_cast(stream))); + if (!uv_is_closing(reinterpret_cast(stream))) { + uv_close(reinterpret_cast(stream), + &on_close_delete); + } + break; + } + default: + assert(false && "Unable to determine stream type"); + } - if (usesTty) { - uv_read_stop(reinterpret_cast(this->Input.tty)); - uv_close(reinterpret_cast(this->Input.tty), - &on_close_delete); - uv_close(reinterpret_cast(this->Output.tty), - &on_close_delete); - } else { - uv_close(reinterpret_cast(this->Input.pipe), - &on_close_delete); - uv_close(reinterpret_cast(this->Output.pipe), - &on_close_delete); + stream = nullptr; +} + +bool cmStdIoConnection::OnConnectionShuttingDown() +{ + if (ReadStream) { + uv_read_stop(ReadStream); } + ShutdownStream(ReadStream); + ShutdownStream(WriteStream); + + cmEventBasedConnection::OnConnectionShuttingDown(); + return true; } diff --git a/Source/cmServerConnection.h b/Source/cmServerConnection.h index df404ce..7b0c9b6 100644 --- a/Source/cmServerConnection.h +++ b/Source/cmServerConnection.h @@ -45,16 +45,8 @@ public: bool OnServeStart(std::string* pString) override; private: - typedef union - { - uv_tty_t* tty; - uv_pipe_t* pipe; - } InOutUnion; - - bool usesTty = false; - - InOutUnion Input; - InOutUnion Output; + void SetupStream(uv_stream_t*& stream, int file_id); + void ShutdownStream(uv_stream_t*& stream); }; /*** -- cgit v0.12 From dc7a18d82eb0013a2afbdea9ba5fec131fc3179f Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Sat, 22 Jul 2017 17:23:11 -0600 Subject: server: test buffer parsing --- CMakeLists.txt | 3 + Tests/CMakeLists.txt | 4 ++ Tests/CMakeServerLib/CMakeLists.txt | 17 ++++++ Tests/CMakeServerLib/testServerBuffering.cpp | 86 ++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+) create mode 100644 Tests/CMakeServerLib/CMakeLists.txt create mode 100644 Tests/CMakeServerLib/testServerBuffering.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c9e632e..c578ec3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -753,6 +753,9 @@ add_subdirectory(Tests) if(NOT CMake_TEST_EXTERNAL_CMAKE) if(BUILD_TESTING) CMAKE_SET_TARGET_FOLDER(CMakeLibTests "Tests") + IF(TARGET CMakeServerLibTests) + CMAKE_SET_TARGET_FOLDER(CMakeServerLibTests "Tests") + ENDIF() endif() if(TARGET documentation) CMAKE_SET_TARGET_FOLDER(documentation "Documentation") diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index f0e58ee..516bc89 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -146,6 +146,10 @@ if(BUILD_TESTING) if(NOT CMake_TEST_EXTERNAL_CMAKE) add_subdirectory(CMakeLib) + + if(CMake_TEST_SERVER_MODE) + add_subdirectory(CMakeServerLib) + endif() endif() add_subdirectory(CMakeOnly) add_subdirectory(RunCMake) diff --git a/Tests/CMakeServerLib/CMakeLists.txt b/Tests/CMakeServerLib/CMakeLists.txt new file mode 100644 index 0000000..f5351fd --- /dev/null +++ b/Tests/CMakeServerLib/CMakeLists.txt @@ -0,0 +1,17 @@ +include_directories( + ${CMAKE_CURRENT_BINARY_DIR} + ${CMake_BINARY_DIR}/Source + ${CMake_SOURCE_DIR}/Source + ) + +set(CMakeServerLib_TESTS + testServerBuffering + ) + +create_test_sourcelist(CMakeLib_TEST_SRCS CMakeServerLibTests.cxx ${CMakeServerLib_TESTS}) +add_executable(CMakeServerLibTests ${CMakeLib_TEST_SRCS}) +target_link_libraries(CMakeServerLibTests CMakeLib CMakeServerLib) + +foreach(test ${CMakeServerLib_TESTS}) + add_test(CMakeServerLib.${test} CMakeServerLibTests ${test} ${${test}_ARGS}) +endforeach() diff --git a/Tests/CMakeServerLib/testServerBuffering.cpp b/Tests/CMakeServerLib/testServerBuffering.cpp new file mode 100644 index 0000000..97be891 --- /dev/null +++ b/Tests/CMakeServerLib/testServerBuffering.cpp @@ -0,0 +1,86 @@ +#include "cmConnection.h" +#include "cmServerConnection.h" +#include +#include +#include +#include + +void print_error(const std::vector& input, + const std::vector& output) +{ + std::cerr << "Responses don't equal input messages input." << std::endl; + std::cerr << "Responses: " << std::endl; + + for (auto& msg : output) { + std::cerr << "'" << msg << "'" << std::endl; + } + + std::cerr << "Input messages" << std::endl; + for (auto& msg : input) { + std::cerr << "'" << msg << "'" << std::endl; + } +} + +std::string trim_newline(const std::string& _buffer) +{ + auto buffer = _buffer; + while (!buffer.empty() && (buffer.back() == '\n' || buffer.back() == '\r')) { + buffer.pop_back(); + } + return buffer; +} + +int testServerBuffering(int, char** const) +{ + std::vector messages = { + "{ \"test\": 10}", "{ \"test\": { \"test2\": false} }", + "{ \"test\": [1, 2, 3] }", + "{ \"a\": { \"1\": {}, \n\n\n \"2\":[] \t\t\t\t}}" + }; + + std::string fullMessage; + for (auto& msg : messages) { + fullMessage += "[== \"CMake Server\" ==[\n"; + fullMessage += msg; + fullMessage += "\n]== \"CMake Server\" ==]\n"; + } + + // The buffering strategy should cope with any fragmentation, including + // just getting the characters one at a time. + auto bufferingStrategy = + std::unique_ptr(new cmServerBufferStrategy); + std::vector response; + std::string rawBuffer; + for (size_t i = 0; i < fullMessage.size(); i++) { + rawBuffer += fullMessage[i]; + std::string packet = bufferingStrategy->BufferMessage(rawBuffer); + do { + if (!packet.empty() && packet != "\r\n") { + response.push_back(trim_newline(packet)); + } + packet = bufferingStrategy->BufferMessage(rawBuffer); + } while (!packet.empty()); + } + + if (response != messages) { + print_error(messages, response); + return 1; + } + + // We should also be able to deal with getting a bunch at once + response.clear(); + std::string packet = bufferingStrategy->BufferMessage(fullMessage); + do { + if (!packet.empty() && packet != "\r\n") { + response.push_back(trim_newline(packet)); + } + packet = bufferingStrategy->BufferMessage(fullMessage); + } while (!packet.empty()); + + if (response != messages) { + print_error(messages, response); + return 1; + } + + return 0; +} -- cgit v0.12 From 7ef28843618519c222806a0df82ed8f87ad2ca0c Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Wed, 19 Jul 2017 20:23:34 -0600 Subject: server: Moved buffer formatting into bufferstrategy --- Source/cmConnection.cxx | 6 +++++- Source/cmConnection.h | 11 +++++++++++ Source/cmServer.cxx | 3 +-- Source/cmServerConnection.cxx | 7 +++++++ Source/cmServerConnection.h | 1 + 5 files changed, 25 insertions(+), 3 deletions(-) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index 6cf8e5b..89013dc 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -67,9 +67,13 @@ bool cmEventBasedConnection::IsOpen() const return this->WriteStream != nullptr; } -void cmEventBasedConnection::WriteData(const std::string& data) +void cmEventBasedConnection::WriteData(const std::string& _data) { + auto data = _data; assert(this->WriteStream); + if (BufferStrategy) { + data = BufferStrategy->BufferOutMessage(data); + } auto ds = data.size(); diff --git a/Source/cmConnection.h b/Source/cmConnection.h index b1b51fe..ddb7744 100644 --- a/Source/cmConnection.h +++ b/Source/cmConnection.h @@ -39,6 +39,17 @@ public: virtual std::string BufferMessage(std::string& rawBuffer) = 0; /*** + * Called to properly buffer an outgoing message. + * + * @param rawBuffer Message to format in the correct way + * + * @return Formatted message + */ + virtual std::string BufferOutMessage(const std::string& rawBuffer) const + { + return rawBuffer; + }; + /*** * Resets the internal state of the buffering */ virtual void clear(); diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index 14e1fd1..6a63797 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -282,8 +282,7 @@ void cmServer::WriteJsonObject(cmConnection* connection, } } - connection->WriteData(std::string("\n") + kSTART_MAGIC + std::string("\n") + - result + kEND_MAGIC + std::string("\n")); + connection->WriteData(result); } cmServerProtocol* cmServer::FindMatchingProtocol( diff --git a/Source/cmServerConnection.cxx b/Source/cmServerConnection.cxx index dd14932..e686403 100644 --- a/Source/cmServerConnection.cxx +++ b/Source/cmServerConnection.cxx @@ -153,6 +153,13 @@ void cmConnectionBufferStrategy::clear() { } +std::string cmServerBufferStrategy::BufferOutMessage( + const std::string& rawBuffer) const +{ + return std::string("\n") + kSTART_MAGIC + std::string("\n") + rawBuffer + + kEND_MAGIC + std::string("\n"); +} + std::string cmServerBufferStrategy::BufferMessage(std::string& RawReadBuffer) { for (;;) { diff --git a/Source/cmServerConnection.h b/Source/cmServerConnection.h index 7b0c9b6..4ca908d 100644 --- a/Source/cmServerConnection.h +++ b/Source/cmServerConnection.h @@ -25,6 +25,7 @@ class cmServerBufferStrategy : public cmConnectionBufferStrategy { public: std::string BufferMessage(std::string& rawBuffer) override; + std::string BufferOutMessage(const std::string& rawBuffer) const override; private: std::string RequestBuffer; -- cgit v0.12 From 882dcef8e02e432e6469b7fca38aff212d1541ab Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Wed, 19 Jul 2017 22:04:13 -0600 Subject: server: Made connections in a server have a mutex to avoid use after frees --- Source/cmServer.cxx | 35 ++++++++++++++++++++++++++++------- Source/cmServer.h | 1 + 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index 6a63797..c7f8704 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -244,9 +244,11 @@ cmFileMonitor* cmServer::FileMonitor() const void cmServer::WriteJsonObject(const Json::Value& jsonValue, const DebugInfo* debug) const { + uv_rwlock_rdlock(&ConnectionsMutex); for (auto& connection : this->Connections) { WriteJsonObject(connection.get(), jsonValue, debug); } + uv_rwlock_rdunlock(&ConnectionsMutex); } void cmServer::WriteJsonObject(cmConnection* connection, @@ -443,10 +445,15 @@ bool cmServerBase::Serve(std::string* errorMessage) OnServeStart(); - for (auto& connection : Connections) { - if (!connection->OnServeStart(errorMessage)) { - return false; + { + uv_rwlock_rdlock(&ConnectionsMutex); + for (auto& connection : Connections) { + if (!connection->OnServeStart(errorMessage)) { + uv_rwlock_rdunlock(&ConnectionsMutex); + return false; + } } + uv_rwlock_rdunlock(&ConnectionsMutex); } if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { @@ -479,10 +486,14 @@ void cmServerBase::StartShutDown() uv_signal_stop(&this->SIGHUPHandler); } - for (auto& connection : Connections) { - connection->OnConnectionShuttingDown(); + { + uv_rwlock_wrlock(&ConnectionsMutex); + for (auto& connection : Connections) { + connection->OnConnectionShuttingDown(); + } + Connections.clear(); + uv_rwlock_wrunlock(&ConnectionsMutex); } - Connections.clear(); uv_walk(&Loop, on_walk_to_shutdown, nullptr); } @@ -496,7 +507,12 @@ bool cmServerBase::OnSignal(int signum) cmServerBase::cmServerBase(cmConnection* connection) { - uv_loop_init(&Loop); + auto err = uv_loop_init(&Loop); + (void)err; + assert(err == 0); + + err = uv_rwlock_init(&ConnectionsMutex); + assert(err == 0); AddNewConnection(connection); } @@ -510,11 +526,14 @@ cmServerBase::~cmServerBase() } uv_loop_close(&Loop); + uv_rwlock_destroy(&ConnectionsMutex); } void cmServerBase::AddNewConnection(cmConnection* ownedConnection) { + uv_rwlock_wrlock(&ConnectionsMutex); Connections.emplace_back(ownedConnection); + uv_rwlock_wrunlock(&ConnectionsMutex); ownedConnection->SetServer(this); } @@ -528,9 +547,11 @@ void cmServerBase::OnDisconnect(cmConnection* pConnection) auto pred = [pConnection](const std::unique_ptr& m) { return m.get() == pConnection; }; + uv_rwlock_wrlock(&ConnectionsMutex); Connections.erase( std::remove_if(Connections.begin(), Connections.end(), pred), Connections.end()); + uv_rwlock_wrunlock(&ConnectionsMutex); if (Connections.empty()) { StartShutDown(); } diff --git a/Source/cmServer.h b/Source/cmServer.h index cb7df10..d8f73c1 100644 --- a/Source/cmServer.h +++ b/Source/cmServer.h @@ -61,6 +61,7 @@ public: void OnDisconnect(cmConnection* pConnection); protected: + mutable uv_rwlock_t ConnectionsMutex; std::vector> Connections; bool ServeThreadRunning = false; -- cgit v0.12 From 693fa0a96e111270337eb76a4da1255774657e1a Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Tue, 22 Aug 2017 09:46:00 -0600 Subject: server: Added assert to monitor uv_run status --- Source/cmServer.cxx | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index c7f8704..b1b4020 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -411,7 +412,10 @@ static void __start_thread(void* arg) { auto server = reinterpret_cast(arg); std::string error; - server->Serve(&error); + bool success = server->Serve(&error); + if (!success || error.empty() == false) { + std::cerr << "Error during serve: " << error << std::endl; + } } static void __shutdownThread(uv_async_t* arg) @@ -457,8 +461,12 @@ bool cmServerBase::Serve(std::string* errorMessage) } if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { + // It is important we don't ever let the event loop exit with open handles + // at best this is a memory leak, but it can also introduce race conditions + // which can hang the program. + assert(false && "Event loop stopped in unclean state."); + *errorMessage = "Internal Error: Event loop stopped in unclean state."; - StartShutDown(); return false; } -- cgit v0.12 From 124424e9974ade26521bf4751f3701c1e7d91c3d Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Thu, 20 Jul 2017 21:18:41 -0600 Subject: server: Protect several fields from potentially pointing to bad memory --- Source/cmConnection.cxx | 5 ++++- Source/cmServerConnection.cxx | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index 89013dc..c0d1b82 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -118,7 +118,9 @@ void cmEventBasedConnection::OnDisconnect(int onerror) { (void)onerror; this->OnConnectionShuttingDown(); - this->Server->OnDisconnect(this); + if (this->Server) { + this->Server->OnDisconnect(this); + } } cmConnection::~cmConnection() @@ -127,6 +129,7 @@ cmConnection::~cmConnection() bool cmConnection::OnConnectionShuttingDown() { + this->Server = nullptr; return true; } diff --git a/Source/cmServerConnection.cxx b/Source/cmServerConnection.cxx index e686403..44af75f 100644 --- a/Source/cmServerConnection.cxx +++ b/Source/cmServerConnection.cxx @@ -53,6 +53,9 @@ void cmStdIoConnection::SetupStream(uv_stream_t*& stream, int file_id) void cmStdIoConnection::SetServer(cmServerBase* s) { cmConnection::SetServer(s); + if (!s) { + return; + } SetupStream(this->ReadStream, 0); SetupStream(this->WriteStream, 1); -- cgit v0.12 From 0e4d503384a254c843a065c26f77bc19dfccff16 Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Sat, 22 Jul 2017 09:29:18 -0600 Subject: server: Added thread check to protect writedata --- Source/cmConnection.cxx | 6 ++++++ Source/cmServer.cxx | 6 ++++++ Source/cmServer.h | 9 +++++++++ 3 files changed, 21 insertions(+) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index c0d1b82..f482412 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -69,6 +69,12 @@ bool cmEventBasedConnection::IsOpen() const void cmEventBasedConnection::WriteData(const std::string& _data) { +#ifndef NDEBUG + auto curr_thread_id = uv_thread_self(); + assert(this->Server); + assert(uv_thread_equal(&curr_thread_id, &this->Server->ServeThreadId)); +#endif + auto data = _data; assert(this->WriteStream); if (BufferStrategy) { diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index b1b4020..30d0f51 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -436,6 +436,12 @@ bool cmServerBase::StartServeThread() bool cmServerBase::Serve(std::string* errorMessage) { +#ifndef NDEBUG + uv_thread_t blank_thread_t = {}; + assert(uv_thread_equal(&blank_thread_t, &ServeThreadId)); + ServeThreadId = uv_thread_self(); +#endif + errorMessage->clear(); uv_signal_init(&Loop, &this->SIGINTHandler); diff --git a/Source/cmServer.h b/Source/cmServer.h index d8f73c1..15fd2ba 100644 --- a/Source/cmServer.h +++ b/Source/cmServer.h @@ -67,6 +67,15 @@ protected: bool ServeThreadRunning = false; uv_thread_t ServeThread; uv_async_t ShutdownSignal; +#ifndef NDEBUG +public: + // When the server starts it will mark down it's current thread ID, + // which is useful in other contexts to just assert that operations + // are performed on that same thread. + uv_thread_t ServeThreadId = {}; + +protected: +#endif uv_loop_t Loop; -- cgit v0.12