From d5279442b90eb2055b2e765d4854c4dead48b586 Mon Sep 17 00:00:00 2001 From: Bryan Biedenkapp Date: Mon, 27 Mar 2023 10:15:10 -0400 Subject: [PATCH] refactor REST connection handler into separate client/server classes; correct issue with improper handling of resolved endpoint data resulting in crash conditions; fix memory leak issue by not deleting an array when done; --- src/network/rest/http/ClientConnection.h | 170 ++++++++++++++++++ src/network/rest/http/HTTPClient.h | 43 ++--- src/network/rest/http/HTTPServer.h | 8 +- .../http/{Connection.h => ServerConnection.h} | 100 +++-------- ...ionManager.h => ServerConnectionManager.h} | 18 +- src/remote/RESTClient.cpp | 5 +- 6 files changed, 233 insertions(+), 111 deletions(-) create mode 100644 src/network/rest/http/ClientConnection.h rename src/network/rest/http/{Connection.h => ServerConnection.h} (66%) rename src/network/rest/http/{ConnectionManager.h => ServerConnectionManager.h} (86%) diff --git a/src/network/rest/http/ClientConnection.h b/src/network/rest/http/ClientConnection.h new file mode 100644 index 00000000..357f7f3e --- /dev/null +++ b/src/network/rest/http/ClientConnection.h @@ -0,0 +1,170 @@ +/** +* Digital Voice Modem - Host Software +* GPLv2 Open Source. Use is subject to license terms. +* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. +* +* @package DVM / Host Software +* +*/ +// +// Based on code from the CRUD project. (https://github.com/venediktov/CRUD) +// Licensed under the BPL-1.0 License (https://opensource.org/license/bsl1-0-html) +// +/* +* Copyright (c) 2003-2013 Christopher M. Kohlhoff +* Copyright (C) 2023 by Bryan Biedenkapp N2PLL +* +* Permission is hereby granted, free of charge, to any person or organization +* obtaining a copy of the software and accompanying documentation covered by +* this license (the “Software”) to use, reproduce, display, distribute, execute, +* and transmit the Software, and to prepare derivative works of the Software, and +* to permit third-parties to whom the Software is furnished to do so, all subject +* to the following: +* +* The copyright notices in the Software and this entire statement, including the +* above license grant, this restriction and the following disclaimer, must be included +* in all copies of the Software, in whole or in part, and all derivative works of the +* Software, unless such copies or derivative works are solely in the form of +* machine-executable object code generated by a source language processor. +* +* THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +* PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR ANYONE +* DISTRIBUTING THE SOFTWARE BE LIABLE FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN +* CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE +* OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +*/ +#if !defined(__REST_HTTP__CLIENT_CONNECTION_H__) +#define __REST_HTTP__CLIENT_CONNECTION_H__ + +#include "Defines.h" +#include "network/rest/http/HTTPLexer.h" +#include "network/rest/http/HTTPPayload.h" +#include "Utils.h" + +#include +#include +#include +#include +#include + +namespace network +{ + namespace rest + { + namespace http + { + // --------------------------------------------------------------------------- + // Class Declaration + // This class represents a single connection from a client. + // --------------------------------------------------------------------------- + + template + class ClientConnection + { + public: + /// Initializes a new instance of the ClientConnection class. + explicit ClientConnection(asio::ip::tcp::socket socket, RequestHandlerType& handler) : + m_socket(std::move(socket)), + m_requestHandler(handler), + m_lexer(HTTPLexer(true)) + { + /* stub */ + } + /// Initializes a copy instance of the ClientConnection class. + ClientConnection(const ClientConnection&) = delete; + + /// + ClientConnection& operator=(const ClientConnection&) = delete; + + /// Start the first asynchronous operation for the connection. + void start() { read(); } + /// Stop all asynchronous operations associated with the connection. + void stop() + { + try + { + if (m_socket.is_open()) { + m_socket.close(); + } + } + catch(const std::exception&) { /* ignore */ } + } + + /// Perform an synchronous write operation. + void send(HTTPPayload request) + { + request.attachHostHeader(m_socket.remote_endpoint()); + write(request); + } + private: + /// Perform an asynchronous read operation. + void read() + { + m_socket.async_read_some(asio::buffer(m_buffer), [=](asio::error_code ec, std::size_t bytes_transferred) { + if (!ec) { + HTTPLexer::ResultType result; + char* content; + + std::tie(result, content) = m_lexer.parse(m_request, m_buffer.data(), m_buffer.data() + bytes_transferred); + + std::string contentLength = m_request.headers.find("Content-Length"); + if (contentLength != "") { + size_t length = (size_t)::strtoul(contentLength.c_str(), NULL, 10); + m_request.content = std::string(content, length); + } + + m_request.headers.add("RemoteHost", m_socket.remote_endpoint().address().to_string()); + + if (result == HTTPLexer::GOOD) { + m_requestHandler.handleRequest(m_request, m_reply); + } + else if (result == HTTPLexer::BAD) { + return; + } + else { + read(); + } + } + else if (ec != asio::error::operation_aborted) { + if (m_socket.is_open()) { + m_socket.close(); + } + } + }); + } + + /// Perform an synchronous write operation. + void write(HTTPPayload request) + { + try + { + auto buffers = request.toBuffers(); + asio::write(m_socket, buffers); + } + catch(const asio::system_error& e) + { + asio::error_code ec = e.code(); + if (ec) { + // initiate graceful connection closure + asio::error_code ignored_ec; + m_socket.shutdown(asio::ip::tcp::socket::shutdown_both, ignored_ec); + } + } + } + + asio::ip::tcp::socket m_socket; + + RequestHandlerType& m_requestHandler; + + std::array m_buffer; + + HTTPPayload m_request; + HTTPLexer m_lexer; + HTTPPayload m_reply; + }; + } // namespace http + } // namespace rest +} // namespace network + +#endif // __REST_HTTP__CLIENT_CONNECTION_H__ diff --git a/src/network/rest/http/HTTPClient.h b/src/network/rest/http/HTTPClient.h index 368a1ee4..8ee234e8 100644 --- a/src/network/rest/http/HTTPClient.h +++ b/src/network/rest/http/HTTPClient.h @@ -33,8 +33,7 @@ #define __REST_HTTP__HTTP_CLIENT_H__ #include "Defines.h" -#include "network/rest/http/Connection.h" -#include "network/rest/http/ConnectionManager.h" +#include "network/rest/http/ClientConnection.h" #include "network/rest/http/HTTPRequestHandler.h" #include "Thread.h" @@ -59,7 +58,7 @@ namespace network // This class implements top-level routines of the HTTP client. // --------------------------------------------------------------------------- - template class ConnectionImpl = Connection> + template class ConnectionImpl = ClientConnection> class HTTPClient : private Thread { public: /// Initializes a new instance of the HTTPClient class. @@ -68,7 +67,6 @@ namespace network m_port(port), m_connection(nullptr), m_ioContext(), - m_connectionManager(), m_socket(m_ioContext), m_requestHandler() { @@ -115,6 +113,8 @@ namespace network if (m_connection != nullptr) { m_connection->stop(); + delete m_connection; + m_connection = nullptr; } wait(); @@ -124,47 +124,48 @@ namespace network /// virtual void entry() { - asio::ip::tcp::resolver resolver(m_ioContext); - auto endpoints = resolver.resolve(m_address, std::to_string(m_port)); + while (m_running) { + asio::ip::tcp::resolver resolver(m_ioContext); + auto endpoints = resolver.resolve(m_address, std::to_string(m_port)); - connect(endpoints); + connect(endpoints); - while (m_running) { // the entry() call will block until all asynchronous operations // have finished m_ioContext.run(); - m_ioContext.restart(); - connect(endpoints); } } /// Perform an asynchronous connect operation. void connect(asio::ip::basic_resolver_results& endpoints) { - if (m_connection != nullptr) { - m_connection->stop(); - m_socket = asio::ip::tcp::socket(m_ioContext); - } + std::lock_guard guard(m_lock); + { + if (m_connection != nullptr) { + m_connection->stop(); + delete m_connection; + m_connection = nullptr; + + m_socket = asio::ip::tcp::socket(m_ioContext); + } - asio::connect(m_socket, endpoints); - m_connection = std::make_shared(std::move(m_socket), m_connectionManager, m_requestHandler, false, true); - m_connection->start(); + asio::connect(m_socket, endpoints); + m_connection = new ConnectionType(std::move(m_socket), m_requestHandler); + m_connection->start(); + } } std::string m_address; uint16_t m_port; typedef ConnectionImpl ConnectionType; - typedef std::shared_ptr ConnectionTypePtr; - ConnectionTypePtr m_connection; + ConnectionType* m_connection; bool m_running = false; asio::io_context m_ioContext; - ConnectionManager m_connectionManager; - asio::ip::tcp::socket m_socket; RequestHandlerType m_requestHandler; diff --git a/src/network/rest/http/HTTPServer.h b/src/network/rest/http/HTTPServer.h index eb9640a6..1fe242ef 100644 --- a/src/network/rest/http/HTTPServer.h +++ b/src/network/rest/http/HTTPServer.h @@ -38,8 +38,8 @@ #define __REST_HTTP__HTTP_SERVER_H__ #include "Defines.h" -#include "network/rest/http/Connection.h" -#include "network/rest/http/ConnectionManager.h" +#include "network/rest/http/ServerConnection.h" +#include "network/rest/http/ServerConnectionManager.h" #include "network/rest/http/HTTPRequestHandler.h" #include @@ -62,7 +62,7 @@ namespace network // This class implements top-level routines of the HTTP server. // --------------------------------------------------------------------------- - template class ConnectionImpl = Connection> + template class ConnectionImpl = ServerConnection> class HTTPServer { public: /// Initializes a new instance of the HTTPServer class. @@ -143,7 +143,7 @@ namespace network asio::io_service m_ioService; asio::ip::tcp::acceptor m_acceptor; - ConnectionManager m_connectionManager; + ServerConnectionManager m_connectionManager; asio::ip::tcp::socket m_socket; diff --git a/src/network/rest/http/Connection.h b/src/network/rest/http/ServerConnection.h similarity index 66% rename from src/network/rest/http/Connection.h rename to src/network/rest/http/ServerConnection.h index 961360db..395a08a3 100644 --- a/src/network/rest/http/Connection.h +++ b/src/network/rest/http/ServerConnection.h @@ -34,8 +34,8 @@ * CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE * OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#if !defined(__REST_HTTP__CONNECTION_H__) -#define __REST_HTTP__CONNECTION_H__ +#if !defined(__REST_HTTP__SERVER_CONNECTION_H__) +#define __REST_HTTP__SERVER_CONNECTION_H__ #include "Defines.h" #include "network/rest/http/HTTPLexer.h" @@ -54,12 +54,11 @@ namespace network { namespace http { - // --------------------------------------------------------------------------- // Class Prototypes // --------------------------------------------------------------------------- - template class ConnectionManager; + template class ServerConnectionManager; // --------------------------------------------------------------------------- // Class Declaration @@ -67,29 +66,28 @@ namespace network // --------------------------------------------------------------------------- template - class Connection : public std::enable_shared_from_this> + class ServerConnection : public std::enable_shared_from_this> { - typedef Connection selfType; + typedef ServerConnection selfType; typedef std::shared_ptr selfTypePtr; - typedef ConnectionManager ConnectionManagerType; + typedef ServerConnectionManager ConnectionManagerType; public: - /// Initializes a new instance of the Connection class. - explicit Connection(asio::ip::tcp::socket socket, ConnectionManagerType& manager, RequestHandlerType& handler, - bool persistent = false, bool client = false) : + /// Initializes a new instance of the ServerConnection class. + explicit ServerConnection(asio::ip::tcp::socket socket, ConnectionManagerType& manager, RequestHandlerType& handler, + bool persistent = false) : m_socket(std::move(socket)), m_connectionManager(manager), m_requestHandler(handler), - m_lexer(HTTPLexer(client)), - m_persistent(persistent), - m_client(client) + m_lexer(HTTPLexer(false)), + m_persistent(persistent) { /* stub */ } - /// Initializes a copy instance of the Connection class. - Connection(const Connection&) = delete; + /// Initializes a copy instance of the ServerConnection class. + ServerConnection(const ServerConnection&) = delete; /// - Connection& operator=(const Connection&) = delete; + ServerConnection& operator=(const ServerConnection&) = delete; /// Start the first asynchronous operation for the connection. void start() { read(); } @@ -105,12 +103,6 @@ namespace network catch(const std::exception&) { /* ignore */ } } - /// Perform an synchronous write operation. - void send(HTTPPayload request) - { - request.attachHostHeader(m_socket.remote_endpoint()); - write(request); - } private: /// Perform an asynchronous read operation. void read() @@ -134,40 +126,20 @@ namespace network m_request.headers.add("RemoteHost", m_socket.remote_endpoint().address().to_string()); - if (m_client) { - if (result == HTTPLexer::GOOD) { - m_requestHandler.handleRequest(m_request, m_reply); - } - else if (result == HTTPLexer::BAD) { - return; - } - else { - read(); - } + if (result == HTTPLexer::GOOD) { + m_requestHandler.handleRequest(m_request, m_reply); + write(); + } + else if (result == HTTPLexer::BAD) { + m_reply = HTTPPayload::statusPayload(HTTPPayload::BAD_REQUEST); + write(); } else { - if (result == HTTPLexer::GOOD) { - m_requestHandler.handleRequest(m_request, m_reply); - write(); - } - else if (result == HTTPLexer::BAD) { - m_reply = HTTPPayload::statusPayload(HTTPPayload::BAD_REQUEST); - write(); - } - else { - read(); - } + read(); } } else if (ec != asio::error::operation_aborted) { - if (m_client) { - if (m_socket.is_open()) { - m_socket.close(); - } - } - else { - m_connectionManager.stop(this->shared_from_this()); - } + m_connectionManager.stop(this->shared_from_this()); } }); } @@ -175,10 +147,6 @@ namespace network /// Perform an asynchronous write operation. void write() { - if (m_client) { - return; - } - if (!m_persistent) { auto self(this->shared_from_this()); } else { @@ -209,25 +177,6 @@ namespace network }); } - /// Perform an synchronous write operation. - void write(HTTPPayload request) - { - try - { - auto buffers = request.toBuffers(); - asio::write(m_socket, buffers); - } - catch(const asio::system_error& e) - { - asio::error_code ec = e.code(); - if (ec) { - // initiate graceful connection closure - asio::error_code ignored_ec; - m_socket.shutdown(asio::ip::tcp::socket::shutdown_both, ignored_ec); - } - } - } - asio::ip::tcp::socket m_socket; ConnectionManagerType& m_connectionManager; @@ -240,10 +189,9 @@ namespace network HTTPPayload m_reply; bool m_persistent; - bool m_client; }; } // namespace http } // namespace rest } // namespace network -#endif // __REST_HTTP__CONNECTION_H__ +#endif // __REST_HTTP__SERVER_CONNECTION_H__ diff --git a/src/network/rest/http/ConnectionManager.h b/src/network/rest/http/ServerConnectionManager.h similarity index 86% rename from src/network/rest/http/ConnectionManager.h rename to src/network/rest/http/ServerConnectionManager.h index f3d1a3f9..94a9a526 100644 --- a/src/network/rest/http/ConnectionManager.h +++ b/src/network/rest/http/ServerConnectionManager.h @@ -34,8 +34,8 @@ * CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE * OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#if !defined(__REST_HTTP__CONNECTION_MANAGER_H__) -#define __REST_HTTP__CONNECTION_MANAGER_H__ +#if !defined(__REST_HTTP__SERVER_CONNECTION_MANAGER_H__) +#define __REST_HTTP__SERVER_CONNECTION_MANAGER_H__ #include "Defines.h" @@ -56,16 +56,16 @@ namespace network // --------------------------------------------------------------------------- template - class ConnectionManager + class ServerConnectionManager { public: - /// Initializes a new instance of the ConnectionManager class. - ConnectionManager() { /* stub */ } - /// Initializes a copy instance of the ConnectionManager class. - ConnectionManager(const ConnectionManager&) = delete; + /// Initializes a new instance of the ServerConnectionManager class. + ServerConnectionManager() { /* stub */ } + /// Initializes a copy instance of the ServerConnectionManager class. + ServerConnectionManager(const ServerConnectionManager&) = delete; /// - ConnectionManager& operator=(const ConnectionManager&) = delete; + ServerConnectionManager& operator=(const ServerConnectionManager&) = delete; /// Add the specified connection to the manager and start it. void start(ConnectionPtr c) @@ -105,5 +105,5 @@ namespace network } // namespace rest } // namespace network -#endif // __REST_HTTP__CONNECTION_MANAGER_H__ +#endif // __REST_HTTP__SERVER_CONNECTION_MANAGER_H__ \ No newline at end of file diff --git a/src/remote/RESTClient.cpp b/src/remote/RESTClient.cpp index 4e605d0f..eca06434 100644 --- a/src/remote/RESTClient.cpp +++ b/src/remote/RESTClient.cpp @@ -167,9 +167,9 @@ int RESTClient::send(const std::string& address, uint32_t port, const std::strin typedef network::rest::BasicRequestDispatcher RESTDispatcherType; RESTDispatcherType m_dispatcher(RESTClient::responseHandler); + HTTPClient client(address, port); try { - HTTPClient client(address, port); if (!client.open()) return ERRNO_SOCK_OPEN; client.setHandler(m_dispatcher); @@ -187,6 +187,8 @@ int RESTClient::send(const std::string& address, uint32_t port, const std::strin edac::SHA256 sha256; sha256.buffer(in, (uint32_t)(size), out); + delete[] in; + std::stringstream ss; ss << std::hex; @@ -248,6 +250,7 @@ int RESTClient::send(const std::string& address, uint32_t port, const std::strin client.close(); } catch (std::exception&) { + client.close(); return ERRNO_INTERNAL_ERROR; }