From e01d0c36af115ed46d54b5dbbacfe3ad92226bd3 Mon Sep 17 00:00:00 2001 From: Ed Tanous Date: Fri, 30 Jun 2023 13:21:32 -0700 Subject: [PATCH] Fix bugprone-unchecked-optional-access findings Clang-tidy has the aforementioned check, which shows a few places in the core where we ignored the required optional checks. Fix all uses. Note, we cannot enable the check that this time because of some weird code in health.hpp that crashes tidy[1]. That will need to be a future improvement. There are tests that call something like ASSERT(optional) EXPECT(optional->foo()) While this isn't an actual violation, clang-tidy doesn't seem to be smart enough to deal with it, so add some explicit checks. [1] https://github.com/llvm/llvm-project/issues/55530 Tested: Redfish service validator passes. Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3 Signed-off-by: Ed Tanous --- http/http2_connection.hpp | 4 +- http/http_client.hpp | 21 ++++--- http/http_connection.hpp | 23 +++++++- http/http_response.hpp | 46 +++++++-------- include/persistent_data.hpp | 5 +- redfish-core/include/query.hpp | 2 +- redfish-core/include/utils/json_utils.hpp | 2 +- redfish-core/lib/account_service.hpp | 23 ++++---- redfish-core/lib/ethernet.hpp | 42 +++++--------- redfish-core/lib/virtual_media.hpp | 35 +++++++----- test/http/verb_test.cpp | 3 +- .../include/utils/query_param_test.cpp | 56 +++++++++++++++---- 12 files changed, 156 insertions(+), 106 deletions(-) diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp index a63b234575..27df36e82f 100644 --- a/http/http2_connection.hpp +++ b/http/http2_connection.hpp @@ -154,8 +154,8 @@ class HTTP2Connection : completeResponseFields(thisReq, thisRes); thisRes.addHeader(boost::beast::http::field::date, getCachedDateStr()); - boost::beast::http::fields& fields = thisRes.stringResponse->base(); - std::string code = std::to_string(thisRes.stringResponse->result_int()); + boost::beast::http::fields& fields = thisRes.stringResponse.base(); + std::string code = std::to_string(thisRes.stringResponse.result_int()); hdr.emplace_back(headerFromStringViews(":status", code)); for (const boost::beast::http::fields::value_type& header : fields) { diff --git a/http/http_client.hpp b/http/http_client.hpp index d82c566a06..71fb885551 100644 --- a/http/http_client.hpp +++ b/http/http_client.hpp @@ -125,6 +125,7 @@ struct PendingRequest {} }; +namespace http = boost::beast::http; class ConnectionInfo : public std::enable_shared_from_this { private: @@ -137,10 +138,9 @@ class ConnectionInfo : public std::enable_shared_from_this uint32_t connId; // Data buffers - boost::beast::http::request req; - std::optional< - boost::beast::http::response_parser> - parser; + http::request req; + using parser_type = http::response_parser; + std::optional parser; boost::beast::flat_static_buffer buffer; Response res; @@ -329,9 +329,10 @@ class ConnectionInfo : public std::enable_shared_from_this { state = ConnState::recvInProgress; - parser.emplace(std::piecewise_construct, std::make_tuple()); + parser_type& thisParser = parser.emplace(std::piecewise_construct, + std::make_tuple()); - parser->body_limit(connPolicy->requestByteLimit); + thisParser.body_limit(connPolicy->requestByteLimit); timer.expires_after(std::chrono::seconds(30)); timer.async_wait(std::bind_front(onTimeout, weak_from_this())); @@ -340,14 +341,14 @@ class ConnectionInfo : public std::enable_shared_from_this if (sslConn) { boost::beast::http::async_read( - *sslConn, buffer, *parser, + *sslConn, buffer, thisParser, std::bind_front(&ConnectionInfo::afterRead, this, shared_from_this())); } else { boost::beast::http::async_read( - conn, buffer, *parser, + conn, buffer, thisParser, std::bind_front(&ConnectionInfo::afterRead, this, shared_from_this())); } @@ -375,6 +376,10 @@ class ConnectionInfo : public std::enable_shared_from_this } BMCWEB_LOG_DEBUG("recvMessage() bytes transferred: {}", bytesTransferred); + if (!parser) + { + return; + } BMCWEB_LOG_DEBUG("recvMessage() data: {}", parser->get().body()); unsigned int respCode = parser->get().result_int(); diff --git a/http/http_connection.hpp b/http/http_connection.hpp index b5d0d2e59e..ba4af3f747 100644 --- a/http/http_connection.hpp +++ b/http/http_connection.hpp @@ -94,6 +94,10 @@ class Connection : // don't require auth if (preverified) { + if (!req) + { + return false; + } mtlsSession = verifyMtlsUser(req->ipAddress, ctx); if (mtlsSession) { @@ -202,6 +206,10 @@ class Connection : void handle() { std::error_code reqEc; + if (!parser) + { + return; + } crow::Request& thisReq = req.emplace(parser->release(), reqEc); if (reqEc) { @@ -363,6 +371,10 @@ class Connection : { return; } + if (!req) + { + return; + } req->ipAddress = ip; } @@ -389,7 +401,10 @@ class Connection : void doReadHeaders() { BMCWEB_LOG_DEBUG("{} doReadHeaders", logPtr(this)); - + if (!parser) + { + return; + } // Clean up any previous Connection. boost::beast::http::async_read_header( adaptor, buffer, *parser, @@ -475,6 +490,10 @@ class Connection : void doRead() { BMCWEB_LOG_DEBUG("{} doRead", logPtr(this)); + if (!parser) + { + return; + } startDeadline(); boost::beast::http::async_read_some( adaptor, buffer, *parser, @@ -515,7 +534,7 @@ class Connection : { BMCWEB_LOG_DEBUG("{} doWrite", logPtr(this)); thisRes.preparePayload(); - serializer.emplace(*thisRes.stringResponse); + serializer.emplace(thisRes.stringResponse); startDeadline(); boost::beast::http::async_write(adaptor, *serializer, [this, self(shared_from_this())]( diff --git a/http/http_response.hpp b/http/http_response.hpp index c4f9366ed3..cb07a83635 100644 --- a/http/http_response.hpp +++ b/http/http_response.hpp @@ -23,26 +23,26 @@ struct Response using response_type = boost::beast::http::response; - std::optional stringResponse; + response_type stringResponse; nlohmann::json jsonValue; void addHeader(std::string_view key, std::string_view value) { - stringResponse->insert(key, value); + stringResponse.insert(key, value); } void addHeader(boost::beast::http::field key, std::string_view value) { - stringResponse->insert(key, value); + stringResponse.insert(key, value); } void clearHeader(boost::beast::http::field key) { - stringResponse->erase(key); + stringResponse.erase(key); } - Response() : stringResponse(response_type{}) {} + Response() = default; Response(Response&& res) noexcept : stringResponse(std::move(res.stringResponse)), @@ -73,7 +73,7 @@ struct Response return *this; } stringResponse = std::move(r.stringResponse); - r.stringResponse.emplace(response_type{}); + r.stringResponse.clear(); jsonValue = std::move(r.jsonValue); // Only need to move completion handler if not already completed @@ -98,27 +98,27 @@ struct Response void result(unsigned v) { - stringResponse->result(v); + stringResponse.result(v); } void result(boost::beast::http::status v) { - stringResponse->result(v); + stringResponse.result(v); } boost::beast::http::status result() const { - return stringResponse->result(); + return stringResponse.result(); } unsigned resultInt() const { - return stringResponse->result_int(); + return stringResponse.result_int(); } std::string_view reason() const { - return stringResponse->reason(); + return stringResponse.reason(); } bool isCompleted() const noexcept @@ -128,29 +128,29 @@ struct Response std::string& body() { - return stringResponse->body(); + return stringResponse.body(); } std::string_view getHeaderValue(std::string_view key) const { - return stringResponse->base()[key]; + return stringResponse.base()[key]; } void keepAlive(bool k) { - stringResponse->keep_alive(k); + stringResponse.keep_alive(k); } bool keepAlive() const { - return stringResponse->keep_alive(); + return stringResponse.keep_alive(); } void preparePayload() { // This code is a throw-free equivalent to // beast::http::message::prepare_payload - boost::optional pSize = stringResponse->payload_size(); + boost::optional pSize = stringResponse.payload_size(); using boost::beast::http::status; using boost::beast::http::status_class; using boost::beast::http::to_status_class; @@ -160,12 +160,11 @@ struct Response } else { - bool is1XXReturn = to_status_class(stringResponse->result()) == + bool is1XXReturn = to_status_class(stringResponse.result()) == status_class::informational; if (*pSize > 0 && - (is1XXReturn || - stringResponse->result() == status::no_content || - stringResponse->result() == status::not_modified)) + (is1XXReturn || stringResponse.result() == status::no_content || + stringResponse.result() == status::not_modified)) { BMCWEB_LOG_CRITICAL( "{} Response content provided but code was no-content or not_modified, which aren't allowed to have a body", @@ -174,13 +173,14 @@ struct Response body().clear(); } } - stringResponse->content_length(*pSize); + stringResponse.content_length(*pSize); } void clear() { BMCWEB_LOG_DEBUG("{} Clearing response containers", logPtr(this)); - stringResponse.emplace(response_type{}); + stringResponse.clear(); + stringResponse.body().shrink_to_fit(); jsonValue = nullptr; completed = false; expectedHash = std::nullopt; @@ -188,7 +188,7 @@ struct Response void write(std::string_view bodyPart) { - stringResponse->body() += std::string(bodyPart); + stringResponse.body() += std::string(bodyPart); } std::string computeEtag() const diff --git a/include/persistent_data.hpp b/include/persistent_data.hpp index 5079a8c151..048d9877da 100644 --- a/include/persistent_data.hpp +++ b/include/persistent_data.hpp @@ -234,9 +234,10 @@ class ConfigFile session["username"] = p.second->username; session["csrf_token"] = p.second->csrfToken; session["client_ip"] = p.second->clientIp; - if (p.second->clientId) + const std::optional& clientId = p.second->clientId; + if (clientId) { - session["client_id"] = *p.second->clientId; + session["client_id"] = *clientId; } sessions.emplace_back(std::move(session)); } diff --git a/redfish-core/include/query.hpp b/redfish-core/include/query.hpp index 78de6cae06..5962093bfc 100644 --- a/redfish-core/include/query.hpp +++ b/redfish-core/include/query.hpp @@ -135,7 +135,7 @@ inline bool handleIfMatch(crow::App& app, const crow::Request& req, std::optional queryOpt = query_param::parseParameters(req.url().params(), asyncResp->res); - if (queryOpt == std::nullopt) + if (!queryOpt) { return false; } diff --git a/redfish-core/include/utils/json_utils.hpp b/redfish-core/include/utils/json_utils.hpp index 3d3ae8d4bd..5b4ad9a545 100644 --- a/redfish-core/include/utils/json_utils.hpp +++ b/redfish-core/include/utils/json_utils.hpp @@ -552,7 +552,7 @@ bool readJsonPatch(const crow::Request& req, crow::Response& res, std::string_view key, UnpackTypes&&... in) { std::optional jsonRequest = readJsonPatchHelper(req, res); - if (jsonRequest == std::nullopt) + if (!jsonRequest) { return false; } diff --git a/redfish-core/lib/account_service.hpp b/redfish-core/lib/account_service.hpp index 173de73e3e..9f2748ddc1 100644 --- a/redfish-core/lib/account_service.hpp +++ b/redfish-core/lib/account_service.hpp @@ -1806,7 +1806,7 @@ inline void processAfterCreateUser( inline void processAfterGetAllGroups( const std::shared_ptr& asyncResp, const std::string& username, const std::string& password, - const std::optional& roleId, std::optional enabled, + const std::string& roleId, bool enabled, std::optional> accountTypes, const std::vector& allGroupsList) { @@ -1871,7 +1871,6 @@ inline void processAfterGetAllGroups( messages::internalError(asyncResp->res); return; } - crow::connections::systemBus->async_method_call( [asyncResp, username, password](const boost::system::error_code& ec2, sdbusplus::message_t& m) { @@ -1879,7 +1878,7 @@ inline void processAfterGetAllGroups( }, "xyz.openbmc_project.User.Manager", "/xyz/openbmc_project/user", "xyz.openbmc_project.User.Manager", "CreateUser", username, userGroups, - *roleId, *enabled); + roleId, enabled); } inline void handleAccountCollectionPost( @@ -1892,24 +1891,28 @@ inline void handleAccountCollectionPost( } std::string username; std::string password; - std::optional roleId("User"); - std::optional enabled = true; + std::optional roleIdJson; + std::optional enabledJson; std::optional> accountTypes; - if (!json_util::readJsonPatch( - req, asyncResp->res, "UserName", username, "Password", password, - "RoleId", roleId, "Enabled", enabled, "AccountTypes", accountTypes)) + if (!json_util::readJsonPatch(req, asyncResp->res, "UserName", username, + "Password", password, "RoleId", roleIdJson, + "Enabled", enabledJson, "AccountTypes", + accountTypes)) { return; } - std::string priv = getPrivilegeFromRoleId(*roleId); + std::string roleId = roleIdJson.value_or("User"); + std::string priv = getPrivilegeFromRoleId(roleId); if (priv.empty()) { - messages::propertyValueNotInList(asyncResp->res, *roleId, "RoleId"); + messages::propertyValueNotInList(asyncResp->res, roleId, "RoleId"); return; } roleId = priv; + bool enabled = enabledJson.value_or(true); + // Reading AllGroups property sdbusplus::asio::getProperty>( *crow::connections::systemBus, "xyz.openbmc_project.User.Manager", diff --git a/redfish-core/lib/ethernet.hpp b/redfish-core/lib/ethernet.hpp index 7783e7cfa4..57fe24c431 100644 --- a/redfish-core/lib/ethernet.hpp +++ b/redfish-core/lib/ethernet.hpp @@ -1295,34 +1295,27 @@ inline void // not explicitly provided are assumed to be unmodified from the // current state of the interface. Merge existing state into the // current request. - const std::string* addr = nullptr; - const std::string* gw = nullptr; - uint8_t prefixLength = 0; - bool errorInEntry = false; if (address) { - if (ip_util::ipv4VerifyIpAndGetBitcount(*address)) - { - addr = &(*address); - } - else + if (!ip_util::ipv4VerifyIpAndGetBitcount(*address)) { messages::propertyValueFormatError(asyncResp->res, *address, pathString + "/Address"); - errorInEntry = true; + return; } } else if (nicIpEntry != ipv4Data.cend()) { - addr = &(nicIpEntry->address); + address = (nicIpEntry->address); } else { messages::propertyMissing(asyncResp->res, pathString + "/Address"); - errorInEntry = true; + return; } + uint8_t prefixLength = 0; if (subnetMask) { if (!ip_util::ipv4VerifyIpAndGetBitcount(*subnetMask, @@ -1331,7 +1324,7 @@ inline void messages::propertyValueFormatError( asyncResp->res, *subnetMask, pathString + "/SubnetMask"); - errorInEntry = true; + return; } } else if (nicIpEntry != ipv4Data.cend()) @@ -1342,50 +1335,41 @@ inline void messages::propertyValueFormatError( asyncResp->res, nicIpEntry->netmask, pathString + "/SubnetMask"); - errorInEntry = true; + return; } } else { messages::propertyMissing(asyncResp->res, pathString + "/SubnetMask"); - errorInEntry = true; + return; } if (gateway) { - if (ip_util::ipv4VerifyIpAndGetBitcount(*gateway)) - { - gw = &(*gateway); - } - else + if (!ip_util::ipv4VerifyIpAndGetBitcount(*gateway)) { messages::propertyValueFormatError(asyncResp->res, *gateway, pathString + "/Gateway"); - errorInEntry = true; + return; } } else if (nicIpEntry != ipv4Data.cend()) { - gw = &nicIpEntry->gateway; + gateway = nicIpEntry->gateway; } else { messages::propertyMissing(asyncResp->res, pathString + "/Gateway"); - errorInEntry = true; - } - - if (errorInEntry) - { return; } if (nicIpEntry != ipv4Data.cend()) { deleteAndCreateIPAddress(IpVersion::IpV4, ifaceId, - nicIpEntry->id, prefixLength, *gw, - *addr, asyncResp); + nicIpEntry->id, prefixLength, *gateway, + *address, asyncResp); nicIpEntry = getNextStaticIpEntry(++nicIpEntry, ipv4Data.cend()); } diff --git a/redfish-core/lib/virtual_media.hpp b/redfish-core/lib/virtual_media.hpp index e425cc188d..9ef45e719a 100644 --- a/redfish-core/lib/virtual_media.hpp +++ b/redfish-core/lib/virtual_media.hpp @@ -391,7 +391,7 @@ inline std::optional inline std::optional getTransferProtocolFromParam( const std::optional& transferProtocolType) { - if (transferProtocolType == std::nullopt) + if (!transferProtocolType) { return {}; } @@ -670,7 +670,7 @@ inline void validateParams(const std::shared_ptr& asyncResp, } // optional param inserted must be true - if ((actionParams.inserted != std::nullopt) && !*actionParams.inserted) + if (actionParams.inserted && !*actionParams.inserted) { BMCWEB_LOG_ERROR( "Request action optional parameter Inserted must be true."); @@ -682,7 +682,7 @@ inline void validateParams(const std::shared_ptr& asyncResp, } // optional param transferMethod must be stream - if ((actionParams.transferMethod != std::nullopt) && + if (actionParams.transferMethod && (*actionParams.transferMethod != "Stream")) { BMCWEB_LOG_ERROR("Request action optional parameter " @@ -708,7 +708,8 @@ inline void validateParams(const std::shared_ptr& asyncResp, getTransferProtocolFromParam(actionParams.transferProtocolType); // ImageUrl does not contain valid protocol type - if (*uriTransferProtocolType == TransferProtocol::invalid) + if (uriTransferProtocolType && + *uriTransferProtocolType == TransferProtocol::invalid) { BMCWEB_LOG_ERROR("Request action parameter ImageUrl must " "contain specified protocol type from list: " @@ -720,21 +721,21 @@ inline void validateParams(const std::shared_ptr& asyncResp, } // transferProtocolType should contain value from list - if (*paramTransferProtocolType == TransferProtocol::invalid) + if (paramTransferProtocolType && + *paramTransferProtocolType == TransferProtocol::invalid) { BMCWEB_LOG_ERROR("Request action parameter TransferProtocolType " "must be provided with value from list: " "(CIFS, HTTPS)."); - messages::propertyValueNotInList(asyncResp->res, - *actionParams.transferProtocolType, - "TransferProtocolType"); + messages::propertyValueNotInList( + asyncResp->res, actionParams.transferProtocolType.value_or(""), + "TransferProtocolType"); return; } // valid transfer protocol not provided either with URI nor param - if ((uriTransferProtocolType == std::nullopt) && - (paramTransferProtocolType == std::nullopt)) + if (!uriTransferProtocolType && !paramTransferProtocolType) { BMCWEB_LOG_ERROR("Request action parameter ImageUrl must " "contain specified protocol type or param " @@ -746,8 +747,7 @@ inline void validateParams(const std::shared_ptr& asyncResp, } // valid transfer protocol provided both with URI and param - if ((paramTransferProtocolType != std::nullopt) && - (uriTransferProtocolType != std::nullopt)) + if (paramTransferProtocolType && uriTransferProtocolType) { // check if protocol is the same for URI and param if (*paramTransferProtocolType != *uriTransferProtocolType) @@ -758,15 +758,20 @@ inline void validateParams(const std::shared_ptr& asyncResp, "provided with param imageUrl."); messages::actionParameterValueTypeError( - asyncResp->res, *actionParams.transferProtocolType, + asyncResp->res, actionParams.transferProtocolType.value_or(""), "TransferProtocolType", "InsertMedia"); return; } } + if (!paramTransferProtocolType) + { + messages::internalError(asyncResp->res); + return; + } // validation passed, add protocol to URI if needed - if (uriTransferProtocolType == std::nullopt) + if (!uriTransferProtocolType) { actionParams.imageUrl = getUriWithTransferProtocol( *actionParams.imageUrl, *paramTransferProtocolType); @@ -783,7 +788,7 @@ inline void validateParams(const std::shared_ptr& asyncResp, } doMountVmLegacy(asyncResp, service, resName, *actionParams.imageUrl, - !(*actionParams.writeProtected), + !(actionParams.writeProtected.value_or(false)), std::move(*actionParams.userName), std::move(*actionParams.password)); } diff --git a/test/http/verb_test.cpp b/test/http/verb_test.cpp index aff6ec7405..698e39cbd1 100644 --- a/test/http/verb_test.cpp +++ b/test/http/verb_test.cpp @@ -27,8 +27,7 @@ TEST(BoostToHttpVerb, ValidCase) { HttpVerb httpVerb = static_cast(verbIndex); std::optional verb = httpVerbFromBoost(verbMap[httpVerb]); - ASSERT_TRUE(verb.has_value()); - EXPECT_EQ(*verb, httpVerb); + EXPECT_EQ(verb, httpVerb); } } diff --git a/test/redfish-core/include/utils/query_param_test.cpp b/test/redfish-core/include/utils/query_param_test.cpp index 7aa4ad519b..c5ae21f1d2 100644 --- a/test/redfish-core/include/utils/query_param_test.cpp +++ b/test/redfish-core/include/utils/query_param_test.cpp @@ -358,9 +358,12 @@ TEST(RecursiveSelect, ReservedPropertiesAreSelected) ASSERT_TRUE(ret); crow::Response res; std::optional query = parseParameters(ret->params(), res); - - ASSERT_NE(query, std::nullopt); - recursiveSelect(root, query->selectTrie.root); + ASSERT_TRUE(query); + if (!query) + { + return; + } + recursiveSelect(root, query.value().selectTrie.root); EXPECT_EQ(root, expected); } @@ -508,10 +511,18 @@ TEST(QueryParams, ParseParametersOnly) { auto ret = boost::urls::parse_relative_ref("/redfish/v1?only"); ASSERT_TRUE(ret); + if (!ret) + { + return; + } crow::Response res; std::optional query = parseParameters(ret->params(), res); - ASSERT_TRUE(query != std::nullopt); + ASSERT_TRUE(query); + if (!query) + { + return; + } EXPECT_TRUE(query->isOnly); } @@ -519,14 +530,22 @@ TEST(QueryParams, ParseParametersExpand) { auto ret = boost::urls::parse_relative_ref("/redfish/v1?$expand=*"); ASSERT_TRUE(ret); + if (!ret) + { + return; + } crow::Response res; std::optional query = parseParameters(ret->params(), res); if constexpr (bmcwebInsecureEnableQueryParams) { - ASSERT_NE(query, std::nullopt); - EXPECT_TRUE(query->expandType == + ASSERT_TRUE(query); + if (!query) + { + return; + } + EXPECT_TRUE(query.value().expandType == redfish::query_param::ExpandType::Both); } else @@ -539,12 +558,20 @@ TEST(QueryParams, ParseParametersTop) { auto ret = boost::urls::parse_relative_ref("/redfish/v1?$top=1"); ASSERT_TRUE(ret); + if (!ret) + { + return; + } crow::Response res; std::optional query = parseParameters(ret->params(), res); - ASSERT_TRUE(query != std::nullopt); - EXPECT_EQ(query->top, 1); + ASSERT_TRUE(query); + if (!query) + { + return; + } + EXPECT_EQ(query.value().top, 1); } TEST(QueryParams, ParseParametersTopOutOfRangeNegative) @@ -562,7 +589,10 @@ TEST(QueryParams, ParseParametersTopOutOfRangePositive) { auto ret = boost::urls::parse_relative_ref("/redfish/v1?$top=1001"); ASSERT_TRUE(ret); - + if (!ret) + { + return; + } crow::Response res; std::optional query = parseParameters(ret->params(), res); @@ -577,8 +607,12 @@ TEST(QueryParams, ParseParametersSkip) crow::Response res; std::optional query = parseParameters(ret->params(), res); - ASSERT_TRUE(query != std::nullopt); - EXPECT_EQ(query->skip, 1); + ASSERT_TRUE(query); + if (!query) + { + return; + } + EXPECT_EQ(query.value().skip, 1); } TEST(QueryParams, ParseParametersSkipOutOfRange) {