Manage Request with shared_ptr
This is an attempt to solve a class of use-after-move bugs on the
Request objects which have popped up several times. This more clearly
identifies code which owns the Request objects and has a need to keep it
alive. Currently it's just the `Connection` (or `HTTP2Connection`)
(which needs to access Request headers while sending the response), and
the `validatePrivilege()` function (which needs to temporarily own the
Request while doing an asynchronous D-Bus call). Route handlers are
provided a non-owning `Request&` for immediate use and required to not
hold the `Request&` for future use.
Tested: Redfish validator passes (with a few unrelated fails).
Redfish URLs are sent to a browser as HTML instead of raw JSON.
Change-Id: Id581fda90b6bceddd08a5dc7ff0a04b91e7394bf
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/http/app.hpp b/http/app.hpp
index eea1305..29cb4de 100644
--- a/http/app.hpp
+++ b/http/app.hpp
@@ -54,14 +54,14 @@
App& operator=(const App&&) = delete;
template <typename Adaptor>
- void handleUpgrade(Request& req,
+ void handleUpgrade(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
Adaptor&& adaptor)
{
router.handleUpgrade(req, asyncResp, std::forward<Adaptor>(adaptor));
}
- void handle(Request& req,
+ void handle(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
{
router.handle(req, asyncResp);
diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp
index 2c60c12..efe66b6 100644
--- a/http/http2_connection.hpp
+++ b/http/http2_connection.hpp
@@ -37,7 +37,7 @@
struct Http2StreamData
{
- Request req;
+ std::shared_ptr<Request> req = std::make_shared<Request>();
std::optional<bmcweb::HttpBody::reader> reqReader;
Response res;
std::optional<bmcweb::HttpBody::writer> writer;
@@ -170,7 +170,7 @@
Http2StreamData& stream = it->second;
Response& res = stream.res;
res = std::move(completedRes);
- crow::Request& thisReq = stream.req;
+ crow::Request& thisReq = *stream.req;
completeResponseFields(thisReq, res);
res.addHeader(boost::beast::http::field::date, getCachedDateStr());
@@ -243,7 +243,7 @@
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
}
}
- crow::Request& thisReq = it->second.req;
+ crow::Request& thisReq = *it->second.req;
thisReq.ioService = static_cast<decltype(thisReq.ioService)>(
&adaptor.get_executor().context());
BMCWEB_LOG_DEBUG("Handling {} \"{}\"", logPtr(&thisReq),
@@ -285,7 +285,7 @@
{
asyncResp->res.setExpectedHash(expected);
}
- handler->handle(thisReq, asyncResp);
+ handler->handle(it->second.req, asyncResp);
}
return 0;
}
@@ -306,8 +306,8 @@
if (!reqReader)
{
reqReader.emplace(
- bmcweb::HttpBody::reader(thisStream->second.req.req.base(),
- thisStream->second.req.req.body()));
+ bmcweb::HttpBody::reader(thisStream->second.req->req.base(),
+ thisStream->second.req->req.body()));
}
boost::beast::error_code ec;
reqReader->put(boost::asio::const_buffer(data, len), ec);
@@ -425,7 +425,7 @@
return -1;
}
- crow::Request& thisReq = thisStream->second.req;
+ crow::Request& thisReq = *thisStream->second.req;
if (nameSv == ":path")
{
@@ -493,7 +493,7 @@
Http2StreamData& stream = streams[frame.hd.stream_id];
// http2 is by definition always tls
- stream.req.isSecure = true;
+ stream.req->isSecure = true;
}
return 0;
}
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index e02c518..29e876d 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -30,6 +30,7 @@
#include <atomic>
#include <chrono>
+#include <memory>
#include <vector>
namespace crow
@@ -232,7 +233,7 @@
{
return;
}
- req = crow::Request(parser->release(), reqEc);
+ req = std::make_shared<crow::Request>(parser->release(), reqEc);
if (reqEc)
{
BMCWEB_LOG_DEBUG("Request failed to construct{}", reqEc.message());
@@ -240,15 +241,15 @@
completeRequest(res);
return;
}
- req.session = userSession;
+ req->session = userSession;
// Fetch the client IP address
- req.ipAddress = ip;
+ req->ipAddress = ip;
// Check for HTTP version 1.1.
- if (req.version() == 11)
+ if (req->version() == 11)
{
- if (req.getHeaderValue(boost::beast::http::field::host).empty())
+ if (req->getHeaderValue(boost::beast::http::field::host).empty())
{
res.result(boost::beast::http::status::bad_request);
completeRequest(res);
@@ -257,11 +258,11 @@
}
BMCWEB_LOG_INFO("Request: {} HTTP/{}.{} {} {} {}", logPtr(this),
- req.version() / 10, req.version() % 10,
- req.methodString(), req.target(),
- req.ipAddress.to_string());
+ req->version() / 10, req->version() % 10,
+ req->methodString(), req->target(),
+ req->ipAddress.to_string());
- req.ioService = static_cast<decltype(req.ioService)>(
+ req->ioService = static_cast<decltype(req->ioService)>(
&adaptor.get_executor().context());
if (res.completed)
@@ -269,19 +270,19 @@
completeRequest(res);
return;
}
- keepAlive = req.keepAlive();
+ keepAlive = req->keepAlive();
if constexpr (!std::is_same_v<Adaptor, boost::beast::test::stream>)
{
#ifndef BMCWEB_INSECURE_DISABLE_AUTHX
- if (!crow::authentication::isOnAllowlist(req.url().path(),
- req.method()) &&
- req.session == nullptr)
+ if (!crow::authentication::isOnAllowlist(req->url().path(),
+ req->method()) &&
+ req->session == nullptr)
{
BMCWEB_LOG_WARNING("Authentication failed");
forward_unauthorized::sendUnauthorized(
- req.url().encoded_path(),
- req.getHeaderValue("X-Requested-With"),
- req.getHeaderValue("Accept"), res);
+ req->url().encoded_path(),
+ req->getHeaderValue("X-Requested-With"),
+ req->getHeaderValue("Accept"), res);
completeRequest(res);
return;
}
@@ -294,11 +295,11 @@
self->completeRequest(thisRes);
});
bool isSse =
- isContentTypeAllowed(req.getHeaderValue("Accept"),
+ isContentTypeAllowed(req->getHeaderValue("Accept"),
http_helpers::ContentType::EventStream, false);
std::string_view upgradeType(
- req.getHeaderValue(boost::beast::http::field::upgrade));
- if ((req.isUpgrade() &&
+ req->getHeaderValue(boost::beast::http::field::upgrade));
+ if ((req->isUpgrade() &&
bmcweb::asciiIEquals(upgradeType, "websocket")) ||
isSse)
{
@@ -320,7 +321,7 @@
return;
}
std::string_view expected =
- req.getHeaderValue(boost::beast::http::field::if_none_match);
+ req->getHeaderValue(boost::beast::http::field::if_none_match);
if (!expected.empty())
{
res.setExpectedHash(expected);
@@ -371,7 +372,7 @@
res = std::move(thisRes);
res.keepAlive(keepAlive);
- completeResponseFields(req, res);
+ completeResponseFields(*req, res);
res.addHeader(boost::beast::http::field::date, getCachedDateStr());
doWrite();
@@ -515,7 +516,7 @@
}
std::string_view expect =
- req.getHeaderValue(boost::beast::http::field::expect);
+ parser->get()[boost::beast::http::field::expect];
if (bmcweb::asciiIEquals(expect, "100-continue"))
{
res.result(boost::beast::http::status::continue_);
@@ -644,8 +645,7 @@
userSession = nullptr;
- // Destroy the Request via the std::optional
- req.clear();
+ req->clear();
doReadHeaders();
}
@@ -732,7 +732,7 @@
boost::beast::flat_static_buffer<8192> buffer;
- crow::Request req;
+ std::shared_ptr<crow::Request> req;
crow::Response res;
std::shared_ptr<persistent_data::UserSession> userSession;
diff --git a/http/http_request.hpp b/http/http_request.hpp
index d1a6ac0..d2a1e25 100644
--- a/http/http_request.hpp
+++ b/http/http_request.hpp
@@ -18,7 +18,8 @@
struct Request
{
- boost::beast::http::request<bmcweb::HttpBody> req;
+ using Body = boost::beast::http::request<bmcweb::HttpBody>;
+ Body req;
private:
boost::urls::url urlBase;
@@ -32,9 +33,7 @@
std::shared_ptr<persistent_data::UserSession> session;
std::string userRole;
- Request(boost::beast::http::request<bmcweb::HttpBody> reqIn,
- std::error_code& ec) :
- req(std::move(reqIn))
+ Request(Body reqIn, std::error_code& ec) : req(std::move(reqIn))
{
if (!setUrlInfo())
{
diff --git a/http/routing.hpp b/http/routing.hpp
index fa6db58..7e481a0 100644
--- a/http/routing.hpp
+++ b/http/routing.hpp
@@ -491,7 +491,7 @@
return route;
}
- FindRouteResponse findRoute(Request& req) const
+ FindRouteResponse findRoute(const Request& req) const
{
FindRouteResponse findRoute;
@@ -529,11 +529,11 @@
}
template <typename Adaptor>
- void handleUpgrade(Request& req,
+ void handleUpgrade(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
Adaptor&& adaptor)
{
- std::optional<HttpVerb> verb = httpVerbFromBoost(req.method());
+ std::optional<HttpVerb> verb = httpVerbFromBoost(req->method());
if (!verb || static_cast<size_t>(*verb) >= perMethods.size())
{
asyncResp->res.result(boost::beast::http::status::not_found);
@@ -543,11 +543,12 @@
Trie& trie = perMethod.trie;
std::vector<BaseRule*>& rules = perMethod.rules;
- Trie::FindResult found = trie.find(req.url().encoded_path());
+ Trie::FindResult found = trie.find(req->url().encoded_path());
unsigned ruleIndex = found.ruleIndex;
if (ruleIndex == 0U)
{
- BMCWEB_LOG_DEBUG("Cannot match rules {}", req.url().encoded_path());
+ BMCWEB_LOG_DEBUG("Cannot match rules {}",
+ req->url().encoded_path());
asyncResp->res.result(boost::beast::http::status::not_found);
return;
}
@@ -563,7 +564,7 @@
{
BMCWEB_LOG_DEBUG(
"Rule found but method mismatch: {} with {}({}) / {}",
- req.url().encoded_path(), req.methodString(),
+ req->url().encoded_path(), req->methodString(),
static_cast<uint32_t>(*verb), methods);
asyncResp->res.result(boost::beast::http::status::not_found);
return;
@@ -574,25 +575,24 @@
// TODO(ed) This should be able to use std::bind_front, but it doesn't
// appear to work with the std::move on adaptor.
- validatePrivilege(
- req, asyncResp, rule,
- [&rule, asyncResp, adaptor = std::forward<Adaptor>(adaptor)](
- Request& thisReq) mutable {
- rule.handleUpgrade(thisReq, asyncResp, std::move(adaptor));
+ validatePrivilege(req, asyncResp, rule,
+ [req, &rule, asyncResp,
+ adaptor = std::forward<Adaptor>(adaptor)]() mutable {
+ rule.handleUpgrade(*req, asyncResp, std::move(adaptor));
});
}
- void handle(Request& req,
+ void handle(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
{
- std::optional<HttpVerb> verb = httpVerbFromBoost(req.method());
+ std::optional<HttpVerb> verb = httpVerbFromBoost(req->method());
if (!verb || static_cast<size_t>(*verb) >= perMethods.size())
{
asyncResp->res.result(boost::beast::http::status::not_found);
return;
}
- FindRouteResponse foundRoute = findRoute(req);
+ FindRouteResponse foundRoute = findRoute(*req);
if (foundRoute.route.rule == nullptr)
{
@@ -600,13 +600,13 @@
// route
if (foundRoute.allowHeader.empty())
{
- foundRoute.route = findRouteByIndex(req.url().encoded_path(),
+ foundRoute.route = findRouteByIndex(req->url().encoded_path(),
notFoundIndex);
}
else
{
// See if we have a method not allowed (405) handler
- foundRoute.route = findRouteByIndex(req.url().encoded_path(),
+ foundRoute.route = findRouteByIndex(req->url().encoded_path(),
methodNotAllowedIndex);
}
}
@@ -640,14 +640,15 @@
BMCWEB_LOG_DEBUG("Matched rule '{}' {} / {}", rule.rule,
static_cast<uint32_t>(*verb), rule.getMethods());
- if (req.session == nullptr)
+ if (req->session == nullptr)
{
- rule.handle(req, asyncResp, params);
+ rule.handle(*req, asyncResp, params);
return;
}
- validatePrivilege(req, asyncResp, rule,
- [&rule, asyncResp, params](Request& thisReq) mutable {
- rule.handle(thisReq, asyncResp, params);
+ validatePrivilege(
+ req, asyncResp, rule,
+ [req, asyncResp, &rule, params = std::move(params)]() {
+ rule.handle(*req, asyncResp, params);
});
}
diff --git a/include/dbus_privileges.hpp b/include/dbus_privileges.hpp
index a58f9be..4cafe52 100644
--- a/include/dbus_privileges.hpp
+++ b/include/dbus_privileges.hpp
@@ -106,19 +106,18 @@
return true;
}
-template <typename CallbackFn>
-void afterGetUserInfo(Request& req,
- const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
- BaseRule& rule, CallbackFn callback,
- const boost::system::error_code& ec,
- const dbus::utility::DBusPropertiesMap& userInfoMap)
+inline bool
+ afterGetUserInfo(Request& req,
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ BaseRule& rule, const boost::system::error_code& ec,
+ const dbus::utility::DBusPropertiesMap& userInfoMap)
{
if (ec)
{
BMCWEB_LOG_ERROR("GetUserInfo failed...");
asyncResp->res.result(
boost::beast::http::status::internal_server_error);
- return;
+ return false;
}
if (!populateUserInfo(req, userInfoMap))
@@ -126,7 +125,7 @@
BMCWEB_LOG_ERROR("Failed to populate user information");
asyncResp->res.result(
boost::beast::http::status::internal_server_error);
- return;
+ return false;
}
if (!isUserPrivileged(req, asyncResp, rule))
@@ -134,28 +133,30 @@
// User is not privileged
BMCWEB_LOG_ERROR("Insufficient Privilege");
asyncResp->res.result(boost::beast::http::status::forbidden);
- return;
+ return false;
}
- callback(req);
+
+ return true;
}
template <typename CallbackFn>
-void validatePrivilege(Request& req,
+void validatePrivilege(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
BaseRule& rule, CallbackFn&& callback)
{
- if (req.session == nullptr)
+ if (req->session == nullptr)
{
return;
}
- std::string username = req.session->username;
+ std::string username = req->session->username;
crow::connections::systemBus->async_method_call(
- [req{std::move(req)}, asyncResp, &rule,
- callback = std::forward<CallbackFn>(callback)](
+ [req, asyncResp, &rule, callback = std::forward<CallbackFn>(callback)](
const boost::system::error_code& ec,
const dbus::utility::DBusPropertiesMap& userInfoMap) mutable {
- afterGetUserInfo(req, asyncResp, rule,
- std::forward<CallbackFn>(callback), ec, userInfoMap);
+ if (afterGetUserInfo(*req, asyncResp, rule, ec, userInfoMap))
+ {
+ callback();
+ }
},
"xyz.openbmc_project.User.Manager", "/xyz/openbmc_project/user",
"xyz.openbmc_project.User.Manager", "GetUserInfo", username);
diff --git a/redfish-core/include/query.hpp b/redfish-core/include/query.hpp
index 063f1a8..2d280cb 100644
--- a/redfish-core/include/query.hpp
+++ b/redfish-core/include/query.hpp
@@ -16,7 +16,6 @@
#include <functional>
#include <memory>
-#include <new>
#include <optional>
#include <string>
#include <string_view>
@@ -31,11 +30,10 @@
namespace redfish
{
-inline void
- afterIfMatchRequest(crow::App& app,
- const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
- crow::Request& req, const std::string& ifMatchHeader,
- const crow::Response& resIn)
+inline void afterIfMatchRequest(
+ crow::App& app, const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ const std::shared_ptr<crow::Request>& req, const std::string& ifMatchHeader,
+ const crow::Response& resIn)
{
std::string computedEtag = resIn.computeEtag();
BMCWEB_LOG_DEBUG("User provided if-match etag {} computed etag {}",
@@ -46,7 +44,7 @@
return;
}
// Restart the request without if-match
- req.clearHeader(boost::beast::http::field::if_match);
+ req->clearHeader(boost::beast::http::field::if_match);
BMCWEB_LOG_DEBUG("Restarting request");
app.handle(req, asyncResp);
}
@@ -84,8 +82,10 @@
boost::system::error_code ec;
// Try to GET the same resource
- crow::Request newReq(
- {boost::beast::http::verb::get, req.url().encoded_path(), 11}, ec);
+ auto getReq = std::make_shared<crow::Request>(
+ crow::Request::Body{boost::beast::http::verb::get,
+ req.url().encoded_path(), 11},
+ ec);
if (ec)
{
@@ -94,17 +94,21 @@
}
// New request has the same credentials as the old request
- newReq.session = req.session;
+ getReq->session = req.session;
// Construct a new response object to fill in, and check the hash of before
// we modify the Resource.
std::shared_ptr<bmcweb::AsyncResp> getReqAsyncResp =
std::make_shared<bmcweb::AsyncResp>();
+ // Ideally we would have a shared_ptr to the original Request which we could
+ // modify to remove the If-Match and restart it. But instead we have to make
+ // a full copy to restart it.
getReqAsyncResp->res.setCompleteRequestHandler(std::bind_front(
- afterIfMatchRequest, std::ref(app), asyncResp, req, ifMatch));
+ afterIfMatchRequest, std::ref(app), asyncResp,
+ std::make_shared<crow::Request>(req), std::move(ifMatch)));
- app.handle(newReq, getReqAsyncResp);
+ app.handle(getReq, getReqAsyncResp);
return false;
}
diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp
index eadb66e..3b04e21 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -503,7 +503,8 @@
// TODO(Ed) copy request headers?
// newReq.session = req.session;
std::error_code ec;
- crow::Request newReq({boost::beast::http::verb::get, *url, 11}, ec);
+ auto newReq = std::make_shared<crow::Request>(
+ crow::Request::Body{boost::beast::http::verb::get, *url, 11}, ec);
if (ec)
{
messages::internalError(res);
@@ -854,8 +855,10 @@
const std::string subQuery = node.uri + *queryStr;
BMCWEB_LOG_DEBUG("URL of subquery: {}", subQuery);
std::error_code ec;
- crow::Request newReq({boost::beast::http::verb::get, subQuery, 11},
- ec);
+ auto newReq = std::make_shared<crow::Request>(
+ crow::Request::Body{boost::beast::http::verb::get, subQuery,
+ 11},
+ ec);
if (ec)
{
messages::internalError(finalRes->res);
diff --git a/test/http/http2_connection_test.cpp b/test/http/http2_connection_test.cpp
index 34dc7cd..0385ed6 100644
--- a/test/http/http2_connection_test.cpp
+++ b/test/http/http2_connection_test.cpp
@@ -38,16 +38,17 @@
struct FakeHandler
{
bool called = false;
- void handle(Request& req,
+ void handle(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
{
called = true;
- EXPECT_EQ(req.url().buffer(), "/redfish/v1/");
- EXPECT_EQ(req.methodString(), "GET");
- EXPECT_EQ(req.getHeaderValue(boost::beast::http::field::user_agent),
+ EXPECT_EQ(req->url().buffer(), "/redfish/v1/");
+ EXPECT_EQ(req->methodString(), "GET");
+ EXPECT_EQ(req->getHeaderValue(boost::beast::http::field::user_agent),
"curl/8.5.0");
- EXPECT_EQ(req.getHeaderValue(boost::beast::http::field::accept), "*/*");
- EXPECT_EQ(req.getHeaderValue(":authority"), "localhost:18080");
+ EXPECT_EQ(req->getHeaderValue(boost::beast::http::field::accept),
+ "*/*");
+ EXPECT_EQ(req->getHeaderValue(":authority"), "localhost:18080");
asyncResp->res.write("StringOutput");
}
};
diff --git a/test/http/http_connection_test.cpp b/test/http/http_connection_test.cpp
index caf50c8..393e209 100644
--- a/test/http/http_connection_test.cpp
+++ b/test/http/http_connection_test.cpp
@@ -23,7 +23,7 @@
struct FakeHandler
{
static void
- handleUpgrade(Request& /*req*/,
+ handleUpgrade(const std::shared_ptr<Request>& /*req*/,
const std::shared_ptr<bmcweb::AsyncResp>& /*asyncResp*/,
boost::beast::test::stream&& /*adaptor*/)
{
@@ -31,16 +31,16 @@
EXPECT_FALSE(true);
}
- void handle(Request& req,
+ void handle(const std::shared_ptr<Request>& req,
const std::shared_ptr<bmcweb::AsyncResp>& /*asyncResp*/)
{
- EXPECT_EQ(req.method(), boost::beast::http::verb::get);
- EXPECT_EQ(req.target(), "/");
- EXPECT_EQ(req.getHeaderValue(boost::beast::http::field::host),
+ EXPECT_EQ(req->method(), boost::beast::http::verb::get);
+ EXPECT_EQ(req->target(), "/");
+ EXPECT_EQ(req->getHeaderValue(boost::beast::http::field::host),
"openbmc_project.xyz");
- EXPECT_FALSE(req.keepAlive());
- EXPECT_EQ(req.version(), 11);
- EXPECT_EQ(req.body(), "");
+ EXPECT_FALSE(req->keepAlive());
+ EXPECT_EQ(req->version(), 11);
+ EXPECT_EQ(req->body(), "");
called = true;
}
diff --git a/test/http/router_test.cpp b/test/http/router_test.cpp
index f233317..2372541 100644
--- a/test/http/router_test.cpp
+++ b/test/http/router_test.cpp
@@ -87,7 +87,8 @@
{
constexpr std::string_view url = "/foo/bar";
- Request req{{boost::beast::http::verb::get, url, 11}, ec};
+ auto req = std::make_shared<Request>(
+ Request::Body{boost::beast::http::verb::get, url, 11}, ec);
std::shared_ptr<bmcweb::AsyncResp> asyncResp =
std::make_shared<bmcweb::AsyncResp>();
@@ -112,7 +113,8 @@
constexpr std::string_view url = "/foo/bar";
- Request req{{boost::beast::http::verb::get, url, 11}, ec};
+ auto req = std::make_shared<Request>(
+ Request::Body{boost::beast::http::verb::get, url, 11}, ec);
router.newRuleTagged<getParameterTag(url)>("/foo/<path>")
.notFound()(nullCallback);
@@ -142,7 +144,8 @@
constexpr std::string_view url = "/foo/bar";
- Request req{{boost::beast::http::verb::patch, url, 11}, ec};
+ auto req = std::make_shared<Request>(
+ Request::Body{boost::beast::http::verb::patch, url, 11}, ec);
router.newRuleTagged<getParameterTag(url)>(std::string(url))
.methods(boost::beast::http::verb::get)(nullCallback);