Skip to content

Commit

Permalink
Fix bugprone-unchecked-optional-access findings
Browse files Browse the repository at this point in the history 
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] llvm/llvm-project#55530

Tested: Redfish service validator passes.

Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3
Signed-off-by: Ed Tanous <edtanous@google.com>
  • Loading branch information
@edtanous
edtanous committed Aug 7, 2023
1 parent 1ccf70f commit e01d0c3
Show file tree
Hide file tree
Showing 12 changed files with 156 additions and 106 deletions.
4 changes: 2 additions & 2 deletions http/http2_connection.hpp
View file
Expand Up @@ -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)
{
Expand Down
21 changes: 13 additions & 8 deletions http/http_client.hpp
View file
Expand Up @@ -125,6 +125,7 @@ struct PendingRequest
{}
};

namespace http = boost::beast::http;
class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
{
private:
Expand All @@ -137,10 +138,9 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
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;

Expand Down Expand Up @@ -329,9 +329,10 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
{
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()));
Expand All @@ -340,14 +341,14 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
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()));
}
Expand Down Expand Up @@ -375,6 +376,10 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
}
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();
Expand Down
23 changes: 21 additions & 2 deletions http/http_connection.hpp
View file
Expand Up @@ -94,6 +94,10 @@ class Connection :
// don't require auth
if (preverified)
{
if (!req)
{
return false;
}
mtlsSession = verifyMtlsUser(req->ipAddress, ctx);
if (mtlsSession)
{
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -363,6 +371,10 @@ class Connection :
{
return;
}
if (!req)
{
return;
}
req->ipAddress = ip;
}

Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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())](
Expand Down
46 changes: 23 additions & 23 deletions http/http_response.hpp
View file
Expand Up @@ -23,26 +23,26 @@ struct Response
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)),
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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<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;
Expand All @@ -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",
Expand All @@ -174,21 +173,22 @@ 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;
}

void write(std::string_view bodyPart)
{
stringResponse->body() += std::string(bodyPart);
stringResponse.body() += std::string(bodyPart);
}

std::string computeEtag() const
Expand Down
5 changes: 3 additions & 2 deletions include/persistent_data.hpp
View file
Expand Up @@ -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<std::string>& clientId = p.second->clientId;
if (clientId)
{
session["client_id"] = *p.second->clientId;
session["client_id"] = *clientId;
}
sessions.emplace_back(std::move(session));
}
Expand Down
2 changes: 1 addition & 1 deletion redfish-core/include/query.hpp
View file
Expand Up @@ -135,7 +135,7 @@ inline bool handleIfMatch(crow::App& app, const crow::Request& req,

std::optional<query_param::Query> queryOpt =
query_param::parseParameters(req.url().params(), asyncResp->res);
if (queryOpt == std::nullopt)
if (!queryOpt)
{
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion redfish-core/include/utils/json_utils.hpp
View file
Expand Up @@ -552,7 +552,7 @@ bool readJsonPatch(const crow::Request& req, crow::Response& res,
std::string_view key, UnpackTypes&&... in)
{
std::optional<nlohmann::json> jsonRequest = readJsonPatchHelper(req, res);
if (jsonRequest == std::nullopt)
if (!jsonRequest)
{
return false;
}
Expand Down
23 changes: 13 additions & 10 deletions redfish-core/lib/account_service.hpp
View file
Expand Up @@ -1806,7 +1806,7 @@ inline void processAfterCreateUser(
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)
{
Expand Down Expand Up @@ -1871,15 +1871,14 @@ 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) {
processAfterCreateUser(asyncResp, username, password, ec2, m);
},
"xyz.openbmc_project.User.Manager", "/xyz/openbmc_project/user",
"xyz.openbmc_project.User.Manager", "CreateUser", username, userGroups,
*roleId, *enabled);
roleId, enabled);
}

inline void handleAccountCollectionPost(
Expand All @@ -1892,24 +1891,28 @@ inline void handleAccountCollectionPost(
}
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",
Expand Down

0 comments on commit e01d0c3

Please sign in to comment.