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/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;