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 <edtanous@google.com>
diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp
index a63b234..27df36e 100644
--- a/http/http2_connection.hpp
+++ b/http/http2_connection.hpp
@@ -154,8 +154,8 @@
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 d82c566..71fb885 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -125,6 +125,7 @@
{}
};
+namespace http = boost::beast::http;
class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
{
private:
@@ -137,10 +138,9 @@
uint32_t connId;
// Data buffers
- boost::beast::http::request<boost::beast::http::string_body> req;
- std::optional<
- boost::beast::http::response_parser<boost::beast::http::string_body>>
- parser;
+ http::request<http::string_body> req;
+ using parser_type = http::response_parser<http::string_body>;
+ std::optional<parser_type> parser;
boost::beast::flat_static_buffer<httpReadBufferSize> buffer;
Response res;
@@ -329,9 +329,10 @@
{
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 @@
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 @@
}
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 b5d0d2e..ba4af3f 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -94,6 +94,10 @@
// don't require auth
if (preverified)
{
+ if (!req)
+ {
+ return false;
+ }
mtlsSession = verifyMtlsUser(req->ipAddress, ctx);
if (mtlsSession)
{
@@ -202,6 +206,10 @@
void handle()
{
std::error_code reqEc;
+ if (!parser)
+ {
+ return;
+ }
crow::Request& thisReq = req.emplace(parser->release(), reqEc);
if (reqEc)
{
@@ -363,6 +371,10 @@
{
return;
}
+ if (!req)
+ {
+ return;
+ }
req->ipAddress = ip;
}
@@ -389,7 +401,10 @@
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 @@
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 @@
{
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 c4f9366..cb07a83 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -23,26 +23,26 @@
using response_type =
boost::beast::http::response<boost::beast::http::string_body>;
- std::optional<response_type> 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 @@
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 @@
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 @@
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<uint64_t> pSize = stringResponse->payload_size();
+ boost::optional<uint64_t> 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 @@
}
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 @@
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 @@
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 5079a8c..048d987 100644
--- a/include/persistent_data.hpp
+++ b/include/persistent_data.hpp
@@ -234,9 +234,10 @@
session["username"] = p.second->username;
session["csrf_token"] = p.second->csrfToken;
session["client_ip"] = p.second->clientIp;
- if (p.second->clientId)
+ const std::optional<std::string>& 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 78de6ca..5962093 100644
--- a/redfish-core/include/query.hpp
+++ b/redfish-core/include/query.hpp
@@ -135,7 +135,7 @@
std::optional<query_param::Query> 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 3d3ae8d..5b4ad9a 100644
--- a/redfish-core/include/utils/json_utils.hpp
+++ b/redfish-core/include/utils/json_utils.hpp
@@ -552,7 +552,7 @@
std::string_view key, UnpackTypes&&... in)
{
std::optional<nlohmann::json> 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 173de73..9f2748d 100644
--- a/redfish-core/lib/account_service.hpp
+++ b/redfish-core/lib/account_service.hpp
@@ -1806,7 +1806,7 @@
inline void processAfterGetAllGroups(
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
const std::string& username, const std::string& password,
- const std::optional<std::string>& roleId, std::optional<bool> enabled,
+ const std::string& roleId, bool enabled,
std::optional<std::vector<std::string>> accountTypes,
const std::vector<std::string>& allGroupsList)
{
@@ -1871,7 +1871,6 @@
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 @@
},
"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 @@
}
std::string username;
std::string password;
- std::optional<std::string> roleId("User");
- std::optional<bool> enabled = true;
+ std::optional<std::string> roleIdJson;
+ std::optional<bool> enabledJson;
std::optional<std::vector<std::string>> 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<std::vector<std::string>>(
*crow::connections::systemBus, "xyz.openbmc_project.User.Manager",
diff --git a/redfish-core/lib/ethernet.hpp b/redfish-core/lib/ethernet.hpp
index 7783e7c..57fe24c 100644
--- a/redfish-core/lib/ethernet.hpp
+++ b/redfish-core/lib/ethernet.hpp
@@ -1295,34 +1295,27 @@
// 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 @@
messages::propertyValueFormatError(
asyncResp->res, *subnetMask,
pathString + "/SubnetMask");
- errorInEntry = true;
+ return;
}
}
else if (nicIpEntry != ipv4Data.cend())
@@ -1342,50 +1335,41 @@
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 e425cc1..9ef45e7 100644
--- a/redfish-core/lib/virtual_media.hpp
+++ b/redfish-core/lib/virtual_media.hpp
@@ -391,7 +391,7 @@
inline std::optional<TransferProtocol> getTransferProtocolFromParam(
const std::optional<std::string>& transferProtocolType)
{
- if (transferProtocolType == std::nullopt)
+ if (!transferProtocolType)
{
return {};
}
@@ -670,7 +670,7 @@
}
// 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 @@
}
// 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 @@
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 @@
}
// 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 @@
}
// 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 @@
"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 @@
}
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 aff6ec7..698e39c 100644
--- a/test/http/verb_test.cpp
+++ b/test/http/verb_test.cpp
@@ -27,8 +27,7 @@
{
HttpVerb httpVerb = static_cast<HttpVerb>(verbIndex);
std::optional<HttpVerb> 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 7aa4ad5..c5ae21f 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 @@
ASSERT_TRUE(ret);
crow::Response res;
std::optional<Query> 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 @@
{
auto ret = boost::urls::parse_relative_ref("/redfish/v1?only");
ASSERT_TRUE(ret);
+ if (!ret)
+ {
+ return;
+ }
crow::Response res;
std::optional<Query> query = parseParameters(ret->params(), res);
- ASSERT_TRUE(query != std::nullopt);
+ ASSERT_TRUE(query);
+ if (!query)
+ {
+ return;
+ }
EXPECT_TRUE(query->isOnly);
}
@@ -519,14 +530,22 @@
{
auto ret = boost::urls::parse_relative_ref("/redfish/v1?$expand=*");
ASSERT_TRUE(ret);
+ if (!ret)
+ {
+ return;
+ }
crow::Response res;
std::optional<Query> 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 @@
{
auto ret = boost::urls::parse_relative_ref("/redfish/v1?$top=1");
ASSERT_TRUE(ret);
+ if (!ret)
+ {
+ return;
+ }
crow::Response res;
std::optional<Query> 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 @@
{
auto ret = boost::urls::parse_relative_ref("/redfish/v1?$top=1001");
ASSERT_TRUE(ret);
-
+ if (!ret)
+ {
+ return;
+ }
crow::Response res;
std::optional<Query> query = parseParameters(ret->params(), res);
@@ -577,8 +607,12 @@
crow::Response res;
std::optional<Query> 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)
{