diff --git a/score/launch_manager/src/daemon/src/osal/ipc_comms.hpp b/score/launch_manager/src/daemon/src/osal/ipc_comms.hpp index a0ce742513..d932fb6e16 100644 --- a/score/launch_manager/src/daemon/src/osal/ipc_comms.hpp +++ b/score/launch_manager/src/daemon/src/osal/ipc_comms.hpp @@ -15,6 +15,7 @@ #define OSAL_IPC_COMMS_HPP_INCLUDED #include +#include #include #include @@ -29,21 +30,30 @@ using IpcCommsP = std::shared_ptr; /// @brief Structure for managing inter-process communication synchronization. /// The `IpcCommsSync` structure is designed to handle synchronization mechanisms required -/// for inter-process communication. It uses a semaphore to signal kRunning and -/// a process ID to identify the communicating process. +/// for inter-process communication. It uses a semaphore to signal kRunning, preceded by a magic number and a +/// layout version that allow the client to verify that the shared memory really was set up by Launch Manager. /// It is only created for processes that are expected to report kRunning. struct IpcCommsSync final { + /// @brief Magic number used as a safeguard by application processes + static constexpr std::uint32_t kMagic{0x4C4D5352U}; + + /// @brief Expected value of `version_`. + static constexpr std::uint32_t kLayoutVersion{1U}; + + /// @brief Marks the shared memory as a comms object created by Launch Manager. + /// It is the first member, so that checking it does not depend on the layout of the remaining members. + /// Launch Manager writes it last, after all other members have been initialized. + std::uint32_t magic_; + + /// @brief Layout version of this structure, to detect a Launch Manager and client built from different versions. + std::uint32_t version_; + /// @brief Semaphore used by the process to report kRunning. /// The communicating process posts this semaphore exactly once to signal that it has finished initialization. /// Launch Manager waits on it (and also posts it to wake the waiter up early if the process terminates). Semaphore send_sync_; - /// @brief Process ID of the communicating process. - /// The `pid_` is written by the child process right after fork. The client checks it against its own pid. - /// The `pid_` is used as an expected bit pattern on the client side. - ProcessID pid_; - /// @brief Constant for the synchronization file descriptor. /// The `sync_fd` is a constant representing the file descriptor used for synchronization /// during communication. It is set to a value of 111 by default. @@ -109,6 +119,9 @@ struct IpcCommsSync final return nullptr; // Releasing mapping unmaps the memory } + mapping->version_ = kLayoutVersion; + mapping->magic_ = kMagic; + IpcCommsSync* const block = mapping.get(); return IpcCommsP(block, IpcCommsOwnerDeletor{std::move(mapping)}); } diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp index cffd838f04..2b5d691e67 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher.cpp @@ -89,7 +89,6 @@ void handleComms(score::mw::lifecycle::internal::osal::ChildProcessConfig& param } param.fd = dup2(param.fd, param.shared_block->sync_fd); - param.shared_block->pid_ = getpid(); // Store pid for check at client end // Flag FD_CLOEXEC is cleared to ensure that sync_fd remains open after the execve call. if (-1 == fcntl(IpcCommsSync::sync_fd, F_SETFD, 0)) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher_UT.cpp index e37ef1f417..90c274daff 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_launcher_UT.cpp @@ -71,7 +71,6 @@ class ProcessLauncherTest : public ::testing::Test std::memset(test_buffer.data(), 0, test_buffer.size()); auto* sync = reinterpret_cast(test_buffer.data()); - sync->pid_ = 0; EXPECT_EQ(sync->send_sync_.init(0, false), OsalReturnType::kSuccess); // Real msync calls would treat this fake shared memory as invalid because it isn't page aligned. ON_CALL(*g_syscall_mock, msync(sync, _, _)).WillByDefault(Return(0)); @@ -397,12 +396,18 @@ TEST_F(SetupCommsTest, getCommsFails) TEST_F(SetupCommsTest, initSemaphoresFails) { - RecordProperty("Description", "Verify that if setting up the semaphores fails, startProcess fails"); + RecordProperty( + "Description", + "Verify that if setting up the semaphores fails, startProcess fails, the shared memory is not marked as a " + "valid comms object and no semaphore is destroyed"); - StubShmObject(); + IpcCommsSync* block = StubShmObject(); + block->magic_ = 0U; EXPECT_CALL(*semaphore_mock_, sem_init).WillRepeatedly(Return(SemError(EINVAL))); + EXPECT_CALL(*g_syscall_mock, sem_destroy).Times(0); EXPECT_EQ(process_launcher->startProcess(pid_, sync_, config_), OsalReturnType::kFail); + EXPECT_EQ(block->magic_, 0U); } TEST_F(ProcessLauncherTest, releasingCommsObjectDestroysSemaphore) @@ -419,6 +424,8 @@ TEST_F(ProcessLauncherTest, releasingCommsObjectDestroysSemaphore) IpcCommsP block = IpcCommsSync::createCommsObject(fd); ASSERT_NE(block, nullptr); + EXPECT_EQ(block->magic_, IpcCommsSync::kMagic); + EXPECT_EQ(block->version_, IpcCommsSync::kLayoutVersion); EXPECT_CALL(*g_syscall_mock, msync(data, _, _)).WillOnce(Return(0)); Expectation destroyed = EXPECT_CALL(*g_syscall_mock, sem_destroy).WillOnce(Return(0)); @@ -797,7 +804,8 @@ TEST_F(StartProcessTest, startProcessChildSuccess) static_cast(process_launcher->startProcess(pid_, sync_, config_)); // No return from forked process - EXPECT_EQ(block->pid_, forked_pid); + EXPECT_EQ(block->magic_, IpcCommsSync::kMagic); + EXPECT_EQ(block->version_, IpcCommsSync::kLayoutVersion); } class ClientMethodsTest : public ProcessLauncherTest diff --git a/score/launch_manager/src/lifecycle_client/src/details/report_running_impl.cpp b/score/launch_manager/src/lifecycle_client/src/details/report_running_impl.cpp index 1fcc84ea95..3df8621ea5 100644 --- a/score/launch_manager/src/lifecycle_client/src/details/report_running_impl.cpp +++ b/score/launch_manager/src/lifecycle_client/src/details/report_running_impl.cpp @@ -78,9 +78,18 @@ score::Result ReportRunningImpl::reportKRunningtoDaemon() const } // This is our best safeguard against incorrect data treated as an IPCCommsSync - if (sync->pid_ != getpid()) + if (sync->magic_ != IpcCommsSync::kMagic) { - LM_LOG_ERROR() << "[Lifecycle client] Cannot report kRunning from a process not started by Launch Manager"; + LM_LOG_ERROR() << "[Lifecycle client] FD " << sync_fd + << " is not a Launch Manager comms object, cannot report kRunning"; + return comms_error; + } + + if (sync->version_ != IpcCommsSync::kLayoutVersion) + { + LM_LOG_ERROR() << "[Lifecycle client] Comms object version " << sync->version_ + << " does not match expected version " << IpcCommsSync::kLayoutVersion + << ", Launch Manager and client are incompatible"; return comms_error; } diff --git a/score/launch_manager/src/lifecycle_client/src/details/report_running_impl_UT.cpp b/score/launch_manager/src/lifecycle_client/src/details/report_running_impl_UT.cpp index e2d06c8e29..00dd4c4670 100644 --- a/score/launch_manager/src/lifecycle_client/src/details/report_running_impl_UT.cpp +++ b/score/launch_manager/src/lifecycle_client/src/details/report_running_impl_UT.cpp @@ -38,11 +38,14 @@ using namespace score::mw::lifecycle::internal::osal; alignas(IpcCommsSync) static char g_fake_ipc_storage[sizeof(IpcCommsSync)]; // Helper to create a fake IPC structure with specified properties -void* CreateFakeIpcComms(pid_t pid) +void* CreateFakeIpcComms( + std::uint32_t magic = IpcCommsSync::kMagic, + std::uint32_t version = IpcCommsSync::kLayoutVersion) { std::memset(g_fake_ipc_storage, 0, sizeof(IpcCommsSync)); IpcCommsSync* fake_comms = reinterpret_cast(g_fake_ipc_storage); - fake_comms->pid_ = pid; + fake_comms->magic_ = magic; + fake_comms->version_ = version; return g_fake_ipc_storage; } @@ -90,15 +93,12 @@ class ReportRunningImplTest : public ::testing::Test // By default, close succeeds EXPECT_CALL(*g_syscall_mock, close(_)).WillRepeatedly(Return(0)); - // By default, getpid returns current PID - EXPECT_CALL(*g_syscall_mock, getpid()).WillRepeatedly(Return(::getpid())); - // By default, mmap succeeds and returns valid IPC structure EXPECT_CALL(*g_syscall_mock, mmap(_, _, _, _, _, _)) .WillRepeatedly(Invoke([](void*, size_t length, int, int, int, off_t) -> void* { if (length == sizeof(IpcCommsSync)) { - return CreateFakeIpcComms(::getpid()); + return CreateFakeIpcComms(); } return MAP_FAILED; })); @@ -296,15 +296,47 @@ TEST_F(ReportRunningImplTest, GivenReportRunningImplInstance_ExceptGetCommsObjec EXPECT_EQ(result.error(), ExecErrc::kCommunicationError); } -TEST_F(ReportRunningImplTest, GivenWrongProcessId_WhenPidDoesNotMatch_ThenReturnsCommunicationError) +TEST_F(ReportRunningImplTest, GivenWrongMagic_WhenReportRunningStateCalled_ThenReturnsCommunicationError) { RecordProperty("DerivationTechnique", "explorative-testing"); RecordProperty( "Description", - "Given a ReportRunningImpl instace. " - "Expect a pid mismatch. " + "Given a ReportRunningImpl instance. " + "Expect the shared memory not to contain the Launch Manager magic number. " "When ReportRunningState() is called. " - "Then ReportRunningState() returns a kCommunicationError error. "); + "Then ReportRunningState() returns a kCommunicationError error and does not post the semaphore. "); + + // Given + ReportRunningImpl impl; + + // Expect + EXPECT_CALL(*g_syscall_mock, mmap(_, _, _, _, _, _)) + .WillOnce([](void*, size_t length, int, int, int, off_t) -> void* { + if (length == sizeof(IpcCommsSync)) + { + return CreateFakeIpcComms(0U, IpcCommsSync::kLayoutVersion); + } + return MAP_FAILED; + }); + EXPECT_CALL(*semaphore_mock_, sem_post(_)).Times(0); + + // When + auto result = impl.ReportRunningState(); + + // Then + EXPECT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), ExecErrc::kCommunicationError); +} + +TEST_F(ReportRunningImplTest, GivenWrongLayoutVersion_WhenReportRunningStateCalled_ThenReturnsCommunicationError) +{ + RecordProperty("DerivationTechnique", "explorative-testing"); + RecordProperty( + "Description", + "Given a ReportRunningImpl instance. " + "Expect the shared memory to contain a different layout version. " + "When ReportRunningState() is called. " + "Then ReportRunningState() returns a kCommunicationError error and does not post the semaphore. "); // Given ReportRunningImpl impl; @@ -314,10 +346,11 @@ TEST_F(ReportRunningImplTest, GivenWrongProcessId_WhenPidDoesNotMatch_ThenReturn .WillOnce([](void*, size_t length, int, int, int, off_t) -> void* { if (length == sizeof(IpcCommsSync)) { - return CreateFakeIpcComms(99999); + return CreateFakeIpcComms(IpcCommsSync::kMagic, IpcCommsSync::kLayoutVersion + 1U); } return MAP_FAILED; }); + EXPECT_CALL(*semaphore_mock_, sem_post(_)).Times(0); // When auto result = impl.ReportRunningState();