mctp: break circle dependencies in production code Break circle dependencies in production code to resolve memory leaks: - Change device to weak_ptr in MctpdEndpoint. - Actively transfer ownership of callback std::function rvalue references via local capture move in lambdas. - Invoke endpointRemoved() in ~MctpdDevice() for safe destruction. Major test improvement: - Add Test 15 to verify destruction notification. Misc test fixes: - Add two-stage ASIO event queue draining in test TearDown(). - Reorder stack variables to prevent stack use-after-free. - Add bounds to loops and use prctl for mock server cleanup. Tested: All 15 tests passed in Docker (Task id: task-7293). Google-Bug-Id: 505231613 Change-Id: I318a5654cbd0d3dbffa9edb416fd508343dc3354
diff --git a/src/MctpEndpoint.cpp b/src/MctpEndpoint.cpp index 7ffcc27..205ba9f 100644 --- a/src/MctpEndpoint.cpp +++ b/src/MctpEndpoint.cpp
@@ -335,10 +335,15 @@ std::string MctpdEndpoint::describe() const { - return std::string("network: ") - .append(std::to_string(mctp.network)) - .append(", EID: ") - .append(std::to_string(mctp.eid)) - .append(" | ") - .append(device->describe()); + std::string desc = std::string("network: ") + .append(std::to_string(mctp.network)) + .append(", EID: ") + .append(std::to_string(mctp.eid)) + .append(" | "); + + if (auto dev = device.lock()) + { + return desc.append(dev->describe()); + } + return desc.append("unknown device"); }
diff --git a/src/MctpEndpoint.hpp b/src/MctpEndpoint.hpp index 711d5e9..33d8114 100644 --- a/src/MctpEndpoint.hpp +++ b/src/MctpEndpoint.hpp
@@ -83,7 +83,8 @@ * recovered * * @param removed The callback to execute when the MCTP layer indicates the - * endpoint has been removed. + * endpoint has been removed. A remove event will also be + * issued when the MctpEndpoint object is destroyed. */ virtual void subscribe(Event&& degraded, Event&& available, Event&& removed) = 0; @@ -222,7 +223,7 @@ void removed(); private: - std::shared_ptr<MctpDevice> device; + std::weak_ptr<MctpDevice> device; std::shared_ptr<sdbusplus::asio::connection> connection; sdbusplus::message::object_path objpath; struct @@ -260,7 +261,10 @@ const std::vector<uint8_t>& physaddr); MctpdDevice(const MctpdDevice& other) = delete; MctpdDevice(MctpdDevice&& other) = delete; - ~MctpdDevice() override = default; + ~MctpdDevice() override + { + endpointRemoved(); + } void setup(std::function<void(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep)>&&
diff --git a/src/MctpReactorDevice.cpp b/src/MctpReactorDevice.cpp index 123ca4a..6ed3d88 100644 --- a/src/MctpReactorDevice.cpp +++ b/src/MctpReactorDevice.cpp
@@ -135,6 +135,8 @@ std::function<void(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep)>&& action) { + auto cb = std::move(action); + if (!targetBusInfo.empty()) { if (!callbackToken) @@ -161,7 +163,9 @@ { if (self->savedAction) { - self->setup(std::move(self->savedAction)); + auto act = std::move(self->savedAction); + self->savedAction = nullptr; + self->setup(std::move(act)); } } }); @@ -172,7 +176,7 @@ BusInfo epBusInfo = extractBusInfo(epConfig); if (epBusInfo == targetBusInfo) { - finaliseEndpoint(epPath, std::move(action)); + finaliseEndpoint(epPath, std::move(cb)); return; } } @@ -182,13 +186,13 @@ savedAction(std::make_error_code(std::errc::operation_canceled), nullptr); } - savedAction = std::move(action); + savedAction = std::move(cb); return; } // Fallback to old static path auto onGetPropertyReturned = - [weak{weak_from_this()}, action{std::move(action)}]( + [weak{weak_from_this()}, action{std::move(cb)}]( const boost::system::error_code& ec, const std::variant<std::vector<std::string>>& value) mutable { if (ec) @@ -309,7 +313,11 @@ endpoint = std::make_shared<MctpdEndpoint>(shared_from_this(), connection, endpointObjectPath, network, eid, isI2cAccessible); - action({}, endpoint); + + // Actively clear the caller's action wrapper to sever any lingering + // lifecycle dependencies bound to the callback's closure object. + auto execute = std::move(action); + execute({}, endpoint); } void MctpReactorDevice::endpointRemoved()
diff --git a/src/MctpUtil.cpp b/src/MctpUtil.cpp index b28c39b..8546584 100644 --- a/src/MctpUtil.cpp +++ b/src/MctpUtil.cpp
@@ -562,6 +562,7 @@ { associationMatch.reset(); associationRemoveMatch.reset(); + endpointStates.clear(); // Prevent state leaking between unit tests } MctpCallbackToken registerMctpEndpointCallback(MctpEndpointCallback&& cb)
diff --git a/tests/test_MctpUtil.cpp b/tests/test_MctpUtil.cpp index 3cbc58f..05a4a50 100644 --- a/tests/test_MctpUtil.cpp +++ b/tests/test_MctpUtil.cpp
@@ -19,9 +19,11 @@ * The tests run in a single thread using `io.poll()` to match the production * execution model. */ +#include "MctpReactorDevice.hpp" #include "MctpUtil.hpp" #include "Utils.hpp" +#include <sys/prctl.h> #include <sys/types.h> #include <sys/wait.h> #include <unistd.h> @@ -32,6 +34,7 @@ #include <sdbusplus/bus/match.hpp> #include <array> +#include <csignal> #include <cstdlib> #include <iostream> #include <memory> @@ -80,7 +83,11 @@ if (mockPid == 0) { // Child process + // Ask the kernel to automatically send SIGTERM to this process + // if the parent (the test suite) dies for any reason, including a + // segfault. // clang-format off + prctl(PR_SET_PDEATHSIG, SIGTERM); // NOLINT(cppcoreguidelines-pro-type-vararg) std::string scriptPath = TEST_SRC_DIR "/mock_mctpd.py"; execlp("python3", "python3", scriptPath.c_str(), nullptr); // NOLINT(cppcoreguidelines-pro-type-vararg) // clang-format on @@ -98,6 +105,20 @@ kill(mockPid, SIGTERM); waitpid(mockPid, nullptr, 0); } + + // First, process any pending D-Bus replies (like GetSubTree) while conn + // is still externally referenced. This prevents conn from destructing + // inside sd_bus_process if a callback drops the last reference. + while (io.poll() > 0) + {} + + // Now safely destroy the connection + conn.reset(); + + // Drain any cancellation handlers (e.g. async_read) queued by conn's + // destructor + while (io.poll() > 0) + {} } static bool checkPathInMapper(const std::string& path) @@ -1023,11 +1044,11 @@ targetBusInfo["Port"] = "1.2.5"; targetBusInfo["Configuration"] = "1"; + bool setupCompleted = false; + // Create MctpReactorDevice auto mctpDev = std::make_shared<MctpReactorDevice>( conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); - - bool setupCompleted = false; mctpDev->setup([&](const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep) { setupCompleted = true; @@ -1051,7 +1072,9 @@ io.poll(); usleep(100000); // 100ms if (setupCompleted) + { break; + } } EXPECT_TRUE(setupCompleted); @@ -1095,11 +1118,11 @@ targetBusInfo["Port"] = "1.2.5"; targetBusInfo["Configuration"] = "1"; + bool setupCompleted = false; + // Create MctpReactorDevice auto mctpDev = std::make_shared<MctpReactorDevice>( conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); - - bool setupCompleted = false; mctpDev->setup([&](const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep) { setupCompleted = true; @@ -1143,18 +1166,20 @@ } ASSERT_TRUE(populated); + bool sensorNotified = false; + // Create BusInfo for target device BusInfo targetBusInfo; targetBusInfo["BusType"] = "USB"; targetBusInfo["Port"] = "1.2.5"; targetBusInfo["Configuration"] = "1"; + bool setupCompleted = false; + std::shared_ptr<MctpEndpoint> endpoint; + // Create MctpReactorDevice auto mctpDev = std::make_shared<MctpReactorDevice>( conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); - - bool setupCompleted = false; - std::shared_ptr<MctpEndpoint> endpoint; mctpDev->setup([&](const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep) { setupCompleted = true; @@ -1167,7 +1192,6 @@ ASSERT_TRUE(setupCompleted); ASSERT_NE(endpoint, nullptr); - bool sensorNotified = false; endpoint->subscribe( nullptr, nullptr, [&](const std::shared_ptr<MctpEndpoint>&) { sensorNotified = true; }); @@ -1184,7 +1208,9 @@ io.poll(); usleep(100000); // 100ms if (sensorNotified) + { break; + } } EXPECT_TRUE(sensorNotified); @@ -1204,12 +1230,12 @@ targetBusInfo["Port"] = "1.2.5"; targetBusInfo["Configuration"] = "1"; + bool setupCompleted = false; + std::error_code errorResult; + // Create MctpReactorDevice auto mctpDev = std::make_shared<MctpReactorDevice>( conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); - - bool setupCompleted = false; - std::error_code errorResult; mctpDev->setup([&](const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep) { setupCompleted = true; @@ -1229,6 +1255,85 @@ EXPECT_EQ(errorResult, std::make_error_code(std::errc::operation_canceled)); } +// 15. Immediate Notification on Device Destruction (Case E) +TEST_F(MctpUtilTest, MctpReactorDevice_DestructionNotification_USB) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/14"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + setupMctpEndpointListener(conn, MctpMessageType::NVME_MI); + + // Trigger Add event FIRST! + std::string addCmd = + "busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 com.example.Control TriggerAdd ss \"" + + endpointPath + "\" \"" + emConfigPath + "\""; + runCmd(addCmd); + + // Wait for map to be populated + bool populated = false; + for (int i = 0; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (mctpEndpointConfigMap.find(endpointPath) != + mctpEndpointConfigMap.end()) + { + populated = true; + break; + } + } + ASSERT_TRUE(populated); + + // Create BusInfo for target device + BusInfo targetBusInfo; + targetBusInfo["BusType"] = "USB"; + targetBusInfo["Port"] = "1.2.5"; + targetBusInfo["Configuration"] = "1"; + + bool sensorNotified = false; + auto cb = [&](const std::shared_ptr<MctpEndpoint>&) { + sensorNotified = true; + }; + { + bool setupCompleted = false; + std::shared_ptr<MctpEndpoint> endpoint; + + // Create MctpReactorDevice + auto mctpDev = std::make_shared<MctpReactorDevice>( + conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); + + mctpDev->setup([&](const std::error_code&, + const std::shared_ptr<MctpEndpoint>& ep) { + setupCompleted = true; + endpoint = ep; + }); + + io.poll(); + ASSERT_TRUE(setupCompleted); + ASSERT_NE(endpoint, nullptr); + + endpoint->subscribe(nullptr, nullptr, cb); + } + + // mctpDev is destroyed here. Wait for the removal notification to be + // processed. + int i = 0; + for (; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (sensorNotified) + { + break; + } + } + + ASSERT_LT(i, 50) << "Timeout: Destruction notification was never fired!"; + EXPECT_TRUE(sensorNotified); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);