From b4b106407959d567eddb113beef15dfbc6a8d28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Dom=C3=ADnguez=20L=C3=B3pez?= <116071334+Mario-DL@users.noreply.github.com> Date: Thu, 14 Nov 2024 11:03:19 +0100 Subject: [PATCH 1/2] [22024] Improve `OpenSSL` lifecycle handling (#5384) * Refs #22024: Add BB test Signed-off-by: Mario Dominguez * Refs #22024: Make OpenSSLInit Mayers singleton Signed-off-by: Mario Dominguez * Refs #22024: Fix: Do not register atexit in OPenSSL. Instead, Comply with OpenSSL initialization and destruction. Signed-off-by: Mario Dominguez * Refs #22024: Do not reference OpenSSLInit if security features are no present Signed-off-by: Mario Dominguez --------- Signed-off-by: Mario Dominguez (cherry picked from commit 44310c4f8efabfacc55d14604d25175d8f33070a) # Conflicts: # src/cpp/rtps/RTPSDomainImpl.hpp --- src/cpp/rtps/RTPSDomainImpl.hpp | 11 ++++++ src/cpp/rtps/security/SecurityManager.cpp | 1 - src/cpp/security/OpenSSLInit.hpp | 39 ++++++++++++------- .../blackbox/common/BlackboxTestsSecurity.cpp | 27 +++++++++++++ 4 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/cpp/rtps/RTPSDomainImpl.hpp b/src/cpp/rtps/RTPSDomainImpl.hpp index 1f736cd66f2..f9c4f27c4d9 100644 --- a/src/cpp/rtps/RTPSDomainImpl.hpp +++ b/src/cpp/rtps/RTPSDomainImpl.hpp @@ -31,7 +31,15 @@ #include +<<<<<<< HEAD #include +======= +#if HAVE_SECURITY +#include +#endif // HAVE_SECURITY + +#include +>>>>>>> 44310c4f8 ([22024] Improve `OpenSSL` lifecycle handling (#5384)) namespace eprosima { namespace fastrtps { @@ -249,6 +257,9 @@ class RTPSDomainImpl std::shared_ptr boost_singleton_handler_ { eprosima::detail:: BoostAtExitRegistry:: get_instance() }; +#if HAVE_SECURITY + std::shared_ptr openssl_singleton_handler_{ security::OpenSSLInit::get_instance() }; +#endif // HAVE_SECURITY std::mutex m_mutex; diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index b0317e48ec3..0e4a7a6dbd3 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -94,7 +94,6 @@ SecurityManager::SecurityManager( participant->getRTPSParticipantAttributes().allocation.data_limits}) { assert(participant != nullptr); - static OpenSSLInit openssl_init; } SecurityManager::~SecurityManager() diff --git a/src/cpp/security/OpenSSLInit.hpp b/src/cpp/security/OpenSSLInit.hpp index 91391ab74eb..ed2b5d3b5c9 100644 --- a/src/cpp/security/OpenSSLInit.hpp +++ b/src/cpp/security/OpenSSLInit.hpp @@ -1,3 +1,19 @@ +// Copyright 2024 Proyectos y Sistemas de Mantenimiento SL (eProsima). +// +// 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 @@ -14,24 +30,19 @@ class OpenSSLInit OpenSSLInit() { -#if OPENSSL_VERSION_NUMBER < 0x10100000L - OpenSSL_add_all_algorithms(); -#endif // if OPENSSL_VERSION_NUMBER < 0x10100000L + uint64_t opts = OPENSSL_INIT_NO_ATEXIT; + OPENSSL_init_crypto(opts, NULL); } ~OpenSSLInit() { -#if OPENSSL_VERSION_NUMBER < 0x10000000L - ERR_remove_state(0); - ENGINE_cleanup(); -#elif OPENSSL_VERSION_NUMBER < 0x10100000L - ERR_remove_thread_state(NULL); - ENGINE_cleanup(); -#endif // if OPENSSL_VERSION_NUMBER < 0x10000000L - RAND_cleanup(); - CRYPTO_cleanup_all_ex_data(); - ERR_free_strings(); - EVP_cleanup(); + OPENSSL_cleanup(); + } + + static std::shared_ptr get_instance() + { + static auto instance = std::make_shared(); + return instance; } }; diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 3e2211330a2..c416eb3193d 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -5191,6 +5191,33 @@ TEST(Security, participant_stateless_secure_writer_pool_change_is_removed_upon_p EXPECT_EQ(0u, n_logs); } +// Regression test for Redmine issue #22024 +// OpenSSL assertion is not thrown when the library is abruptly finished. +TEST(Security, openssl_correctly_finishes) +{ + // Create + PubSubWriter writer("HelloWorldTopic_openssl_is_correctly_finished"); + PubSubReader reader("HelloWorldTopic_openssl_is_correctly_finished"); + + const std::string governance_file("governance_helloworld_all_enable.smime"); + const std::string permissions_file("permissions_helloworld.smime"); + + CommonPermissionsConfigure(reader, writer, governance_file, permissions_file); + + reader.init(); + writer.init(); + + ASSERT_TRUE(reader.isInitialized()); + ASSERT_TRUE(writer.isInitialized()); + + std::this_thread::sleep_for(std::chrono::seconds(2)); + + // Here we force the atexit function from openssl to be abruptly called + // i.e in a disordered way + // If OpenSSL is not correctly finished, a SIGSEGV will be thrown + std::exit(0); +} + void blackbox_security_init() { certs_path = std::getenv("CERTS_PATH"); From 1a1f29c25dfaa3d0a591dc36bd4377f47c53cfbe Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Fri, 15 Nov 2024 08:09:07 +0100 Subject: [PATCH 2/2] Solve conflicts Signed-off-by: Mario Dominguez --- src/cpp/rtps/RTPSDomainImpl.hpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/cpp/rtps/RTPSDomainImpl.hpp b/src/cpp/rtps/RTPSDomainImpl.hpp index f9c4f27c4d9..1abe1490805 100644 --- a/src/cpp/rtps/RTPSDomainImpl.hpp +++ b/src/cpp/rtps/RTPSDomainImpl.hpp @@ -29,18 +29,13 @@ #include #include +#include #include -<<<<<<< HEAD -#include -======= #if HAVE_SECURITY #include #endif // HAVE_SECURITY -#include ->>>>>>> 44310c4f8 ([22024] Improve `OpenSSL` lifecycle handling (#5384)) - namespace eprosima { namespace fastrtps { namespace rtps {