From f5055dd810d50feb81d4b4a5553b7199d877f5fc Mon Sep 17 00:00:00 2001 From: Arttu Tarkiainen Date: Wed, 5 Apr 2023 17:31:47 +0300 Subject: Fix issues caused by invoking exit() on non-main thread These could cause unpredictable behavior, resulting in crashes if the daemon would be started twice, trying to bind the TCP server socket to an address with already taken port, etc. Fix by: - Add generic runtime exception class for daemon thrown errors. - Replace exit() calls that could occur on non-main thread with throwing the new exception. - Add test case failing without the fix. Change-Id: I5cfe679bdd9a034b1913edf30be00acfe3f3232c Reviewed-by: Iikka Eklund --- src/libs/qlicenseservice/errors.h | 33 +++++++++++++++ src/libs/qlicenseservice/licenser.cpp | 10 ++++- src/libs/qlicenseservice/licenseservice.cpp | 29 ++++++++------ src/libs/qlicenseservice/tcpserver.cpp | 51 +++++++++++++----------- src/libs/qlicenseservice/tcpserver.h | 4 +- src/windows_daemon/windowsdaemon.cpp | 34 ++++++++-------- tests/auto/licenseservice/tst_licenseservice.cpp | 16 ++++++++ 7 files changed, 121 insertions(+), 56 deletions(-) create mode 100644 src/libs/qlicenseservice/errors.h diff --git a/src/libs/qlicenseservice/errors.h b/src/libs/qlicenseservice/errors.h new file mode 100644 index 0000000..fff21dd --- /dev/null +++ b/src/libs/qlicenseservice/errors.h @@ -0,0 +1,33 @@ +/* Copyright (C) 2023 The Qt Company Ltd. + * + * SPDX-License-Identifier: GPL-3.0-only WITH Qt-GPL-exception-1.0 +*/ + +#pragma once + +#include +#include + +namespace QLicenseService { + +class Error : public std::runtime_error +{ +public: + explicit Error(const char *message) + : std::runtime_error(message) + {} + + explicit Error(const std::string &message) + : std::runtime_error(message) + {} + + virtual ~Error() noexcept + {} + + virtual const char *what() const noexcept override + { + return std::runtime_error::what(); + } +}; + +} // namespace QLicenseService diff --git a/src/libs/qlicenseservice/licenser.cpp b/src/libs/qlicenseservice/licenser.cpp index 5d7c814..9094bd7 100644 --- a/src/libs/qlicenseservice/licenser.cpp +++ b/src/libs/qlicenseservice/licenser.cpp @@ -11,6 +11,7 @@ #include "clitoolhandler.h" #include "version.h" #include "utils.h" +#include "errors.h" namespace QLicenseService { @@ -36,6 +37,10 @@ Licenser::Licenser(uint16_t tcpPort, const std::string &settingsPath) m_settings->get("version_query_access_point")); // Start the TCP/IP server m_tcpServer = new TcpServer(tcpPort); + if (!m_tcpServer->init()) { + throw Error("Error while initializing TCP server in port: " + + std::to_string(tcpPort)); + } } Licenser::~Licenser() @@ -50,7 +55,10 @@ int Licenser::listen() { m_infoString = ""; uint16_t socketId = 0; // Placeholder for whatever socket gets active - std::string input = m_tcpServer->listenToClients(socketId); + std::string input; + if (!m_tcpServer->listenToClients(socketId, &input)) + throw QLicenseService::Error("Error while listening to clients!"); + input = utils::trimStr(input); if (input.empty()) { diff --git a/src/libs/qlicenseservice/licenseservice.cpp b/src/libs/qlicenseservice/licenseservice.cpp index 6e1d158..a9c83c3 100644 --- a/src/libs/qlicenseservice/licenseservice.cpp +++ b/src/libs/qlicenseservice/licenseservice.cpp @@ -6,6 +6,7 @@ #include "licenseservice.h" #include "licenser.h" +#include "errors.h" #if __linux__ || __APPLE__ || __MACH__ #include @@ -31,10 +32,9 @@ void LicenseService::run() sigfillset(&signal_set); pthread_sigmask(SIG_BLOCK, &signal_set, NULL); #endif - Licenser licenser(m_tcpPort, m_settingsPath); - - while (1) { - try { + try { + Licenser licenser(m_tcpPort, m_settingsPath); + while (1) { licenser.listen(); if (isCancelRequested()) { updateAndNotifyStatus(Status::Canceled); @@ -42,16 +42,21 @@ void LicenseService::run() } else if (status() != Status::Running) { updateAndNotifyStatus(Status::Running); } - } catch (const std::exception &e) { - setError("Caught exception: " + std::string(e.what())); - updateAndNotifyStatus(Status::Error); - return; - } catch (...) { - setError("Caught unknown exception"); - updateAndNotifyStatus(Status::Error); - return; } + } catch (const QLicenseService::Error &e) { + setError("Caught exception: " + std::string(e.what())); + updateAndNotifyStatus(Status::Error); + return; + } catch (const std::exception &e) { + setError("Caught exception: " + std::string(e.what())); + updateAndNotifyStatus(Status::Error); + return; + } catch (...) { + setError("Caught unknown exception"); + updateAndNotifyStatus(Status::Error); + return; } + // Never reached updateAndNotifyStatus(Status::Finished); } diff --git a/src/libs/qlicenseservice/tcpserver.cpp b/src/libs/qlicenseservice/tcpserver.cpp index 330ad07..565f43a 100644 --- a/src/libs/qlicenseservice/tcpserver.cpp +++ b/src/libs/qlicenseservice/tcpserver.cpp @@ -8,6 +8,20 @@ namespace QLicenseService { TcpServer::TcpServer(uint16_t serverPort) + : m_serverPort(serverPort) +{ +} + +TcpServer::~TcpServer() +{ + doCloseSocket(m_masterSocket); + for (type_socket sd: m_clientSocket) { + doCloseSocket(sd); + } + m_clientSocket.clear(); +} + +bool TcpServer::init() { int opt = TRUE; @@ -18,19 +32,19 @@ TcpServer::TcpServer(uint16_t serverPort) iResult = WSAStartup(MAKEWORD(2, 2), &wsaData); if (iResult != NO_ERROR) { wprintf(L"Error at WSAStartup()\n"); - exit(EXIT_FAILURE); + return false; } #endif // Create a master socket if ((m_masterSocket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) == 0) { perror("socket failed"); - exit(EXIT_FAILURE); + return false; } // Set master socket to allow multiple connections if (setsockopt(m_masterSocket, SOL_SOCKET, SO_REUSEADDR, (char *)&opt, sizeof(opt)) < 0) { perror("setsockopt failed"); - exit(EXIT_FAILURE); + return false; } timeval tv {1, 0}; //t.tv_sec = 2; t.tv_usec = 0; int ret = setsockopt(m_masterSocket, SOL_SOCKET, SO_RCVTIMEO, @@ -43,42 +57,31 @@ TcpServer::TcpServer(uint16_t serverPort) // Type of socket created m_address.sin_family = AF_INET; m_address.sin_addr.s_addr = INADDR_ANY; - m_address.sin_port = htons(serverPort); + m_address.sin_port = htons(m_serverPort); // Bind the socket to localhost if (bind(m_masterSocket, (struct sockaddr *)&m_address, sizeof(m_address)) < 0) { doCloseSocket(m_masterSocket); perror("bind failed"); - exit(EXIT_FAILURE); + return false; } - std::cout << "Listening to port " << serverPort << std::endl; + std::cout << "Listening to port " << m_serverPort << std::endl; //ioctlsocket(m_masterSocket, FIONBIO, &mode); // Try to specify maximum of 3 pending connections for the master socket if (listen(m_masterSocket, 3) < 0) { doCloseSocket(m_masterSocket); perror("listen"); - exit(EXIT_FAILURE); + return false; } m_addrlen = sizeof(m_address); // Accept the incoming connection std::cout << "Waiting for connections ...\n"; + return true; } -TcpServer::~TcpServer() +bool TcpServer::listenToClients(uint16_t &socketId, std::string *data) { - doCloseSocket(m_masterSocket); - for (type_socket sd: m_clientSocket) { - doCloseSocket(sd); - } - m_clientSocket.clear(); -} - - -std::string TcpServer::listenToClients(uint16_t &socketId) -{ - std::string data; - // Clear the socket set FD_ZERO(&m_readfds); @@ -111,7 +114,7 @@ std::string TcpServer::listenToClients(uint16_t &socketId) if ((new_socket = accept(m_masterSocket, (struct sockaddr *)&m_address, (socklen_t *)&m_addrlen)) < 0) { perror("accept"); - exit(EXIT_FAILURE); + return false; } // Add new socket to set of sockets m_clientSocket.insert(new_socket); @@ -133,17 +136,17 @@ std::string TcpServer::listenToClients(uint16_t &socketId) getpeername(sd, (struct sockaddr *)&m_address, (socklen_t *)&m_addrlen); socketId = sd; - data = SOCKET_CLOSED_MSG; + *data = SOCKET_CLOSED_MSG; } else { // Set the string terminating NULL byte // on the end of the data read m_buffer[valread] = '\0'; socketId = sd; - data = m_buffer; + *data = m_buffer; } } } - return data; + return true; } int TcpServer::sendResponse(int socketId, const std::string &message) diff --git a/src/libs/qlicenseservice/tcpserver.h b/src/libs/qlicenseservice/tcpserver.h index 22c3014..ce4ce23 100644 --- a/src/libs/qlicenseservice/tcpserver.h +++ b/src/libs/qlicenseservice/tcpserver.h @@ -48,11 +48,13 @@ public: explicit TcpServer(uint16_t serverPort); ~TcpServer(); - std::string listenToClients(uint16_t &socketId); + bool init(); + bool listenToClients(uint16_t &socketId, std::string *data); int sendResponse(int socketIndex, const std::string &message); void closeClientSocket(int socketIndex); private: + uint16_t m_serverPort; type_socket m_masterSocket; std::unordered_set m_clientSocket; int m_addrlen; diff --git a/src/windows_daemon/windowsdaemon.cpp b/src/windows_daemon/windowsdaemon.cpp index d073685..10ab6f8 100644 --- a/src/windows_daemon/windowsdaemon.cpp +++ b/src/windows_daemon/windowsdaemon.cpp @@ -42,16 +42,15 @@ int __cdecl _tmain(int argc, TCHAR *argv[]) if (lstrcmpi(argv[1], TEXT("--nodaemon")) == 0) { // Fire it up printf("Starting qtlicd as CLI tool\n"); - Licenser licenser(0); - while (true) - { - try { + + try { + Licenser licenser(0); + while (true) licenser.listen(); - } - catch (...) { - printf("ERROR: Qt License Daemon listen() failed!!\n"); - return -1; - } + } + catch (...) { + printf("ERROR: Qt License Daemon listen() failed!!\n"); + return -1; } } // If command-line parameter is "--version", show the version and exit. @@ -255,18 +254,17 @@ VOID SvcInit( DWORD dwArgc, LPTSTR *lpszArgv) // Fire it up printf("Starting daemon\n"); - Licenser licenser(0); - // Perform work until service stops. - while (WaitForSingleObject(ghSvcStopEvent, 0) != WAIT_OBJECT_0) - { - try { + try { + Licenser licenser(0); + // Perform work until service stops. + while (WaitForSingleObject(ghSvcStopEvent, 0) != WAIT_OBJECT_0) licenser.listen(); - } - catch (...) { - printf("ERROR: Qt License Daemon listen() failed!!\n"); - } } + catch (...) { + printf("ERROR: Qt License Daemon listen() failed!!\n"); + } + ReportSvcStatus( SERVICE_STOP_PENDING, NO_ERROR, 0 ); CloseHandle(ghSvcStopEvent); ReportSvcStatus( SERVICE_STOPPED, NO_ERROR, 0 ); diff --git a/tests/auto/licenseservice/tst_licenseservice.cpp b/tests/auto/licenseservice/tst_licenseservice.cpp index 80ec27b..2cda6a2 100644 --- a/tests/auto/licenseservice/tst_licenseservice.cpp +++ b/tests/auto/licenseservice/tst_licenseservice.cpp @@ -20,3 +20,19 @@ TEST_CASE("Start/stop without clients", "[licenseservice]") REQUIRE((service.cancel() && service.waitForFinished())); REQUIRE(service.status() == LicenseService::Status::Canceled); } + +TEST_CASE("Start two daemon instances with same port", "[licenseservice]") +{ + LicenseService service(0, DAEMON_SETTINGS_FILE_DEFAULT); + // 1. Verify starting of license "service" succeeds + REQUIRE((service.start() && service.waitForStarted())); + + LicenseService service2(0, DAEMON_SETTINGS_FILE_DEFAULT); + // 2. Verify starting another instance fails gracefully + REQUIRE((service2.start() && !service2.waitForStarted())); + REQUIRE(service2.status() == LicenseService::Status::Error); + + // 3. Verify stop original service + REQUIRE((service.cancel() && service.waitForFinished())); + REQUIRE(service.status() == LicenseService::Status::Canceled); +} -- cgit v1.2.3