From a4a7b4ad6c8b23902efb6ee6f2057ee9c6f9da86 Mon Sep 17 00:00:00 2001 From: Tomasz Swierczek Date: Wed, 16 Dec 2020 12:11:27 +0100 Subject: [PATCH 01/16] Release 0.1.39 * Replace sqlcipher with upstream 4.4.2 * Unit tests improvements * Small fixes Change-Id: I94a213c7b122c0867915c38c14ebb25db1258420 --- packaging/key-manager.spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packaging/key-manager.spec b/packaging/key-manager.spec index a8b82ac..7fd375c 100644 --- a/packaging/key-manager.spec +++ b/packaging/key-manager.spec @@ -11,7 +11,7 @@ Name: key-manager Summary: Central Key Manager and utilities -Version: 0.1.38 +Version: 0.1.39 Release: 1 Group: Security/Secure Storage License: Apache-2.0 and BSD-3-Clause -- 2.7.4 From df6fadcf5aee54dca55cb170b02cc6874b955675 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Mon, 4 Jan 2021 10:01:30 +0100 Subject: [PATCH 02/16] Make IEncryptionService destructor protected The implicitly-defined destructor is non-virtual and public. We don't want the EncryptionService to be destroyed via IEncryptionService. Change-Id: Iaf2b180cdd4f60a4f20cc1c9e1d593dcd1c1f220 --- src/manager/service/iencryption-service.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/manager/service/iencryption-service.h b/src/manager/service/iencryption-service.h index 35281e2..eb5a8ae 100644 --- a/src/manager/service/iencryption-service.h +++ b/src/manager/service/iencryption-service.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000 - 2015 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2015 - 2021 Samsung Electronics Co., Ltd All Rights Reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -35,6 +35,8 @@ public: int retCode, const RawBuffer &data = RawBuffer()) = 0; virtual void RequestKey(const CryptoRequest &request) = 0; +protected: + ~IEncryptionService() {} }; } // namespace CKM -- 2.7.4 From f8f77aa1a76f0c308ece4a88da6af7125508f466 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Mon, 4 Jan 2021 14:56:28 +0100 Subject: [PATCH 03/16] Use memcpy to avoid unaligned access Casting unsigned char* to signalfd_siginfo* may cause an unaligned access (see -Wcast-align). Use memcpy to avoid it. Verify by sending SIGTERM to key-manager, observing the logs and systemctl status. The service should stop without errors. systemctl start central-key-manager kill -SIGTERM `pidof key-manager` systemctl status central-key-manager Change-Id: I061cc2f488cba9252ed65b0d8ca22840f725a433 --- src/manager/main/socket-manager.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 55150fe..047b6c5 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2014 - 2020 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2014 - 2021 Samsung Electronics Co., Ltd All Rights Reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,6 +32,7 @@ #include #include #include +#include #include @@ -129,23 +130,23 @@ struct SignalService : public GenericSocketService { { LogDebug("Get signal information"); - if (sizeof(struct signalfd_siginfo) != event.rawBuffer.size()) { + signalfd_siginfo siginfo; + if (sizeof(siginfo) != event.rawBuffer.size()) { LogError("Wrong size of signalfd_siginfo struct. Expected: " - << sizeof(signalfd_siginfo) << " Get: " + << sizeof(siginfo) << " Get: " << event.rawBuffer.size()); return; } - auto siginfo = reinterpret_cast - (event.rawBuffer.data()); + memcpy(&siginfo, event.rawBuffer.data(), sizeof(siginfo)); - if (siginfo->ssi_signo == SIGTERM) { + if (siginfo.ssi_signo == SIGTERM) { LogInfo("Got signal: SIGTERM"); m_serviceManager->MainLoopStop(); return; } - LogInfo("This should not happend. Got signal: " << siginfo->ssi_signo); + LogInfo("This should not happen. Got unexpected signal: " << siginfo.ssi_signo); } }; -- 2.7.4 From b24b99857d61e92f680768291d7c0836461d97db Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Fri, 8 Jan 2021 14:52:19 +0100 Subject: [PATCH 04/16] packaging: rpm scriptlet cleanup, handle -p /sbin/ldconfig The RPM documention indicates that during an rpm install or erase, the script(lets): %post, %preun, and %postun (and %pre, %build, %install, etc.) are copied to a temp file, and then the temp file is run as a (/bin/sh or bash) script. Unfortunately the documentation is not clear about how rpmbuild and/or rpm determine where the end of any scriptlet is when it is copied to the file. Most things in the key-manager.spec work correctly as is. These are the %preun, %post, and %postun scriptlets that are "closed" by a following %preun, %post, and %postun, or potentially another scriptlet, e.g. %file. The ones that don't work correctly (only one actually) are those where there is a comment in the spec file before it is closed by another scriptlet. Further complicating things is that the type of scriptlet affects what rpm does and what `rpm -qp --scripts ...` shows. The specific one that didn't work was the "postun -n libkey-manager-client -p /sbin/ldconfig" scriptlet. It is followed by a comment before being "closed" by the %files section (or scriptlet). It can be written two ways: "%postun -n libkey-manager-client\n/sbin/ldconfig" or "%postun -n libkey-manager-client -p /sbin/ldconfig". Either way it's written, `rpm -qp --scripts libkey-manager-client...` will include the comment lines between the %postun line and the following %files line. But the way rpm executes these depends on how they're written. If written as "%postun -n libkey-manager-client\n/sbin/ldconfig" rpm will simply run /sbin/ldconfig with no command line options, i.e. execve ("/sbin/ldconfig", [ "/sbin/ldconfig" ], [ ]); But when written as "%postun -n libkey-manager-client -p /sbin/ldconfig", it will copy the comment lines to a temp file, and pass the temp file name and "1" as (command line) parameters, i.e. execve ("/sbin/ldconfig", [ "/sbin/ldconfig", "/tmp/tmpXXXXXX", "1" ], [ ]); Which results in ldconfig exiting with an error. (Remember, both ways show the comment in `rpm -qp --scripts ...`) Problematic comment line was removed and whole file comments style was adjusted. Additionally some cleanup was performed. Change-Id: I966f0930d7a7b46b401f399aaf2e5c748edc0a1f --- packaging/key-manager.spec | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/packaging/key-manager.spec b/packaging/key-manager.spec index 7fd375c..7102e50 100644 --- a/packaging/key-manager.spec +++ b/packaging/key-manager.spec @@ -67,9 +67,6 @@ BuildRequires: lcov %global misc_dir %{?TZ_SYS_DATA:%TZ_SYS_DATA/ckm-misc}%{!?TZ_SYS_DATA:%/opt/data/ckm-misc} %global coverage_dir %{?TZ_SYS_DATA:%TZ_SYS_DATA/ckm-coverage}%{!?TZ_SYS_DATA:%/opt/data/ckm-coverage} %global bin_dir %{?TZ_SYS_BIN:%TZ_SYS_BIN}%{!?TZ_SYS_BIN:%_bindir} -# image creation error occured if /usr/sbin used for ldconfig -#%global sbin_dir %{?TZ_SYS_SBIN:%TZ_SYS_SBIN}%{!?TZ_SYS_SBIN:%_sbindir} -%global sbin_dir /sbin %global ro_etc_dir %{?TZ_SYS_RO_ETC:%TZ_SYS_RO_ETC}%{!?TZ_SYS_RO_ETC:/etc} %global run_dir %{?TZ_SYS_RUN:%TZ_SYS_RUN}%{!?TZ_SYS_RUN:/var/run} %global initial_values_dir_ro %{ro_data_dir}/initial_values @@ -86,8 +83,8 @@ application to sign and verify (DSA/RSA/ECDSA) signatures. Summary: Central Key Manager (common libraries) Group: Security/Libraries License: Apache-2.0 -Requires(post): %{sbin_dir}/ldconfig -Requires(postun): %{sbin_dir}/ldconfig +Requires(post): /sbin/ldconfig +Requires(postun): /sbin/ldconfig %description -n libkey-manager-common Central Key Manager package (common library) @@ -98,8 +95,8 @@ Group: Security/Libraries License: Apache-2.0 Requires: key-manager = %{version}-%{release} Requires: libkey-manager-common = %{version}-%{release} -Requires(post): %{sbin_dir}/ldconfig -Requires(postun): %{sbin_dir}/ldconfig +Requires(post): /sbin/ldconfig +Requires(postun): /sbin/ldconfig %description -n libkey-manager-client Central Key Manager package (client) @@ -141,8 +138,8 @@ Group: Security/Libraries License: Apache-2.0 BuildRequires: pam-devel Requires: key-manager = %{version}-%{release} -Requires(post): %{sbin_dir}/ldconfig -Requires(postun): %{sbin_dir}/ldconfig +Requires(post): /sbin/ldconfig +Requires(postun): /sbin/ldconfig %description -n key-manager-pam-plugin CKM login/password module to PAM. Used to monitor user login/logout @@ -155,8 +152,8 @@ License: Apache-2.0 BuildRequires: cmake BuildRequires: pkgconfig(openssl1.1) BuildRequires: pkgconfig(libxml-2.0) -Requires(post): %{sbin_dir}/ldconfig -Requires(postun): %{sbin_dir}/ldconfig +Requires(post): /sbin/ldconfig +Requires(postun): /sbin/ldconfig %description -n key-manager-initial-values Includes ckm_initial_values tool for initial values XML generation @@ -231,9 +228,7 @@ make %{?jobs:-j%jobs} %install %make_install -############################################################################### %if ! %{coverage_only} -############################################################################### %install_service multi-user.target.wants central-key-manager.service %install_service sockets.target.wants central-key-manager-api-control.socket @@ -242,9 +237,7 @@ make %{?jobs:-j%jobs} %install_service sockets.target.wants central-key-manager-api-encryption.socket cp -a %{SOURCE1001} %{SOURCE1002} %{SOURCE1003} %{SOURCE1004} %{buildroot}%{_datadir}/ -#################### ! %{coverage_only} ####################################### %endif -############################################################################### %pre # tzplatform-get sync breaked because of on-development situation. comment out just for temporary @@ -271,7 +264,7 @@ cp -a %{SOURCE1001} %{SOURCE1002} %{SOURCE1003} %{SOURCE1004} %{buildroot}%{_dat # #id -u %{user_name} > /dev/null 2>&1 #if [ $? -eq 1 ]; then -# useradd -d /var/lib/empty -s %{sbin_dir}/nologin -r -g %{group_name} %{user_name} > /dev/null 2>&1 +# useradd -d /var/lib/empty -s /sbin/nologin -r -g %{group_name} %{user_name} > /dev/null 2>&1 #fi %post @@ -311,14 +304,12 @@ if [ $1 = 0 ]; then systemctl daemon-reload fi -%post -n libkey-manager-common -p %{sbin_dir}/ldconfig -%post -n libkey-manager-client -p %{sbin_dir}/ldconfig -%postun -n libkey-manager-common -p %{sbin_dir}/ldconfig -%postun -n libkey-manager-client -p %{sbin_dir}/ldconfig +%post -n libkey-manager-common -p /sbin/ldconfig +%post -n libkey-manager-client -p /sbin/ldconfig +%postun -n libkey-manager-common -p /sbin/ldconfig +%postun -n libkey-manager-client -p /sbin/ldconfig -############################################################################### %if ! %{coverage_only} -############################################################################### %files -n key-manager %manifest key-manager.manifest @@ -401,9 +392,7 @@ fi %{bin_dir}/ckm_db_perf %misc_dir -#################### ! %{coverage_only} ####################################### %endif -############################################################################### %files -n key-manager-unit-tests %manifest key-manager-unit-tests.manifest @@ -412,15 +401,11 @@ fi %{bin_dir}/ckm-unit-tests %unit_tests_dir -############################################################################### %if "%{build_type}" == "COVERAGE" -############################################################################### %files -n key-manager-coverage %license LICENSE %{bin_dir}/key-manager-coverage.sh %coverage_dir -#################### %{build_type} == COVERAGE ################################ %endif -############################################################################### -- 2.7.4 From eaa02d5358657430b755d7ec6908719d4d74e59f Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Fri, 8 Jan 2021 17:31:00 +0100 Subject: [PATCH 05/16] Release 0.1.40 * packaging: rpm scriptlet cleanup, handle -p /sbin/ldconfig * Use memcpy to avoid unaligned access * Make IEncryptionService destructor protected Change-Id: Id6c04467097f0a89c58403c5e824d8b2d0a35aea --- packaging/key-manager.spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packaging/key-manager.spec b/packaging/key-manager.spec index 7102e50..6562f65 100644 --- a/packaging/key-manager.spec +++ b/packaging/key-manager.spec @@ -11,7 +11,7 @@ Name: key-manager Summary: Central Key Manager and utilities -Version: 0.1.39 +Version: 0.1.40 Release: 1 Group: Security/Secure Storage License: Apache-2.0 and BSD-3-Clause -- 2.7.4 From b0387c748ad01547b2ac7017832320379d6c8268 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Tue, 12 Jan 2021 12:33:53 +0100 Subject: [PATCH 06/16] Start SocketManager as not working The m_working flag should be set to true only inside MainLoop(). Change-Id: I47138d2036ff87712b4b5ac4b4df385917cd866b --- src/manager/main/socket-manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 047b6c5..543ba35 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -181,7 +181,7 @@ SocketManager::CreateDefaultReadSocketDescription(int sock, bool timeout) SocketManager::SocketManager() : m_maxDesc(0), - m_working(true), + m_working(false), m_counter(0) { FD_ZERO(&m_readSet); -- 2.7.4 From a4a287706e4b1b32eb86d87c61f2a698e67d10d1 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Tue, 12 Jan 2021 12:30:01 +0100 Subject: [PATCH 07/16] Add check for connection counter in the server This is just a precaution targeted more at stress tests rather than regular key-manager usage. Also remove unused ConnectionID operator. Change-Id: I090b7bd29594d8a47cc4142a7713ccfb4c9b121e --- src/manager/main/generic-socket-manager.h | 6 +----- src/manager/main/socket-manager.cpp | 6 ++++++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/manager/main/generic-socket-manager.h b/src/manager/main/generic-socket-manager.h index 243e192..6532b4b 100644 --- a/src/manager/main/generic-socket-manager.h +++ b/src/manager/main/generic-socket-manager.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000-2019 Samsung Electronics Co., Ltd. All rights reserved + * Copyright (c) 2014-2021 Samsung Electronics Co., Ltd. All rights reserved * * Contact: Dongsun Lee * @@ -49,10 +49,6 @@ typedef int InterfaceID; struct ConnectionID { int sock; // This is decriptor used for connection int counter; // Unique handler per socket - inline bool operator<(const ConnectionID &second) const - { - return counter < second.counter; - } }; struct GenericSocketManager; diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 543ba35..b05b220 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include @@ -162,6 +163,11 @@ SocketManager::CreateDefaultReadSocketDescription(int sock, bool timeout) desc.setCynara(false); desc.interfaceID = 0; desc.service = NULL; + + if (m_counter == std::numeric_limits::max()) { + LogError("Connection counter reached the maximum value. Aborting."); + throw std::overflow_error("Connection counter reached the maximum value. Aborting."); + } desc.counter = ++m_counter; if (timeout) { -- 2.7.4 From 458c2cbe8bff78708bcc27ece4c79b7714037cd3 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Tue, 12 Jan 2021 12:37:18 +0100 Subject: [PATCH 08/16] Add timeout queue stress test While one socket connection is active and its timeout not reached yet, we can open and close many new connections, which will push items on SocketManager::m_timeoutQueue. Because first socket has an earliest timeout, the second connection elements won't be touched. Because m_timeoutQueue elements are not removed on socket close, the queue will grow. Still nothing bad should happen. Change-Id: Ied20d2e1517ad471e465c6fa601e368469a4cc37 --- src/manager/main/socket-manager.cpp | 4 + unit-tests/CMakeLists.txt | 9 +++ unit-tests/test_socket-manager.cpp | 141 ++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100644 unit-tests/test_socket-manager.cpp diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index b05b220..af4cc2e 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -48,7 +48,11 @@ namespace { +#ifdef OVERRIDE_SOCKET_TIMEOUT +const time_t SOCKET_TIMEOUT = OVERRIDE_SOCKET_TIMEOUT; +#else // OVERRIDE_SOCKET_TIMEOUT const time_t SOCKET_TIMEOUT = 1000; +#endif // OVERRIDE_SOCKET_TIMEOUT int getCredentialsFromSocket(int sock, CKM::Credentials &cred) { diff --git a/unit-tests/CMakeLists.txt b/unit-tests/CMakeLists.txt index 149a1f2..30eaac0 100644 --- a/unit-tests/CMakeLists.txt +++ b/unit-tests/CMakeLists.txt @@ -31,7 +31,9 @@ SET(PKCS12_TEST_DIR ${UNIT_TESTS_DIR}/pkcs12/) ADD_DEFINITIONS("-DDB_TEST_DIR=\"${DB_TEST_DIR}\"") ADD_DEFINITIONS("-DSS_TEST_DIR=\"${SS_TEST_DIR}\"") ADD_DEFINITIONS("-DPKCS12_TEST_DIR=\"${PKCS12_TEST_DIR}\"") + ADD_DEFINITIONS("-DBOOST_TEST_DYN_LINK") +ADD_DEFINITIONS("-DOVERRIDE_SOCKET_TIMEOUT=10") SET(MANAGER_PATH ${PROJECT_SOURCE_DIR}/src/manager) @@ -82,13 +84,16 @@ SET(UNIT_TESTS_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/test_pkcs12.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_safe-buffer.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_serialization.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test_socket-manager.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_sql.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_stringify.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_ss-crypto.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_sw-backend.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_xml-parser.cpp + ${MANAGER_PATH}/client/client-common.cpp ${MANAGER_PATH}/client-async/descriptor-set.cpp + ${MANAGER_PATH}/client-capi/ckmc-type-converter.cpp ${MANAGER_PATH}/common/algo-param.cpp ${MANAGER_PATH}/common/certificate-impl.cpp ${MANAGER_PATH}/common/ckm-zero-memory.cpp @@ -118,6 +123,10 @@ SET(UNIT_TESTS_SOURCES ${MANAGER_PATH}/dpl/log/src/old_style_log_provider.cpp ${MANAGER_PATH}/initial-values/parser.cpp ${MANAGER_PATH}/initial-values/xml-utils.cpp + ${MANAGER_PATH}/main/cynara.cpp + ${MANAGER_PATH}/main/smack-check.cpp + ${MANAGER_PATH}/main/socket-2-id.cpp + ${MANAGER_PATH}/main/socket-manager.cpp ${MANAGER_PATH}/service/crypto-logic.cpp ${MANAGER_PATH}/service/db-crypto.cpp ${MANAGER_PATH}/service/for-each-file.cpp diff --git a/unit-tests/test_socket-manager.cpp b/unit-tests/test_socket-manager.cpp new file mode 100644 index 0000000..e1a3af5 --- /dev/null +++ b/unit-tests/test_socket-manager.cpp @@ -0,0 +1,141 @@ +/* + * Copyright (c) 2021 Samsung Electronics Co., Ltd All Rights Reserved + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License + */ + +#include + +#include +#include +#include +#include + +#include +#include + +#include +#include + +using namespace CKM; +using namespace std::chrono_literals; + +namespace { + +constexpr char SERVICE_SOCKET_TEST[] = "/tmp/.central-key-manager-test.sock"; +constexpr CKM::InterfaceID SOCKET_ID_TEST = 42; + +class TestService : public GenericSocketService { +public: + ServiceDescriptionVector GetServiceDescription() override { + return ServiceDescriptionVector { + {SERVICE_SOCKET_TEST, "", SOCKET_ID_TEST} + }; + } + void Event(const AcceptEvent &) override { Connected(true); } + void Event(const WriteEvent &) override {} + void Event(const ReadEvent &) override {} + void Event(const CloseEvent &) override { Connected(false); } + void Event(const SecurityEvent &) override {} + + void Start() override {} + void Stop() override {} + + void WaitUntilConnected(bool isConnected) { + std::unique_lock lock(m_mutex); + BOOST_REQUIRE(m_cv.wait_for(lock, 10s, [&]{ return m_connected == isConnected; })); + } + + void Connected(bool isConnected) { + std::unique_lock lock(m_mutex); + m_connected = isConnected; + lock.unlock(); + m_cv.notify_one(); + } + +private: + bool m_connected = false; + std::mutex m_mutex; + std::condition_variable m_cv; +}; + +struct TestSocketManager : public SocketManager { + size_t TimeoutQueueSize() const { return m_timeoutQueue.size(); } +}; + +} // namespace + +BOOST_AUTO_TEST_SUITE(SOCKET_MANAGER_TEST) + +POSITIVE_TEST_CASE(StressTestGrowingTimeoutQueue) +{ + constexpr unsigned REPEATS = 100000; + constexpr auto INTERVAL = REPEATS/10; + + int ret = unlink(SERVICE_SOCKET_TEST); + int err = errno; + BOOST_REQUIRE(ret == 0 || (ret == -1 && err == ENOENT)); + + TestSocketManager manager; + auto service = new TestService(); + manager.RegisterSocketService(service); + + bool exception = false; + std::string what("Unknown exception"); + + std::thread th([&]{ + try { + manager.MainLoop(); + } catch (const std::exception& e) { + exception = true; + what = e.what(); + } catch (...) { + exception = true; + } + }); + + { + SockRAII socket; + ret = socket.connect(SERVICE_SOCKET_TEST); + BOOST_REQUIRE(ret == CKM_API_SUCCESS); + service->WaitUntilConnected(true); + BOOST_REQUIRE(manager.TimeoutQueueSize() == 1); + service->Connected(false); + + // check if the closed socket timeouts are removed from the queue. + SockRAII socket2; + for(unsigned i=0;iWaitUntilConnected(true); + + socket2.disconnect(); + service->WaitUntilConnected(false); + + if ((i + 1) % INTERVAL == 0) + BOOST_TEST_MESSAGE("Creating connections: " << i + 1 << "/" << REPEATS); + } + BOOST_TEST_MESSAGE("Waiting " << OVERRIDE_SOCKET_TIMEOUT + 2 << "s for socket timeouts."); + + // all sockets should be closed after timeout and the queue should be empty + std::this_thread::sleep_for(std::chrono::seconds(OVERRIDE_SOCKET_TIMEOUT + 2)); + BOOST_REQUIRE(manager.TimeoutQueueSize() == 0); + } + + manager.MainLoopStop(); + th.join(); + + BOOST_REQUIRE_MESSAGE(!exception, what); +} + +BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From f3251fc099fc909c40f4d83bd763243c6f6c0498 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Wed, 13 Jan 2021 10:40:17 +0100 Subject: [PATCH 09/16] Refactor SocketManager's timeout queue SocketManager changes: * Remove timeout queue elements on socket closure. Until now it was possible to make the timeout queue grow because its elements were not removed upon socket closure. * The queue now contains only socket numbers of corresponding m_socketDescriptionVector elements. The code responsible for timeout updates in the queue is no longer neccessary and has been removed. * Modify the timeout queue only if corresponding socket has a timeout enabled. * Remove unnecessary 'open' and 'timeout' socket flag check if a timeout occurs. Only the main thread modifies these flags. If there's a timeout, it must have been triggered by an opened socket with timeout enabled. Growing queue test changes: * Compare timeout queue size and connection count in the SocketManager thread. * Assume that first AcceptEvent is triggered by the most reccent connection attempt. * Match client and server sockets to properly detect CloseEvents. * Add more stress to the test with more initial connections. * Throw std exceptions from SocketManager thread. * Wrap SocketManager thread in an object. Check a possible exception in the destructor. * Get rid of unnecessary timeouts. Change-Id: Icd63696a58c4ef6a66c2e487819423df610ca580 --- src/manager/main/socket-manager.cpp | 79 +++++--------- src/manager/main/socket-manager.h | 30 ++++-- unit-tests/test_socket-manager.cpp | 206 ++++++++++++++++++++++++++++-------- 3 files changed, 205 insertions(+), 110 deletions(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index af4cc2e..440e902 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include @@ -176,10 +177,7 @@ SocketManager::CreateDefaultReadSocketDescription(int sock, bool timeout) if (timeout) { desc.timeout = monotonicNow() + SOCKET_TIMEOUT; - Timeout tm; - tm.time = desc.timeout; - tm.sock = sock; - m_timeoutQueue.push(tm); + m_timeoutQueue.push_back(sock); } desc.setTimeout(timeout); @@ -344,7 +342,13 @@ void SocketManager::ReadyForRead(int sock) event.rawBuffer.resize(4096); auto &desc = m_socketDescriptionVector[sock]; - desc.timeout = monotonicNow() + SOCKET_TIMEOUT; + + if (desc.isTimeout()) { + desc.timeout = monotonicNow() + SOCKET_TIMEOUT; + + // move it to the end + m_timeoutQueue.move_back(sock); + } ssize_t size = read(sock, &event.rawBuffer[0], 4096); @@ -400,7 +404,12 @@ void SocketManager::ReadyForWrite(int sock) desc.rawBuffer.erase(desc.rawBuffer.begin(), desc.rawBuffer.begin() + result); - desc.timeout = monotonicNow() + SOCKET_TIMEOUT; + if (desc.isTimeout()) { + desc.timeout = monotonicNow() + SOCKET_TIMEOUT; + + // move it to the end + m_timeoutQueue.move_back(sock); + } if (desc.rawBuffer.empty()) FD_CLR(sock, &m_writeSet); @@ -431,36 +440,16 @@ void SocketManager::MainLoop() timeval localTempTimeout; timeval *ptrTimeout = &localTempTimeout; - // I need to extract timeout from priority_queue. - // Timeout in priority_queue may be deprecated. - // I need to find some actual one. - while (!m_timeoutQueue.empty()) { - auto &top = m_timeoutQueue.top(); - auto &desc = m_socketDescriptionVector[top.sock]; - - if (top.time == desc.timeout) { - // This timeout matches timeout from socket. - // It can be used. - break; - } else { - // This socket was used after timeout in priority queue was set up. - // We need to update timeout and find some useable one. - Timeout tm = { desc.timeout , top.sock}; - m_timeoutQueue.pop(); - m_timeoutQueue.push(tm); - } - } - if (m_timeoutQueue.empty()) { - LogDebug("No usaable timeout found."); + LogDebug("No usable timeout found."); ptrTimeout = NULL; // select will wait without timeout } else { time_t currentTime = monotonicNow(); - auto &pqTimeout = m_timeoutQueue.top(); + auto &pqTimeout = m_socketDescriptionVector[m_timeoutQueue.front()].timeout; // 0 means that select won't block and socket will be closed ;-) ptrTimeout->tv_sec = - currentTime < pqTimeout.time ? pqTimeout.time - currentTime : 0; + currentTime < pqTimeout ? pqTimeout - currentTime : 0; ptrTimeout->tv_usec = 0; } @@ -469,30 +458,12 @@ void SocketManager::MainLoop() if (0 == ret) { // timeout assert(!m_timeoutQueue.empty()); - Timeout pqTimeout = m_timeoutQueue.top(); - m_timeoutQueue.pop(); - - auto &desc = m_socketDescriptionVector[pqTimeout.sock]; - - if (!desc.isTimeout() || !desc.isOpen()) { - // Connection was closed. Timeout is useless... - desc.setTimeout(false); - continue; - } + int sock = m_timeoutQueue.front(); + assert(m_socketDescriptionVector[sock].isTimeout()); + assert(m_socketDescriptionVector[sock].isOpen()); - if (pqTimeout.time < desc.timeout) { - // Is it possible? - // This socket was used after timeout. We need to update timeout. - pqTimeout.time = desc.timeout; - m_timeoutQueue.push(pqTimeout); - continue; - } - - // timeout from m_timeoutQueue matches with socket.timeout - // and connection is open. Time to close it! - // Putting new timeout in queue here is pointless. - desc.setTimeout(false); - CloseSocket(pqTimeout.sock); + // Timeout is reached. Close the connection. + CloseSocket(sock); // All done. Now we should process next select ;-) continue; @@ -815,6 +786,10 @@ void SocketManager::CloseSocket(int sock) desc.interfaceID = -1; desc.rawBuffer.clear(); + // erase corresponding m_timeoutQueue entry if any + if (desc.isTimeout()) + m_timeoutQueue.erase_value(sock); + if (service) service->Event(event); else diff --git a/src/manager/main/socket-manager.h b/src/manager/main/socket-manager.h index b7c8f14..f310c25 100644 --- a/src/manager/main/socket-manager.h +++ b/src/manager/main/socket-manager.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000-2019 Samsung Electronics Co., Ltd. All rights reserved + * Copyright (c) 2014 - 2021 Samsung Electronics Co., Ltd. All rights reserved * * Contact: Dongsun Lee * @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -173,15 +174,6 @@ protected: void Handle(const SecurityEvent &event); // support for generic event Queue - struct Timeout { - time_t time; - int sock; - bool operator<(const Timeout &second) const - { - return time > second.time; // mininum first! - } - }; - SocketDescriptionVector m_socketDescriptionVector; fd_set m_readSet; fd_set m_writeSet; @@ -191,7 +183,23 @@ protected: std::queue m_eventQueue; int m_notifyMe[2]; int m_counter; - std::priority_queue m_timeoutQueue; + + class TimeoutQueue : public std::deque { + public: + void move_back(int sock) { + auto it = std::find(begin(), end(), sock); + assert(it != end()); + erase(it); + push_back(sock); + } + + void erase_value(int sock) { + auto it = std::find(begin(), end(), sock); + assert(it != end()); + erase(it); + } + }; + TimeoutQueue m_timeoutQueue; CommMgr m_commMgr; std::unique_ptr m_cynara; std::vector m_serviceVector; diff --git a/unit-tests/test_socket-manager.cpp b/unit-tests/test_socket-manager.cpp index e1a3af5..b55a31d 100644 --- a/unit-tests/test_socket-manager.cpp +++ b/unit-tests/test_socket-manager.cpp @@ -14,12 +14,18 @@ * limitations under the License */ +#include + #include #include #include #include #include +#include +#include +#include +#include #include #include @@ -27,13 +33,30 @@ #include #include +#include + using namespace CKM; -using namespace std::chrono_literals; namespace { constexpr char SERVICE_SOCKET_TEST[] = "/tmp/.central-key-manager-test.sock"; constexpr CKM::InterfaceID SOCKET_ID_TEST = 42; +constexpr std::chrono::seconds CV_TIMEOUT(10); + +struct TestSocketManager : public SocketManager { + size_t TimeoutQueueSize() const { return m_timeoutQueue.size(); } +}; + +#define THREAD_REQUIRE_MESSAGE(test, message) \ + do { \ + if (!(test)) { \ + std::ostringstream os; \ + os << __FILE__ << ":" << __LINE__ << " " << #test << " " << message; \ + throw std::runtime_error(os.str()); \ + } \ + } while (0) + +#define THREAD_REQUIRE(test) THREAD_REQUIRE_MESSAGE(test, "") class TestService : public GenericSocketService { public: @@ -42,35 +65,148 @@ public: {SERVICE_SOCKET_TEST, "", SOCKET_ID_TEST} }; } - void Event(const AcceptEvent &) override { Connected(true); } + void Event(const AcceptEvent &e) override { + std::unique_lock lock(m_mutex); + + THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, + "Unexpected server entry waiting for client match " << + m_connections.back().server); + + m_connections.push_back({-1 , e.connectionID.sock}); + + LogDebug("AcceptEvent. Added: ? <=>" << e.connectionID.sock); + + CompareSizes(); + + lock.unlock(); + m_cv.notify_one(); + } void Event(const WriteEvent &) override {} void Event(const ReadEvent &) override {} - void Event(const CloseEvent &) override { Connected(false); } + void Event(const CloseEvent &e) override { + std::unique_lock lock(m_mutex); + THREAD_REQUIRE(!m_connections.empty()); + + auto serverMatch = [&](const SocketPair& pair){ + return pair.server == e.connectionID.sock; + }; + auto it = std::find_if(m_connections.begin(), m_connections.end(), serverMatch); + + THREAD_REQUIRE_MESSAGE(it != m_connections.end(), + "Can't find connection for server socket = " << e.connectionID.sock); + + LogDebug("CloseEvent. Removing: " << it->client << "<=>" << it->server); + THREAD_REQUIRE(it->client != -1); + + m_connections.erase(it); + + CompareSizes(); + + lock.unlock(); + m_cv.notify_one(); + } void Event(const SecurityEvent &) override {} void Start() override {} void Stop() override {} - void WaitUntilConnected(bool isConnected) { + void ConnectAndWait(SockRAII& client) { std::unique_lock lock(m_mutex); - BOOST_REQUIRE(m_cv.wait_for(lock, 10s, [&]{ return m_connected == isConnected; })); + + THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, + "Unexpected server entry waiting for client match " << + m_connections.back().server); + + int ret = client.connect(GetServiceDescription()[0].serviceHandlerPath.c_str()); + BOOST_REQUIRE(ret == CKM_API_SUCCESS); + + LogDebug("Connected. Waiting for AcceptEvent for: " << client.get() << "<=> ?"); + + BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return AcceptEventArrived(); })); + + m_connections.back().client = client.get(); + + LogDebug("Accepted. Matched client & server: " << m_connections.back().client << "<=>" << + m_connections.back().server); } - void Connected(bool isConnected) { + void DisconnectAndWait(SockRAII& client) { + int sock = client.get(); + client.disconnect(); + + LogDebug("Disconnected. Waiting for CloseEvent for: " << sock << "<=> ?"); + std::unique_lock lock(m_mutex); - m_connected = isConnected; - lock.unlock(); - m_cv.notify_one(); + BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return ClientAbsent(sock); })); + } + + void WaitForRemainingClosures() { + std::unique_lock lock(m_mutex); + if (!m_connections.empty()) + BOOST_TEST_MESSAGE("Waiting for remaining " << m_connections.size() << " to close."); + + BOOST_REQUIRE(m_cv.wait_for(lock, std::chrono::seconds(OVERRIDE_SOCKET_TIMEOUT + 2), [&]{ + return m_connections.empty(); + })); + + CompareSizes(); } private: - bool m_connected = false; + bool ClientAbsent(int client) const { + auto it = std::find_if(m_connections.begin(), + m_connections.end(), [&](const SocketPair& pair){ + return pair.client == client; + }); + return it == m_connections.end(); + } + + bool AcceptEventArrived() const { + return !m_connections.empty() && m_connections.back().client == -1; + } + + void CompareSizes() const { + auto manager = static_cast(m_serviceManager); + THREAD_REQUIRE(m_connections.size() == manager->TimeoutQueueSize()); + } + std::mutex m_mutex; + struct SocketPair { + int client; + int server; + }; + std::vector m_connections; std::condition_variable m_cv; }; -struct TestSocketManager : public SocketManager { - size_t TimeoutQueueSize() const { return m_timeoutQueue.size(); } +class SocketManagerLoop { +public: + explicit SocketManagerLoop(SocketManager& manager) : + m_manager(manager), + m_thread([&]{ + try { + manager.MainLoop(); + } catch (const std::exception& e) { + m_exception = true; + m_what = e.what(); + } catch (...) { + m_exception = true; + } + }) + { + } + + ~SocketManagerLoop() { + m_manager.MainLoopStop(); + m_thread.join(); + BOOST_CHECK_MESSAGE(!m_exception, m_what); + } + +private: + bool m_exception = false; + std::string m_what = "Unknown exception"; + SocketManager& m_manager; + std::thread m_thread; }; } // namespace @@ -79,6 +215,7 @@ BOOST_AUTO_TEST_SUITE(SOCKET_MANAGER_TEST) POSITIVE_TEST_CASE(StressTestGrowingTimeoutQueue) { + constexpr unsigned INITIAL_CONNECTIONS = 20; constexpr unsigned REPEATS = 100000; constexpr auto INTERVAL = REPEATS/10; @@ -90,52 +227,27 @@ POSITIVE_TEST_CASE(StressTestGrowingTimeoutQueue) auto service = new TestService(); manager.RegisterSocketService(service); - bool exception = false; - std::string what("Unknown exception"); - - std::thread th([&]{ - try { - manager.MainLoop(); - } catch (const std::exception& e) { - exception = true; - what = e.what(); - } catch (...) { - exception = true; - } - }); + SocketManagerLoop loop(manager); { - SockRAII socket; - ret = socket.connect(SERVICE_SOCKET_TEST); - BOOST_REQUIRE(ret == CKM_API_SUCCESS); - service->WaitUntilConnected(true); - BOOST_REQUIRE(manager.TimeoutQueueSize() == 1); - service->Connected(false); + SockRAII socket[INITIAL_CONNECTIONS]; + for (unsigned i=0;iConnectAndWait(socket[i]); + + BOOST_REQUIRE(manager.TimeoutQueueSize() == INITIAL_CONNECTIONS); - // check if the closed socket timeouts are removed from the queue. SockRAII socket2; for(unsigned i=0;iWaitUntilConnected(true); - - socket2.disconnect(); - service->WaitUntilConnected(false); + service->ConnectAndWait(socket2); + service->DisconnectAndWait(socket2); if ((i + 1) % INTERVAL == 0) BOOST_TEST_MESSAGE("Creating connections: " << i + 1 << "/" << REPEATS); } - BOOST_TEST_MESSAGE("Waiting " << OVERRIDE_SOCKET_TIMEOUT + 2 << "s for socket timeouts."); - // all sockets should be closed after timeout and the queue should be empty - std::this_thread::sleep_for(std::chrono::seconds(OVERRIDE_SOCKET_TIMEOUT + 2)); - BOOST_REQUIRE(manager.TimeoutQueueSize() == 0); + // wait for remaining connections to close if any + service->WaitForRemainingClosures(); } - - manager.MainLoopStop(); - th.join(); - - BOOST_REQUIRE_MESSAGE(!exception, what); } BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From bf093164536fdeac3cf17e6d1dd27c2b4ae94a18 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Thu, 14 Jan 2021 13:54:15 +0100 Subject: [PATCH 10/16] Prevent writing to a socket marked as closed It is possible that select() marks a descriptor as ready for both read and write operation. If, additionally, the socket becomes closed in ReadyForRead(), the following call to ReadyForWrite() will attempt to write to a closed socket. It is harmless, unless the closed descriptor is already reused by another thread at the time of write(). This commit prevents it. Change-Id: Idaa829ef74d6df9f24c263f289aeca910b679713 --- src/manager/main/socket-manager.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 440e902..1d4d0f6 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -491,7 +491,9 @@ void SocketManager::MainLoop() } if (FD_ISSET(i, &writeSet)) { - ReadyForWrite(i); + // it is possible that the socket was closed in preceding call to ReadyForRead() + if (m_socketDescriptionVector[i].isOpen()) + ReadyForWrite(i); --ret; } } -- 2.7.4 From fa85ada1b67ba918f8d86adbdf323e2d22f159bc Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Thu, 14 Jan 2021 21:35:46 +0100 Subject: [PATCH 11/16] Add randomized socket manager stress test Registers several test services in the manager. In a loop: * Selects a random service * Selects a random action: * Create a new connection * Disconnect a random existing connection * Send random data through a random connection * Try to receive data from a random connection Change-Id: Id208e3a6ffbd1db82cd3389ba72bd0ff998d7c61 --- unit-tests/test_socket-manager.cpp | 428 +++++++++++++++++++++++++++---------- 1 file changed, 316 insertions(+), 112 deletions(-) diff --git a/unit-tests/test_socket-manager.cpp b/unit-tests/test_socket-manager.cpp index b55a31d..f869595 100644 --- a/unit-tests/test_socket-manager.cpp +++ b/unit-tests/test_socket-manager.cpp @@ -16,13 +16,15 @@ #include +#include +#include #include #include #include #include #include -#include +#include #include #include #include @@ -30,20 +32,28 @@ #include #include +#include +#include + #include #include - -#include +#include using namespace CKM; namespace { +size_t Random(size_t max) +{ + static unsigned int seed = ::time(nullptr); + return ::rand_r(&seed) % max; +} + constexpr char SERVICE_SOCKET_TEST[] = "/tmp/.central-key-manager-test.sock"; constexpr CKM::InterfaceID SOCKET_ID_TEST = 42; constexpr std::chrono::seconds CV_TIMEOUT(10); -struct TestSocketManager : public SocketManager { +struct TestSocketManager final : public SocketManager { size_t TimeoutQueueSize() const { return m_timeoutQueue.size(); } }; @@ -58,128 +68,64 @@ struct TestSocketManager : public SocketManager { #define THREAD_REQUIRE(test) THREAD_REQUIRE_MESSAGE(test, "") -class TestService : public GenericSocketService { -public: - ServiceDescriptionVector GetServiceDescription() override { - return ServiceDescriptionVector { - {SERVICE_SOCKET_TEST, "", SOCKET_ID_TEST} - }; - } - void Event(const AcceptEvent &e) override { - std::unique_lock lock(m_mutex); - - THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, - "Unexpected server entry waiting for client match " << - m_connections.back().server); - - m_connections.push_back({-1 , e.connectionID.sock}); - - LogDebug("AcceptEvent. Added: ? <=>" << e.connectionID.sock); - - CompareSizes(); - - lock.unlock(); - m_cv.notify_one(); - } +class NoOpService : public GenericSocketService { + void Event(const AcceptEvent &) override {} void Event(const WriteEvent &) override {} void Event(const ReadEvent &) override {} - void Event(const CloseEvent &e) override { - std::unique_lock lock(m_mutex); - THREAD_REQUIRE(!m_connections.empty()); - - auto serverMatch = [&](const SocketPair& pair){ - return pair.server == e.connectionID.sock; - }; - auto it = std::find_if(m_connections.begin(), m_connections.end(), serverMatch); - - THREAD_REQUIRE_MESSAGE(it != m_connections.end(), - "Can't find connection for server socket = " << e.connectionID.sock); - - LogDebug("CloseEvent. Removing: " << it->client << "<=>" << it->server); - THREAD_REQUIRE(it->client != -1); - - m_connections.erase(it); - - CompareSizes(); - - lock.unlock(); - m_cv.notify_one(); - } + void Event(const CloseEvent &) override {} void Event(const SecurityEvent &) override {} void Start() override {} void Stop() override {} +}; - void ConnectAndWait(SockRAII& client) { - std::unique_lock lock(m_mutex); - - THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, - "Unexpected server entry waiting for client match " << - m_connections.back().server); - - int ret = client.connect(GetServiceDescription()[0].serviceHandlerPath.c_str()); - BOOST_REQUIRE(ret == CKM_API_SUCCESS); - - LogDebug("Connected. Waiting for AcceptEvent for: " << client.get() << "<=> ?"); - - BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return AcceptEventArrived(); })); - - m_connections.back().client = client.get(); - - LogDebug("Accepted. Matched client & server: " << m_connections.back().client << "<=>" << - m_connections.back().server); - } - - void DisconnectAndWait(SockRAII& client) { - int sock = client.get(); - client.disconnect(); - - LogDebug("Disconnected. Waiting for CloseEvent for: " << sock << "<=> ?"); - - std::unique_lock lock(m_mutex); - BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return ClientAbsent(sock); })); +class TestConnection final : public ServiceConnection { +public: + explicit TestConnection(const std::string& socketPath) : + ServiceConnection(socketPath.c_str()), + m_id(m_counter++) { + auto ret = prepareConnection(); + BOOST_REQUIRE_MESSAGE(ret == CKM_API_SUCCESS, "ret = " << ret); } - void WaitForRemainingClosures() { - std::unique_lock lock(m_mutex); - if (!m_connections.empty()) - BOOST_TEST_MESSAGE("Waiting for remaining " << m_connections.size() << " to close."); - - BOOST_REQUIRE(m_cv.wait_for(lock, std::chrono::seconds(OVERRIDE_SOCKET_TIMEOUT + 2), [&]{ - return m_connections.empty(); - })); - - CompareSizes(); + int Send(const CKM::RawBuffer &send_buf) { + m_sent += send_buf.size(); + return ServiceConnection::send(SerializeMessage(send_buf)); } -private: - bool ClientAbsent(int client) const { - auto it = std::find_if(m_connections.begin(), - m_connections.end(), [&](const SocketPair& pair){ - return pair.client == client; - }); - return it == m_connections.end(); - } + template + void Receive(const T& logReceived) { + if (m_sent == 0) { + // expect timeout + auto ret = m_socket.waitForSocket(POLLIN, 100); + BOOST_REQUIRE_MESSAGE(ret == 0, "ret = " << ret); + logReceived(0); + return; + } - bool AcceptEventArrived() const { - return !m_connections.empty() && m_connections.back().client == -1; + int ret = ServiceConnection::receive(m_recv); + BOOST_REQUIRE_MESSAGE(ret == CKM_API_SUCCESS, "ret = " << ret); + BOOST_REQUIRE(m_recv.Ready()); + while (m_recv.Ready()) { + RawBuffer tmp; + m_recv.Deserialize(tmp); + const auto size = tmp.size(); + BOOST_REQUIRE_MESSAGE(size <= m_sent, size << ">" << m_sent); + logReceived(size); + m_sent -= size; + } } - void CompareSizes() const { - auto manager = static_cast(m_serviceManager); - THREAD_REQUIRE(m_connections.size() == manager->TimeoutQueueSize()); - } + size_t GetId() const { return m_id; } - std::mutex m_mutex; - struct SocketPair { - int client; - int server; - }; - std::vector m_connections; - std::condition_variable m_cv; +private: + size_t m_sent = 0; + size_t m_id; + MessageBuffer m_recv; + static inline size_t m_counter = 0; }; -class SocketManagerLoop { +class SocketManagerLoop final { public: explicit SocketManagerLoop(SocketManager& manager) : m_manager(manager), @@ -199,6 +145,7 @@ public: ~SocketManagerLoop() { m_manager.MainLoopStop(); m_thread.join(); + BOOST_CHECK_MESSAGE(!m_exception, m_what); } @@ -209,6 +156,16 @@ private: std::thread m_thread; }; +std::string Id2SockPath(int id) { + return std::string(SERVICE_SOCKET_TEST) + std::to_string(id); +} + +void unlinkIfExists(const char* path) { + int ret = unlink(path); + int err = errno; + BOOST_REQUIRE(ret == 0 || (ret == -1 && err == ENOENT)); +} + } // namespace BOOST_AUTO_TEST_SUITE(SOCKET_MANAGER_TEST) @@ -219,9 +176,124 @@ POSITIVE_TEST_CASE(StressTestGrowingTimeoutQueue) constexpr unsigned REPEATS = 100000; constexpr auto INTERVAL = REPEATS/10; - int ret = unlink(SERVICE_SOCKET_TEST); - int err = errno; - BOOST_REQUIRE(ret == 0 || (ret == -1 && err == ENOENT)); + class TestService final : public NoOpService { + public: + ServiceDescriptionVector GetServiceDescription() override { + return ServiceDescriptionVector { + {SERVICE_SOCKET_TEST, "", SOCKET_ID_TEST} + }; + } + void Event(const AcceptEvent &e) override { + std::unique_lock lock(m_mutex); + + THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, + "Unexpected server entry waiting for client match " << + m_connections.back().server); + + m_connections.push_back({-1 , e.connectionID.sock}); + + LogDebug("AcceptEvent. Added: ? <=>" << e.connectionID.sock); + + CompareSizes(); + + lock.unlock(); + m_cv.notify_one(); + } + void Event(const CloseEvent &e) override { + std::unique_lock lock(m_mutex); + THREAD_REQUIRE(!m_connections.empty()); + + auto serverMatch = [&](const SocketPair& pair){ + return pair.server == e.connectionID.sock; + }; + auto it = std::find_if(m_connections.begin(), m_connections.end(), serverMatch); + + THREAD_REQUIRE_MESSAGE(it != m_connections.end(), + "Can't find connection for server socket = " << + e.connectionID.sock); + + LogDebug("CloseEvent. Removing: " << it->client << "<=>" << it->server); + THREAD_REQUIRE(it->client != -1); + + m_connections.erase(it); + + CompareSizes(); + + lock.unlock(); + m_cv.notify_one(); + } + + void ConnectAndWait(SockRAII& client) { + std::unique_lock lock(m_mutex); + + THREAD_REQUIRE_MESSAGE(m_connections.empty() || m_connections.back().client != -1, + "Unexpected server entry waiting for client match " << + m_connections.back().server); + + int ret = client.connect(GetServiceDescription()[0].serviceHandlerPath.c_str()); + BOOST_REQUIRE(ret == CKM_API_SUCCESS); + + LogDebug("Connected. Waiting for AcceptEvent for: " << client.get() << "<=> ?"); + + BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return AcceptEventArrived(); })); + + m_connections.back().client = client.get(); + + LogDebug("Accepted. Matched client & server: " << m_connections.back().client << + "<=>" << m_connections.back().server); + } + + void DisconnectAndWait(SockRAII& client) { + int sock = client.get(); + client.disconnect(); + + LogDebug("Disconnected. Waiting for CloseEvent for: " << sock << "<=> ?"); + + std::unique_lock lock(m_mutex); + BOOST_REQUIRE(m_cv.wait_for(lock, CV_TIMEOUT, [&]{ return ClientAbsent(sock); })); + } + + void WaitForRemainingClosures() { + std::unique_lock lock(m_mutex); + if (!m_connections.empty()) + BOOST_TEST_MESSAGE("Waiting for remaining " << m_connections.size() << + " to close."); + + BOOST_REQUIRE(m_cv.wait_for(lock, + std::chrono::seconds(OVERRIDE_SOCKET_TIMEOUT + 2), + [&]{ return m_connections.empty(); })); + + CompareSizes(); + } + + private: + bool ClientAbsent(int client) const { + auto it = std::find_if(m_connections.begin(), + m_connections.end(), [&](const SocketPair& pair){ + return pair.client == client; + }); + return it == m_connections.end(); + } + + bool AcceptEventArrived() const { + return !m_connections.empty() && m_connections.back().client == -1; + } + + void CompareSizes() const { + auto manager = static_cast(m_serviceManager); + THREAD_REQUIRE(m_connections.size() == manager->TimeoutQueueSize()); + } + + std::mutex m_mutex; + struct SocketPair { + int client; + int server; + }; + std::vector m_connections; + std::condition_variable m_cv; + }; + + unlinkIfExists(SERVICE_SOCKET_TEST); TestSocketManager manager; auto service = new TestService(); @@ -250,4 +322,136 @@ POSITIVE_TEST_CASE(StressTestGrowingTimeoutQueue) } } +POSITIVE_TEST_CASE(StressTestRandomSocketEvents) +{ + // Too many services or connections may trigger server side timeouts (OVERRIDE_SOCKET_TIMEOUT) + constexpr int SERVICES = 4; + constexpr int INTERVAL = 1000; + constexpr int REPEATS = 10000; + constexpr int MAX_CONNECTIONS = 4; + // client and server read 2048B and 4096B at once respectively + constexpr size_t MAX_BUF_SIZE = 5000; + + enum Event { + CONNECT, + DISCONNECT, + SEND, + RECEIVE, + + CNT + }; + + class TestService final : public NoOpService { + public: + explicit TestService(int id) : + m_desc({{Id2SockPath(id).c_str(), "", SOCKET_ID_TEST + id}}), + m_id(id) { + + unlinkIfExists(GetSocketPath().c_str()); + } + + ServiceDescriptionVector GetServiceDescription() override { return m_desc; } + + void Event(const ReadEvent &e) override { + LogDebug(e.connectionID.sock << ":" << e.connectionID.counter << " Received " << + e.rawBuffer.size() << "B"); + m_serviceManager->Write(e.connectionID, e.rawBuffer); + } + + size_t GetConnectionCount() const { return m_connections.size(); } + + void AddConnection() { + m_connections.emplace_back(new TestConnection(GetSocketPath())); + LogDebug(Prefix(m_connections.back()->GetId()) << "Connected"); + } + + void Disconnect(size_t idx) { + auto it = m_connections.begin() + idx; + auto cid = (*it)->GetId(); + if (idx != m_connections.size() - 1) + *it = std::move(m_connections.back()); + m_connections.pop_back(); + LogDebug(Prefix(cid) << "Disconnected"); + } + + void Send(size_t idx) { + auto buffer = createRandom(Random(MAX_BUF_SIZE) + 1); + auto& conn = m_connections.at(idx); + auto ret = conn->Send(buffer); + BOOST_REQUIRE_MESSAGE(ret == CKM_API_SUCCESS, "ret = " << ret); + LogDebug(Prefix(conn->GetId())<< "Sent " << buffer.size() << "B"); + } + + void Receive(size_t idx) { + auto& conn = m_connections.at(idx); + conn->Receive([&](const size_t received) { + LogDebug(Prefix(conn->GetId()) << "Received " << received << "B"); + }); + } + + private: + const std::string& GetSocketPath() const { + return m_desc.at(0).serviceHandlerPath; + } + + std::string Prefix(size_t idx) const { + return std::string(" ") + std::to_string(m_id) + ":" + std::to_string(idx) + " "; + } + + ServiceDescriptionVector m_desc; + int m_id; + std::vector> m_connections; + }; + + SocketManager manager; + TestService* services[SERVICES]; + + for (int i = 0;iGetConnectionCount(); + if (cCnt > 0) { + cIdx = Random(cCnt); + eIdx = static_cast(Random(CNT)); + if (eIdx == CONNECT && cCnt == MAX_CONNECTIONS) + eIdx = SEND; // don't connect if there are too many + } + + switch (eIdx) { + case CONNECT: + service->AddConnection(); + break; + + case DISCONNECT: + service->Disconnect(cIdx); + break; + + case SEND: + service->Send(cIdx); + break; + + case RECEIVE: + service->Receive(cIdx); + break; + + default: + BOOST_FAIL("Unexpected event"); + } + + if ((i + 1) % INTERVAL == 0) + BOOST_TEST_MESSAGE("Executing random socket actions: " << i + 1 << "/" << REPEATS); + } +} + BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From af5db16af6f3efb9c134d3bf4b0a01c607e3b37e Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Thu, 21 Jan 2021 13:10:45 +0100 Subject: [PATCH 12/16] Use eventfd instead of pipes for notifications The kernel overhead of an eventfd file descriptor is much lower than that of a pipe, and only one file descriptor is required. Change-Id: Ie6d04d1ea8125190c35e1ef1655f517406eff807 --- src/manager/client-async/connection-thread.cpp | 70 ++++++++++++++------------ src/manager/client-async/connection-thread.h | 30 +++++------ src/manager/main/socket-manager.cpp | 17 ++++--- src/manager/main/socket-manager.h | 2 +- 4 files changed, 63 insertions(+), 56 deletions(-) diff --git a/src/manager/client-async/connection-thread.cpp b/src/manager/client-async/connection-thread.cpp index d663513..1de71de 100644 --- a/src/manager/client-async/connection-thread.cpp +++ b/src/manager/client-async/connection-thread.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000 - 2014 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2014 - 2021 Samsung Electronics Co., Ltd All Rights Reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -30,22 +31,35 @@ namespace CKM { -ConnectionThread::Pipe::Pipe() +ConnectionThread::EventFd::EventFd() { - if (-1 == pipe(m_pipe)) - ThrowMsg(PipeError, "Pipe creation failed " << GetErrnoString(errno)); + m_fd = eventfd(0, EFD_CLOEXEC); + if (-1 == m_fd) { + int err = errno; + ThrowMsg(EventFdError, "EventFd creation failed " << GetErrnoString(err)); + } +} + +ConnectionThread::EventFd::~EventFd() +{ + close(m_fd); } -ConnectionThread::Pipe::~Pipe() +void ConnectionThread::EventFd::notify() { - close(m_pipe[0]); - close(m_pipe[1]); + if (-1 == TEMP_FAILURE_RETRY(eventfd_write(m_fd, 1))) { + int err = errno; + ThrowMsg(EventFdError, "Writing eventfd failed " << GetErrnoString(err)); + } } -void ConnectionThread::Pipe::notify() +void ConnectionThread::EventFd::read() { - if (-1 == TEMP_FAILURE_RETRY(write(m_pipe[1], "j", 1))) - ThrowMsg(PipeError, "Writing pipe failed " << GetErrnoString(errno)); + eventfd_t cnt; + if (-1 == TEMP_FAILURE_RETRY(eventfd_read(m_fd, &cnt))) { + int err = errno; + ThrowMsg(EventFdError, "Failed to read eventfd: " << GetErrnoString(err)); + } } ConnectionThread::ConnectionThread() : @@ -58,7 +72,7 @@ ConnectionThread::~ConnectionThread() { m_join = true; try { - m_pipe.notify(); + m_eventFd.notify(); m_thread.join(); } catch (CKM::Exception &e) { LogError("CKM::Exception::Exception " << e.DumpToString()); @@ -80,21 +94,21 @@ void ConnectionThread::sendMessage(AsyncRequest &&req) m_waitingReqs.push(std::move(req)); lock.unlock(); - // notify via pipe - m_pipe.notify(); + // notify via eventfd + m_eventFd.notify(); } void ConnectionThread::threadLoop() { try { - m_descriptors.add(m_pipe.output(), + m_descriptors.add(m_eventFd.get(), POLLIN, - [this](int fd, short revents) { - newRequest(fd, revents); + [this](int, short revents) { + newRequest(revents); }); while (!m_join) { - // wait for pipe/socket notification + // wait for eventfd/socket notification m_descriptors.wait(); } } catch (CKM::Exception &e) { @@ -111,7 +125,7 @@ void ConnectionThread::threadLoop() m_services.clear(); - // close all descriptors (including pipe) + // close all descriptors (including eventfd) m_descriptors.purge(); // remove waiting requests and notify about error @@ -127,19 +141,6 @@ void ConnectionThread::threadLoop() m_finished = true; } -void ConnectionThread::readPipe(int pipe, short revents) -{ - char buffer[1]; - - if ((revents & POLLIN) == 0) - ThrowMsg(PipeError, "Unexpected event: " << revents << "!=" << POLLIN); - - if (1 != TEMP_FAILURE_RETRY(read(pipe, buffer, 1))) { - int err = errno; - ThrowMsg(PipeError, "Failed to read pipe: " << GetErrnoString(err)); - } -} - Service &ConnectionThread::getService(const std::string &interface) { auto it = m_services.find(interface); @@ -152,9 +153,12 @@ Service &ConnectionThread::getService(const std::string &interface) std::make_pair(interface, Service(m_descriptors, interface))).first->second; } -void ConnectionThread::newRequest(int pipe, short revents) +void ConnectionThread::newRequest(short revents) { - readPipe(pipe, revents); + if ((revents & POLLIN) == 0) + ThrowMsg(EventFdError, "Unexpected event: " << revents << "!=" << POLLIN); + + m_eventFd.read(); std::unique_lock lock(m_mutex); diff --git a/src/manager/client-async/connection-thread.h b/src/manager/client-async/connection-thread.h index ebae27c..11fcba0 100644 --- a/src/manager/client-async/connection-thread.h +++ b/src/manager/client-async/connection-thread.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000 - 2014 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2014 - 2021 Samsung Electronics Co., Ltd All Rights Reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -35,7 +35,7 @@ namespace CKM { class ConnectionThread { public: - DECLARE_EXCEPTION_TYPE(CKM::Exception, PipeError) + DECLARE_EXCEPTION_TYPE(CKM::Exception, EventFdError) ConnectionThread(); virtual ~ConnectionThread(); @@ -54,32 +54,34 @@ public: private: void threadLoop(); - void newRequest(int pipe, short revents); + void newRequest(short revents); - // reads notification pipe - void readPipe(int pipe, short revents); + // reads notification event fd + void readEventFd(short revents); Service &getService(const std::string &interface); - // Helper class that creates a pipe before thread is started - class Pipe { + // Helper class that creates an event fd before thread is started + class EventFd { public: - Pipe(); - ~Pipe(); + EventFd(); + ~EventFd(); - NONCOPYABLE(Pipe); + NONCOPYABLE(EventFd); void notify(); - int output() const + int get() const { - return m_pipe[0]; + return m_fd; } + void read(); + private: - int m_pipe[2]; + int m_fd; }; // shared vars - Pipe m_pipe; + EventFd m_eventFd; AsyncRequest::Queue m_waitingReqs; std::mutex m_mutex; bool m_join; diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 1d4d0f6..2a37cc6 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -195,15 +196,15 @@ SocketManager::SocketManager() : FD_ZERO(&m_readSet); FD_ZERO(&m_writeSet); - if (-1 == pipe(m_notifyMe)) { + m_notifyMe = eventfd(0, EFD_CLOEXEC); + if (-1 == m_notifyMe) { int err = errno; - ThrowMsg(Exception::InitFailed, "Error in pipe: " << GetErrnoString(err)); + ThrowMsg(Exception::InitFailed, "Error in eventfd: " << GetErrnoString(err)); } - LogInfo("Pipe: Read desc: " << m_notifyMe[0] << " Write desc: " << - m_notifyMe[1]); + LogInfo("Notifyfd : " << m_notifyMe); - auto &desc = CreateDefaultReadSocketDescription(m_notifyMe[0], false); + auto &desc = CreateDefaultReadSocketDescription(m_notifyMe, false); desc.service = new DummyService; m_serviceVector.push_back(desc.service); @@ -253,8 +254,8 @@ SocketManager::~SocketManager() if (m_socketDescriptionVector[i].isOpen()) close(i); - // All socket except one were closed. Now pipe input must be closed. - close(m_notifyMe[1]); + // All sockets except one were closed. Now eventfd input must be closed. + close(m_notifyMe); } void SocketManager::ReadyForAccept(int sock) @@ -689,7 +690,7 @@ void SocketManager::CreateEvent(EventFunction fun) void SocketManager::NotifyMe() { - TEMP_FAILURE_RETRY(write(m_notifyMe[1], "You have message ;-)", 1)); + TEMP_FAILURE_RETRY(eventfd_write(m_notifyMe, 1)); } void SocketManager::ProcessQueue() diff --git a/src/manager/main/socket-manager.h b/src/manager/main/socket-manager.h index f310c25..5319d24 100644 --- a/src/manager/main/socket-manager.h +++ b/src/manager/main/socket-manager.h @@ -181,7 +181,7 @@ protected: bool m_working; std::mutex m_eventQueueMutex; std::queue m_eventQueue; - int m_notifyMe[2]; + int m_notifyMe; int m_counter; class TimeoutQueue : public std::deque { -- 2.7.4 From 2418cdab7fd9273ef0fdd5047140f2f445e24077 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Mon, 25 Jan 2021 09:07:44 +0100 Subject: [PATCH 13/16] Catch exceptions before returning to cynara Callbacks registered in cynara may throw. Let's not propagate exceptions to cynara. Change-Id: Idc3bec6208495d0bfdb4d41c3ea0451352c9715b --- src/manager/main/cynara.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/manager/main/cynara.cpp b/src/manager/main/cynara.cpp index c25031f..0a314e6 100644 --- a/src/manager/main/cynara.cpp +++ b/src/manager/main/cynara.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2015 - 2021 Samsung Electronics Co., Ltd All Rights Reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -139,7 +139,14 @@ void Cynara::ChangeStatusCallback( cynara_async_status status, void *ptr) { - static_cast(ptr)->ChangeStatus(oldFd, newFd, status); + try { + static_cast(ptr)->ChangeStatus(oldFd, newFd, status); + } catch (const std::exception& e) { + LogError("Cynara::ChangeStatus failed: " << e.what()); + } catch (...) { + LogError("Cynara::ChangeStatus failed with unknown exception"); + } + } void Cynara::ProcessResponseCallback( @@ -148,7 +155,13 @@ void Cynara::ProcessResponseCallback( int response, void *ptr) { - static_cast(ptr)->ProcessResponse(checkId, cause, response); + try { + static_cast(ptr)->ProcessResponse(checkId, cause, response); + } catch (const std::exception& e) { + LogError("Cynara::ProcessResponse failed: " << e.what()); + } catch (...) { + LogError("Cynara::ProcessResponse failed with unknown exception"); + } } bool Cynara::GetUserFromSocket(int socket, std::string &user) -- 2.7.4 From d7fc8f32312c54ff47c1b184e24304dd9b17c56e Mon Sep 17 00:00:00 2001 From: Konrad Lipinski Date: Tue, 26 Jan 2021 10:09:17 +0100 Subject: [PATCH 14/16] Refrain from retrying close(int) (per man 2 close) Change-Id: I3343546c8aa2590e0147b89dc3c336d5e47a2d07 --- src/manager/main/socket-manager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 2a37cc6..4c7a91f 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2014 - 2021 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2014-2021 Samsung Electronics Co., Ltd. All rights reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -279,7 +279,7 @@ void SocketManager::ReadyForAccept(int sock) || !Cynara::GetUserFromSocket(client, user) || !Cynara::GetClientFromSocket(client, smack)) { LogDebug("Error in getting credentials from socket."); - TEMP_FAILURE_RETRY(close(client)); + close(client); return; } @@ -798,7 +798,7 @@ void SocketManager::CloseSocket(int sock) else LogError("Critical! Service is NULL! This should never happend!"); - TEMP_FAILURE_RETRY(close(sock)); + close(sock); FD_CLR(sock, &m_readSet); FD_CLR(sock, &m_writeSet); } -- 2.7.4 From edb4576bffc91fb451c3ad9281fe41cce01e6e81 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Mon, 25 Jan 2021 09:09:05 +0100 Subject: [PATCH 15/16] Validate cynara sockets Socket descriptors received from cynara are not validated which may lead to: - m_socketDescriptionVector buffer overflow/UB - reuse of already opened descriptors for cynara - growing m_socketDescriptionVector - closure of descriptors used by other parts of code - more than one cynara socket opened at the same time Change-Id: I5c6cd521fbde2a461f24e175571b74885d163b50 --- src/manager/main/socket-manager.cpp | 47 +++++++++++++++++++++++++++++-------- src/manager/main/socket-manager.h | 14 ++--------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/manager/main/socket-manager.cpp b/src/manager/main/socket-manager.cpp index 4c7a91f..ceb3834 100644 --- a/src/manager/main/socket-manager.cpp +++ b/src/manager/main/socket-manager.cpp @@ -166,7 +166,6 @@ SocketManager::CreateDefaultReadSocketDescription(int sock, bool timeout) auto &desc = m_socketDescriptionVector[sock]; desc.setListen(false); desc.setOpen(true); - desc.setCynara(false); desc.interfaceID = 0; desc.service = NULL; @@ -189,6 +188,7 @@ SocketManager::CreateDefaultReadSocketDescription(int sock, bool timeout) } SocketManager::SocketManager() : + m_cynaraSocket(-1), m_maxDesc(0), m_working(false), m_counter(0) @@ -332,7 +332,7 @@ void SocketManager::ReadyForRead(int sock) return; } - if (m_socketDescriptionVector[sock].isCynara()) { + if (sock == m_cynaraSocket) { m_cynara->ProcessSocket(); return; } @@ -375,7 +375,7 @@ void SocketManager::ReadyForRead(int sock) void SocketManager::ReadyForWrite(int sock) { - if (m_socketDescriptionVector[sock].isCynara()) { + if (sock == m_cynaraSocket) { m_cynara->ProcessSocket(); return; } @@ -779,6 +779,8 @@ void SocketManager::CloseSocket(int sock) return; } + assert(sock != m_cynaraSocket); + GenericSocketService::CloseEvent event; event.connectionID.sock = sock; event.connectionID.counter = desc.counter; @@ -806,22 +808,47 @@ void SocketManager::CloseSocket(int sock) void SocketManager::CynaraSocket(int oldFd, int newFd, bool isRW) { if (newFd != oldFd) { - if (newFd >= 0) { - auto &desc = CreateDefaultReadSocketDescription(newFd, false); - desc.service = nullptr; - desc.setCynara(true); - } - if (oldFd >= 0) { + if (oldFd != m_cynaraSocket) { + LogError("Invalid old cynara socket " << oldFd); + return; + } + auto &old = m_socketDescriptionVector[oldFd]; + + assert(old.isOpen()); + old.setOpen(false); - old.setCynara(false); + m_cynaraSocket = -1; FD_CLR(oldFd, &m_writeSet); FD_CLR(oldFd, &m_readSet); } + + if (newFd >= 0) { + if (m_cynaraSocket != -1) { + LogError("Another cynara socket is already opened"); + return; + } + + if (newFd < static_cast(m_socketDescriptionVector.size()) && + m_socketDescriptionVector[newFd].isOpen()) + { + LogError("New cynara socket " << newFd << " is already opened"); + return; + } + + auto &desc = CreateDefaultReadSocketDescription(newFd, false); + desc.service = nullptr; + m_cynaraSocket = newFd; + } } if (newFd >= 0) { + if (m_cynaraSocket != newFd) { + LogError("Invalid new cynara socket " << newFd); + return; + } + FD_SET(newFd, &m_readSet); if (isRW) diff --git a/src/manager/main/socket-manager.h b/src/manager/main/socket-manager.h index 5319d24..0f8c4b9 100644 --- a/src/manager/main/socket-manager.h +++ b/src/manager/main/socket-manager.h @@ -92,11 +92,6 @@ protected: return m_flags & LISTEN; } - bool isCynara() - { - return m_flags & CYNARA; - } - bool isTimeout() { return m_flags & TIMEOUT; @@ -112,11 +107,6 @@ protected: isSet ? m_flags |= LISTEN : m_flags &= ~LISTEN; } - void setCynara(bool isSet) - { - isSet ? m_flags |= CYNARA : m_flags &= ~CYNARA; - } - void setTimeout(bool isSet) { isSet ? m_flags |= TIMEOUT : m_flags &= ~TIMEOUT; @@ -141,8 +131,7 @@ protected: private: static const char LISTEN = 1 << 0; static const char OPEN = 1 << 1; - static const char CYNARA = 1 << 2; - static const char TIMEOUT = 1 << 3; + static const char TIMEOUT = 1 << 2; int m_flags; }; @@ -175,6 +164,7 @@ protected: // support for generic event Queue SocketDescriptionVector m_socketDescriptionVector; + int m_cynaraSocket; fd_set m_readSet; fd_set m_writeSet; int m_maxDesc; -- 2.7.4 From f79764f9d6537461cbdef94da6f1f7acea4ceb51 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Mon, 25 Jan 2021 16:01:56 +0100 Subject: [PATCH 16/16] Make SocketDescription getters const Change-Id: Ide41dc35598b423f8dac320b02b136b17a21c3cf --- src/manager/main/socket-manager.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/manager/main/socket-manager.h b/src/manager/main/socket-manager.h index 0f8c4b9..23c8b4c 100644 --- a/src/manager/main/socket-manager.h +++ b/src/manager/main/socket-manager.h @@ -82,17 +82,17 @@ protected: void CloseSocket(int sock); struct SocketDescription { - bool isOpen() + bool isOpen() const { return m_flags & OPEN; } - bool isListen() + bool isListen() const { return m_flags & LISTEN; } - bool isTimeout() + bool isTimeout() const { return m_flags & TIMEOUT; } -- 2.7.4