Handle exceptions from `sendCommand` in `ec_util.cpp` `sendCommand` in `ec_util.cpp` can throw `CommandRunException` exception. This exception is not handled at few places and can cause `hothd` to crash. This commit makes changes to catch all exceptions in functions which internally call this `sendCommand` method. Since `sendCommand` is a private method in `EcUtilImpl`, I looked at callers of methods in `ec_util.cpp` to find places where exception handling needs to be added. I added logic to throw `ResponseFailure` in methods that return value for DBus methods in `yaml/xyz/openbmc_project/Control/Hoth.interface.yaml` since the YAML config declares this error For `PayloadVersion::version()` method, since this is based on `sdbusplus::xyz::openbmc_project::Software::server::Version` interface, I added logic to return `SdBusError` exception. I also removed `EcUtilImpl::getHothPersistentPanicInfo` and updated the unit tests since that method does not seem to be used anywhere currently I tested the change by following steps that crash hothd without this change. With this change, I observed that hothd did not crash after I followed those steps. Google-Bug-Id: 552303543 Change-Id: Ic5ba46a56376227b20c92ddebb8e1cc08db52a17
diff --git a/ec_util.cpp b/ec_util.cpp index 2c1733e..2162cc2 100644 --- a/ec_util.cpp +++ b/ec_util.cpp
@@ -112,55 +112,6 @@ return stdplus::raw::copyFrom<ec_response_chip_info>(response_body); } -std::optional<ec_response_persistent_panic_info> - EcUtilImpl::getHothPersistentPanicInfo() const -{ - // ec_response_persistent_panic_info is 6KiB. Declare the return value this - // way to leverage NRVO. - std::optional<ec_response_persistent_panic_info> panic; - panic.emplace(); - - std::span<uint8_t> panic_buf(reinterpret_cast<uint8_t*>(&panic.value()), - sizeof(panic.value())); - - // The persistent panic info record is 6KiB long, so we have to retrieve it - // in chunks. - const size_t chunk_size = HOTH_PERSISTENT_PANIC_INFO_CHUNK_SIZE; - const size_t num_chunks = sizeof(panic.value()) / chunk_size; - auto ptr = panic_buf.begin(); - for (size_t i = 0; i < num_chunks; ++i, ptr += chunk_size) - { - ec_request_persistent_panic_info req = { - .operation = PERSISTENT_PANIC_INFO_GET, - .index = i, - }; - - std::vector<uint8_t> response_body = sendCommand( - EC_CMD_BOARD_SPECIFIC_BASE + EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO, - reinterpret_cast<uint8_t*>(&req), sizeof(req)); - if (response_body.size() != chunk_size) - { - stdplus::print(stderr, "Bad response length {} (expected {})\n", - response_body.size(), chunk_size); - throw ResponseFailure(); - } - - // The first chunk should contain a panic magic in the last 4 bytes in - // the panic_record. - if (i == 0) - { - if (!matchPersistentPanicMagic(response_body)) - { - panic.reset(); - return panic; - } - } - - std::copy(response_body.begin(), response_body.end(), ptr); - } - return panic; -} - bool EcUtilImpl::checkHothPersistentPanicInfo() const { ec_request_persistent_panic_info req = {
diff --git a/ec_util.hpp b/ec_util.hpp index 3eee387..6027049 100644 --- a/ec_util.hpp +++ b/ec_util.hpp
@@ -100,11 +100,6 @@ // EC_PRV_CMD_HOTH_CHIP_INFO command. ec_response_chip_info getHothChipInfo() const override; - // Get the Hoth persistent panic info by issuing a series of - // EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO commands. - std::optional<ec_response_persistent_panic_info> - getHothPersistentPanicInfo() const; - // Check the presence of Hoth peresistent panic info by issuing a single // EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO command. bool checkHothPersistentPanicInfo() const override;
diff --git a/hoth.cpp b/hoth.cpp index 70d4305..194a905 100644 --- a/hoth.cpp +++ b/hoth.cpp
@@ -17,6 +17,7 @@ #include "google3/ec_commands.h" #include "google3/host_commands.h" +#include "ec_util.hpp" #include "ec_util_interface.hpp" #include "firmware_mtd_updater.hpp" #include "message_util.hpp" @@ -591,7 +592,18 @@ uint32_t Hoth::getTotalBootTime() { - ec_response_statistics statistic = ecUtil->getHothStatistics(); + ec_response_statistics statistic; + try + { + statistic = ecUtil->getHothStatistics(); + } + catch (const std::exception& e) + { + stdplus::print(stderr, + "Fetching Hoth total boot time had an exception: {}\n", + e.what()); + throw ResponseFailure(); + } if (statistic.valid_words < (offsetof(decltype(statistic), boot_timing_total) + sizeof(statistic.boot_timing_total)) / @@ -607,7 +619,18 @@ uint32_t Hoth::getFirmwareUpdateTime() { - ec_response_statistics statistic = ecUtil->getHothStatistics(); + ec_response_statistics statistic; + try + { + statistic = ecUtil->getHothStatistics(); + } + catch (const std::exception& e) + { + stdplus::print( + stderr, "Fetching Hoth firmware update time had an exception: {}\n", + e.what()); + throw ResponseFailure(); + } if (statistic.valid_words < (offsetof(decltype(statistic), boot_timing_firmware_update) + sizeof(statistic.boot_timing_firmware_update)) / @@ -623,7 +646,19 @@ uint32_t Hoth::getFirmwareMirroringTime() { - ec_response_statistics statistic = ecUtil->getHothStatistics(); + ec_response_statistics statistic; + try + { + statistic = ecUtil->getHothStatistics(); + } + catch (const std::exception& e) + { + stdplus::print( + stderr, + "Fetching Hoth firmware mirroring time had an exception: {}\n", + e.what()); + throw ResponseFailure(); + } if (statistic.valid_words < (offsetof(decltype(statistic), boot_timing_firmware_mirroring) + sizeof(statistic.boot_timing_firmware_mirroring)) / @@ -639,7 +674,19 @@ uint32_t Hoth::getPayloadValidationTime() { - ec_response_statistics statistic = ecUtil->getHothStatistics(); + ec_response_statistics statistic; + try + { + statistic = ecUtil->getHothStatistics(); + } + catch (const std::exception& e) + { + stdplus::print( + stderr, + "Fetching Hoth payload validation time had an exception: {}\n", + e.what()); + throw ResponseFailure(); + } if (statistic.valid_words < (offsetof(decltype(statistic), boot_timing_payload_validation) + sizeof(statistic.boot_timing_payload_validation)) /
diff --git a/test/ec_util_unittest.cpp b/test/ec_util_unittest.cpp index b1d2b0f..69c474d 100644 --- a/test/ec_util_unittest.cpp +++ b/test/ec_util_unittest.cpp
@@ -183,13 +183,12 @@ EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO, ecUtil.kVersionZero, _, sizeof(ec_request_persistent_panic_info))) - .Times(2) + .Times(1) .WillRepeatedly(Return(rsp_buf)); EXPECT_THROW(ecUtil.checkHothPersistentPanicInfo(), ResponseFailure); - EXPECT_THROW(ecUtil.getHothPersistentPanicInfo(), ResponseFailure); } -TEST_F(EcUtilPersistentPanicTest, incorrectPanicMagicReturnsNullopt) +TEST_F(EcUtilPersistentPanicTest, incorrectPanicMagicPanicCheckFalse) { std::vector<uint8_t> rsp_buf(sizeof(kPanicResponseTemplate), 0); auto* rsp = reinterpret_cast<panic_host_command_response*>(rsp_buf.data()); @@ -200,13 +199,12 @@ EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO, ecUtil.kVersionZero, _, sizeof(ec_request_persistent_panic_info))) - .Times(2) + .Times(1) .WillRepeatedly(Return(rsp_buf)); EXPECT_FALSE(ecUtil.checkHothPersistentPanicInfo()); - EXPECT_FALSE(ecUtil.getHothPersistentPanicInfo()); } -TEST_F(EcUtilPersistentPanicTest, correctHostCommandReturnsFullPanicRecord) +TEST_F(EcUtilPersistentPanicTest, correctHostCommandPanicCheckTrue) { std::vector<uint8_t> rsp_bufs[12]; for (int i = 0; i < 12; ++i) @@ -224,7 +222,7 @@ EC_PRV_CMD_HOTH_PERSISTENT_PANIC_INFO, ecUtil.kVersionZero, _, sizeof(ec_request_persistent_panic_info))) - .Times(13) + .Times(1) .WillOnce(Return(rsp_bufs[0])) .WillOnce(Return(rsp_bufs[0])) .WillOnce(Return(rsp_bufs[1])) @@ -240,19 +238,6 @@ .WillOnce(Return(rsp_bufs[11])); EXPECT_TRUE(ecUtil.checkHothPersistentPanicInfo()); - - auto panic = ecUtil.getHothPersistentPanicInfo(); - std::span<uint8_t> panic_buf(reinterpret_cast<uint8_t*>(&panic.value()), - sizeof(panic.value())); - for (uint8_t i = 0; i < 12; ++i) - { - size_t chunk_start = - static_cast<uint32_t>(i) * HOTH_PERSISTENT_PANIC_INFO_CHUNK_SIZE; - EXPECT_EQ(panic_buf[chunk_start], i); - EXPECT_EQ( - panic_buf[chunk_start + HOTH_PERSISTENT_PANIC_INFO_CHUNK_SIZE - 1], - static_cast<uint8_t>(-i)); - } } class EcUtilAuthRecordTest : public EcUtilTest
diff --git a/version.cpp b/version.cpp index 4c90eb1..abd8938 100644 --- a/version.cpp +++ b/version.cpp
@@ -18,10 +18,12 @@ #include "message_util.hpp" +#include <sdbusplus/exception.hpp> #include <stdplus/print.hpp> #include <stdplus/raw.hpp> #include <xyz/openbmc_project/Control/Hoth/error.hpp> +#include <cerrno> #include <span> #include <stdexcept> #include <vector> @@ -70,9 +72,19 @@ std::string PayloadVersion::version() const { - payload_status_response resp = ecUtil->getPayloadStatus(); - uint8_t active_half = resp.header.active_half; + payload_status_response resp; + try + { + resp = ecUtil->getPayloadStatus(); + } + catch (const std::exception& e) + { + stdplus::print(stderr, "Failed to get payload status: {}\n", e.what()); + throw sdbusplus::exception::SdBusError(EIO, + "Failed to get payload status"); + } + uint8_t active_half = resp.header.active_half; if (active_half >= resp.header.region_count || active_half >= PAYLOAD_STATUS_MAX_REGION_STATES) { @@ -80,7 +92,8 @@ stderr, "Invalid active_half ({}) or region_count ({}) in payload status\n", active_half, resp.header.region_count); - throw ResponseFailure(); + throw sdbusplus::exception::SdBusError( + EBADMSG, "Invalid payload status response from hoth"); } return internal::payloadVersionString(resp.states[active_half]);