mctp: Fix priming race condition using global barrier Implement a global querying flag and shared barrier counter in MctpUtil.cpp to postpone all D-Bus events during the priming scan. This fixes a race condition where concurrent events arriving during the multi-step priming query could cause stale endpoints or inconsistent state. Add Tests 4, 5, and 6 to verify that concurrent events (Add, Remove, Remove->Add) are correctly deferred and processed after priming completes. Add #ifdef UNIT_TEST delay in MctpUtil.cpp to simulate the race condition. Change-Id: I66aa19275290611ab5f485a19e0282806441d47f Google-Bug-Id: 490106522 Signed-off-by: Hao Jiang <jianghao@google.com>
diff --git a/src/MctpUtil.cpp b/src/MctpUtil.cpp index 299f0fe..4b3ab4b 100644 --- a/src/MctpUtil.cpp +++ b/src/MctpUtil.cpp
@@ -27,6 +27,7 @@ { State state = State::None; bool querying = false; + std::string emConfigPath; }; static std::map<std::string, EndpointState> endpointStates; @@ -100,6 +101,34 @@ "org.freedesktop.DBus.ObjectManager", "GetManagedObjects"); } +static void triggerDeferredQueries( + const std::shared_ptr<sdbusplus::asio::connection>& conn) +{ + for (auto& [path, state] : endpointStates) + { + if (path == "__global__") + { + continue; + } + + if (state.state == State::Add) + { + state.querying = true; + state.state = State::None; + performEmConfigQuery(conn, path, state.emConfigPath); + } + else if (state.state == State::Remove) + { + state.state = State::None; + auto it = mctpEndpointConfigMap.find(path); + if (it != mctpEndpointConfigMap.end()) + { + mctpEndpointConfigMap.erase(it); + } + } + } +} + void setupMctpEndpointListener( const std::shared_ptr<sdbusplus::asio::connection>& conn) { @@ -118,8 +147,6 @@ std::variant<std::string, std::vector<std::string>>>> interfaces; msg.read(path, interfaces); - lg2::info("DEBUG: InterfacesAdded callback for {PATH}", "PATH", - path.str); auto it = interfaces.find("xyz.openbmc_project.Association"); if (it == interfaces.end()) @@ -151,8 +178,9 @@ } auto& state = endpointStates[endpointPath]; + state.emConfigPath = emConfigPath; - if (state.querying) + if (endpointStates["__global__"].querying || state.querying) { state.state = State::Add; return; @@ -182,7 +210,7 @@ auto& state = endpointStates[endpointPath]; - if (state.querying) + if (endpointStates["__global__"].querying || state.querying) { state.state = State::Remove; } @@ -199,72 +227,106 @@ }); // Prime the cache: Query existing Associations - constexpr auto associationInterfaces = - std::to_array({"xyz.openbmc_project.Association"}); - conn->async_method_call( - [conn](const boost::system::error_code& ec, - const GetSubTreeType& subtree) { - if (ec) - { - lg2::error("Failed to get associations from Object Mapper: {ERROR}", - "ERROR", ec.message()); - return; - } + endpointStates["__global__"].querying = true; + auto barrier = std::make_shared<int>(0); - for (const auto& [path, services] : subtree) - { - if (!path.ends_with("/configured_by")) +#ifdef UNIT_TEST + // Inject 5 second delay ONLY for tests to hit the race condition + auto timer = + std::make_shared<boost::asio::steady_timer>(conn->get_io_context()); + timer->expires_after(std::chrono::seconds(5)); + timer->async_wait([conn, barrier](const boost::system::error_code& ec) { + if (ec) + return; +#endif + + constexpr auto associationInterfaces = + std::to_array({"xyz.openbmc_project.Association"}); + conn->async_method_call( + [conn, barrier](const boost::system::error_code& ec, + const GetSubTreeType& subtree) { + if (ec) { - continue; + lg2::error( + "Failed to get associations from Object Mapper: {ERROR}", + "ERROR", ec.message()); + endpointStates["__global__"].querying = false; + return; } - conn->async_method_call( - [conn, - path](const boost::system::error_code& ec, - const std::variant<std::vector<std::string>>& value) { - if (ec) + if (subtree.empty()) + { + endpointStates["__global__"].querying = false; + return; + } + + for (const auto& [path, services] : subtree) + { + if (!path.ends_with("/configured_by")) { - lg2::error("Failed to get endpoints for {PATH}: {ERROR}", - "PATH", path, "ERROR", ec.message()); - return; + continue; } - const auto* endpoints = - std::get_if<std::vector<std::string>>(&value); - if (!endpoints || endpoints->empty()) - { - return; - } - - std::string emConfigPath = endpoints->front(); std::string endpointPath = path; constexpr std::string_view suffix = "/configured_by"; - if (endpointPath.ends_with("/configured_by")) - { - endpointPath = endpointPath.substr(0, endpointPath.size() - - suffix.length()); - } + endpointPath = endpointPath.substr(0, endpointPath.size() - + suffix.length()); - auto& state = endpointStates[endpointPath]; + (*barrier)++; // Increment barrier! - if (state.querying) - { - state.state = State::Add; - return; - } + conn->async_method_call( + [conn, barrier, endpointPath]( + const boost::system::error_code& ec, + const std::variant<std::vector<std::string>>& value) { + auto cleanup = [&]() { + (*barrier)--; + if (*barrier == 0) + { + endpointStates["__global__"].querying = false; + triggerDeferredQueries(conn); + } + }; - state.querying = true; - performEmConfigQuery(conn, endpointPath, emConfigPath); - }, - "xyz.openbmc_project.ObjectMapper", path, - "org.freedesktop.DBus.Properties", "Get", - "xyz.openbmc_project.Association", "endpoints"); - } - }, - "xyz.openbmc_project.ObjectMapper", - "/xyz/openbmc_project/object_mapper", - "xyz.openbmc_project.ObjectMapper", "GetSubTree", - "/au/com/codeconstruct/mctp1", 0, associationInterfaces); + if (ec) + { + lg2::error( + "Failed to get endpoints for {PATH}: {ERROR}", + "PATH", endpointPath, "ERROR", ec.message()); + cleanup(); + return; + } + + const auto* endpoints = + std::get_if<std::vector<std::string>>(&value); + if (!endpoints || endpoints->empty()) + { + cleanup(); + return; + } + + std::string emConfigPath = endpoints->front(); + + auto& state = endpointStates[endpointPath]; + state.emConfigPath = emConfigPath; + + state.querying = true; + performEmConfigQuery(conn, endpointPath, emConfigPath); + + cleanup(); + }, + "xyz.openbmc_project.ObjectMapper", path, + "org.freedesktop.DBus.Properties", "Get", + "xyz.openbmc_project.Association", "endpoints"); + } + }, + "xyz.openbmc_project.ObjectMapper", + "/xyz/openbmc_project/object_mapper", + "xyz.openbmc_project.ObjectMapper", "GetSubTree", + "/au/com/codeconstruct/mctp1", 0, associationInterfaces); + +#ifdef UNIT_TEST + }); +#endif } BusInfo extractBusInfo(const SensorData& configData)
diff --git a/tests/test_MctpUtil.cpp b/tests/test_MctpUtil.cpp index f643a1a..649d8ac 100644 --- a/tests/test_MctpUtil.cpp +++ b/tests/test_MctpUtil.cpp
@@ -332,6 +332,305 @@ EXPECT_FALSE(it->second.empty()); } +// 4. Race between priming query and new event +TEST_F(MctpUtilTest, PrimeQueryRace_TriggersDeferredQuery) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/test4"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + // Pre-populate association by triggering it BEFORE listener starts! + auto m = conn->new_method_call("xyz.openbmc_project.Mctp", + "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerAdd"); + m.append(endpointPath, emConfigPath); + conn->call(m); + + // Wait for mapperx to see it + bool found = false; + for (int i = 0; i < 300; i++) + { + if (checkPathInMapper(endpointPath + "/configured_by")) + { + found = true; + break; + } + usleep(100000); // 100ms + } + ASSERT_TRUE(found); + + // Now setup listener (this starts priming!) + setupMctpEndpointListener(conn); + + bool queryStarted = false; + int queryCompletedCount = 0; + + const std::string queryStartedSpec = + "type='signal',interface='com.example.Control',member='QueryStarted'"; + auto queryStartedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryStartedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryStarted received\n"; + queryStarted = true; + }); + + const std::string queryCompletedSpec = + "type='signal',interface='com.example.Control',member='QueryCompleted'"; + auto queryCompletedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryCompletedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryCompleted received\n"; + queryCompletedCount++; + }); + + // Wait for query to start (triggered by priming!) + while (!queryStarted) + { + io.poll(); + usleep(10000); // 10ms + } + + std::string endpointPath5 = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/test5"; + std::cout << "[DEBUG] Triggering Add for test5 (racing!)\n"; + // Trigger Add event for a DIFFERENT endpoint while priming query is in + // progress! + conn->async_method_call( + [](const boost::system::error_code& ec, const std::string&) { + if (ec) + { + std::cout << "[TEST] TriggerAdd failed: " << ec.message() << "\n"; + } + }, "xyz.openbmc_project.Mctp", "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerAdd", endpointPath5, emConfigPath); + + // Wait for queries to complete (we expect 2!) + for (int i = 0; i < 100; i++) + { + while (io.poll() > 0) + {} // Drain all ready handlers! + usleep(100000); // 100ms + if (queryCompletedCount >= 2) + { + break; + } + } + + // Wait a bit more to see if any extra signals arrive + usleep(500000); // 500ms + io.poll(); + + EXPECT_EQ(queryCompletedCount, 2); + + // Verify map populated for both endpoints! + EXPECT_EQ(mctpEndpointConfigMap.size(), 2); + + auto it4 = mctpEndpointConfigMap.find(endpointPath); + ASSERT_NE(it4, mctpEndpointConfigMap.end()); + EXPECT_FALSE(it4->second.empty()); + + auto it5 = mctpEndpointConfigMap.find(endpointPath5); + ASSERT_NE(it5, mctpEndpointConfigMap.end()); + EXPECT_FALSE(it5->second.empty()); +} + +// 5. During Priming, Remove on the single endpoint +TEST_F(MctpUtilTest, PrimeQueryRace_RemovePostponed) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/test5"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + // Pre-populate association by triggering it BEFORE listener starts! + auto m = conn->new_method_call("xyz.openbmc_project.Mctp", + "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerAdd"); + m.append(endpointPath, emConfigPath); + conn->call(m); + + // Wait for mapperx to see it + bool found = false; + for (int i = 0; i < 300; i++) + { + if (checkPathInMapper(endpointPath + "/configured_by")) + { + found = true; + break; + } + usleep(100000); // 100ms + } + ASSERT_TRUE(found); + + // Now setup listener (this starts priming!) + setupMctpEndpointListener(conn); + + bool queryStarted = false; + int queryCompletedCount = 0; + + const std::string queryStartedSpec = + "type='signal',interface='com.example.Control',member='QueryStarted'"; + auto queryStartedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryStartedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryStarted received\n"; + queryStarted = true; + }); + + const std::string queryCompletedSpec = + "type='signal',interface='com.example.Control',member='QueryCompleted'"; + auto queryCompletedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryCompletedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryCompleted received\n"; + queryCompletedCount++; + }); + + // Wait for query to start (triggered by priming!) + while (!queryStarted) + { + io.poll(); + usleep(10000); // 10ms + } + + std::cout << "[DEBUG] Triggering Remove (racing!)\n"; + // Trigger Remove event while priming query is in progress! + conn->async_method_call( + [](const boost::system::error_code& ec, const std::string&) { + if (ec) + { + std::cout << "[TEST] TriggerRemove failed: " << ec.message() + << "\n"; + } + }, "xyz.openbmc_project.Mctp", "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerRemove", endpointPath); + + // Wait for queries to complete (we expect 1!) + for (int i = 0; i < 100; i++) + { + while (io.poll() > 0) + {} // Drain all ready handlers! + usleep(100000); // 100ms + if (queryCompletedCount >= 1) + { + break; + } + } + + EXPECT_EQ(queryCompletedCount, 1); + + // Verify map is EMPTY (because it was removed!) + EXPECT_EQ(mctpEndpointConfigMap.size(), 0); + + auto it = mctpEndpointConfigMap.find(endpointPath); + EXPECT_EQ(it, mctpEndpointConfigMap.end()); +} + +// 6. During Priming, Remove -> Add on the single endpoint +TEST_F(MctpUtilTest, PrimeQueryRace_RemoveAddPostponed) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/test6"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + // Pre-populate association by triggering it BEFORE listener starts! + auto m = conn->new_method_call("xyz.openbmc_project.Mctp", + "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerAdd"); + m.append(endpointPath, emConfigPath); + conn->call(m); + + // Wait for mapperx to see it + bool found = false; + for (int i = 0; i < 300; i++) + { + if (checkPathInMapper(endpointPath + "/configured_by")) + { + found = true; + break; + } + usleep(100000); // 100ms + } + ASSERT_TRUE(found); + + // Now setup listener (this starts priming!) + setupMctpEndpointListener(conn); + + bool queryStarted = false; + int queryCompletedCount = 0; + + const std::string queryStartedSpec = + "type='signal',interface='com.example.Control',member='QueryStarted'"; + auto queryStartedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryStartedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryStarted received\n"; + queryStarted = true; + }); + + const std::string queryCompletedSpec = + "type='signal',interface='com.example.Control',member='QueryCompleted'"; + auto queryCompletedMatch = std::make_unique<sdbusplus::bus::match_t>( + static_cast<sdbusplus::bus_t&>(*conn), queryCompletedSpec, + [&](sdbusplus::message_t&) { + std::cout << "[TEST] QueryCompleted received\n"; + queryCompletedCount++; + }); + + // Wait for query to start (triggered by priming!) + while (!queryStarted) + { + io.poll(); + usleep(10000); // 10ms + } + + std::cout << "[DEBUG] Triggering Remove (racing!)\n"; + // Trigger Remove event! + conn->async_method_call( + [](const boost::system::error_code& ec, const std::string&) { + if (ec) + { + std::cout << "[TEST] TriggerRemove failed: " << ec.message() + << "\n"; + } + }, "xyz.openbmc_project.Mctp", "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerRemove", endpointPath); + + std::cout << "[DEBUG] Triggering Add again (racing!)\n"; + // Trigger Add event AGAIN! + conn->async_method_call( + [](const boost::system::error_code& ec, const std::string&) { + if (ec) + { + std::cout << "[TEST] TriggerAdd failed: " << ec.message() << "\n"; + } + }, "xyz.openbmc_project.Mctp", "/au/com/codeconstruct/mctp1", + "com.example.Control", "TriggerAdd", endpointPath, emConfigPath); + + // Wait for queries to complete (we expect 2!) + for (int i = 0; i < 100; i++) + { + while (io.poll() > 0) + {} // Drain all ready handlers! + usleep(100000); // 100ms + if (queryCompletedCount >= 2) + { + break; + } + } + + EXPECT_EQ(queryCompletedCount, 2); + + // Verify map populated + EXPECT_EQ(mctpEndpointConfigMap.size(), 1); + + auto it = mctpEndpointConfigMap.find(endpointPath); + ASSERT_NE(it, mctpEndpointConfigMap.end()); + EXPECT_FALSE(it->second.empty()); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);