Skip to content

Commit

Permalink
Manage Request with shared_ptr
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Signed-off-by: Ed Tanous <[email protected]>
  • Loading branch information
@jonathan-doman @edtanous
jonathan-doman authored and edtanous committed May 3, 2024
1 parent 1aa375b commit 102a4cd
Show file tree
Hide file tree
Showing 11 changed files with 121 additions and 109 deletions.
4 changes: 2 additions & 2 deletions http/app.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ class App
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);
Expand Down
16 changes: 8 additions & 8 deletions http/http2_connection.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace crow

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;
Expand Down Expand Up @@ -170,7 +170,7 @@ class HTTP2Connection :
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());
Expand Down Expand Up @@ -243,7 +243,7 @@ class HTTP2Connection :
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),
Expand Down Expand Up @@ -285,7 +285,7 @@ class HTTP2Connection :
{
asyncResp->res.setExpectedHash(expected);
}
handler->handle(thisReq, asyncResp);
handler->handle(it->second.req, asyncResp);
}
return 0;
}
Expand All @@ -306,8 +306,8 @@ class HTTP2Connection :
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);
Expand Down Expand Up @@ -425,7 +425,7 @@ class HTTP2Connection :
return -1;
}

crow::Request& thisReq = thisStream->second.req;
crow::Request& thisReq = *thisStream->second.req;

if (nameSv == ":path")
{
Expand Down Expand Up @@ -493,7 +493,7 @@ class HTTP2Connection :

Http2StreamData& stream = streams[frame.hd.stream_id];
// http2 is by definition always tls
stream.req.isSecure = true;
stream.req->isSecure = true;
}
return 0;
}
Expand Down
50 changes: 25 additions & 25 deletions http/http_connection.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include <atomic>
#include <chrono>
#include <memory>
#include <vector>

namespace crow
Expand Down Expand Up @@ -232,23 +233,23 @@ class Connection :
{
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());
res.result(boost::beast::http::status::bad_request);
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);
Expand All @@ -257,31 +258,31 @@ class Connection :
}

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)
{
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;
}
Expand All @@ -294,11 +295,11 @@ class Connection :
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)
{
Expand All @@ -320,7 +321,7 @@ class Connection :
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);
Expand Down Expand Up @@ -371,7 +372,7 @@ class Connection :
res = std::move(thisRes);
res.keepAlive(keepAlive);

completeResponseFields(req, res);
completeResponseFields(*req, res);
res.addHeader(boost::beast::http::field::date, getCachedDateStr());

doWrite();
Expand Down Expand Up @@ -515,7 +516,7 @@ class Connection :
}

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_);
Expand Down Expand Up @@ -644,8 +645,7 @@ class Connection :

userSession = nullptr;

// Destroy the Request via the std::optional
req.clear();
req->clear();
doReadHeaders();
}

Expand Down Expand Up @@ -732,7 +732,7 @@ class Connection :

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;
Expand Down
7 changes: 3 additions & 4 deletions http/http_request.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ namespace crow

struct Request
{
boost::beast::http::request<bmcweb::HttpBody> req;
using Body = boost::beast::http::request<bmcweb::HttpBody>;
Body req;

private:
boost::urls::url urlBase;
Expand All @@ -32,9 +33,7 @@ struct Request
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())
{
Expand Down
43 changes: 22 additions & 21 deletions http/routing.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ class Router
return route;
}

FindRouteResponse findRoute(Request& req) const
FindRouteResponse findRoute(const Request& req) const
{
FindRouteResponse findRoute;

Expand Down Expand Up @@ -529,11 +529,11 @@ class Router
}

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);
Expand All @@ -543,11 +543,12 @@ class Router
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;
}
Expand All @@ -563,7 +564,7 @@ class Router
{
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;
Expand All @@ -574,39 +575,38 @@ class Router

// 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)
{
// Couldn't find a normal route with any verb, try looking for a 404
// 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);
}
}
Expand Down Expand Up @@ -640,14 +640,15 @@ class Router
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);
});
}

Expand Down
Loading

0 comments on commit 102a4cd

Please sign in to comment.