From 102a4cdacb0a6d8c8c3c97e10bedbb66000ac5dc Mon Sep 17 00:00:00 2001 From: Jonathan Doman Date: Mon, 15 Apr 2024 16:56:23 -0700 Subject: 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 Signed-off-by: Ed Tanous --- http/app.hpp | 4 ++-- http/http2_connection.hpp | 16 +++++++-------- http/http_connection.hpp | 50 +++++++++++++++++++++++------------------------ http/http_request.hpp | 7 +++---- http/routing.hpp | 43 ++++++++++++++++++++-------------------- 5 files changed, 60 insertions(+), 60 deletions(-) (limited to 'http') diff --git a/http/app.hpp b/http/app.hpp index eea13058e1..29cb4de9eb 100644 --- a/http/app.hpp +++ b/http/app.hpp @@ -54,14 +54,14 @@ class App App& operator=(const App&&) = delete; template - void handleUpgrade(Request& req, + void handleUpgrade(const std::shared_ptr& req, const std::shared_ptr& asyncResp, Adaptor&& adaptor) { router.handleUpgrade(req, asyncResp, std::forward(adaptor)); } - void handle(Request& req, + void handle(const std::shared_ptr& req, const std::shared_ptr& asyncResp) { router.handle(req, asyncResp); diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp index 2c60c1268e..efe66b635f 100644 --- a/http/http2_connection.hpp +++ b/http/http2_connection.hpp @@ -37,7 +37,7 @@ namespace crow struct Http2StreamData { - Request req; + std::shared_ptr req = std::make_shared(); std::optional reqReader; Response res; std::optional writer; @@ -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()); @@ -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( &adaptor.get_executor().context()); BMCWEB_LOG_DEBUG("Handling {} \"{}\"", logPtr(&thisReq), @@ -285,7 +285,7 @@ class HTTP2Connection : { asyncResp->res.setExpectedHash(expected); } - handler->handle(thisReq, asyncResp); + handler->handle(it->second.req, asyncResp); } return 0; } @@ -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); @@ -425,7 +425,7 @@ class HTTP2Connection : return -1; } - crow::Request& thisReq = thisStream->second.req; + crow::Request& thisReq = *thisStream->second.req; if (nameSv == ":path") { @@ -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; } diff --git a/http/http_connection.hpp b/http/http_connection.hpp index e02c518ebb..29e876d0a9 100644 --- a/http/http_connection.hpp +++ b/http/http_connection.hpp @@ -30,6 +30,7 @@ #include #include +#include #include namespace crow @@ -232,7 +233,7 @@ class Connection : { return; } - req = crow::Request(parser->release(), reqEc); + req = std::make_shared(parser->release(), reqEc); if (reqEc) { BMCWEB_LOG_DEBUG("Request failed to construct{}", reqEc.message()); @@ -240,15 +241,15 @@ class Connection : 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 @@ 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( + req->ioService = static_castioService)>( &adaptor.get_executor().context()); if (res.completed) @@ -269,19 +270,19 @@ class Connection : completeRequest(res); return; } - keepAlive = req.keepAlive(); + keepAlive = req->keepAlive(); if constexpr (!std::is_same_v) { #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 @@ 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) { @@ -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); @@ -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(); @@ -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_); @@ -644,8 +645,7 @@ class Connection : userSession = nullptr; - // Destroy the Request via the std::optional - req.clear(); + req->clear(); doReadHeaders(); } @@ -732,7 +732,7 @@ class Connection : boost::beast::flat_static_buffer<8192> buffer; - crow::Request req; + std::shared_ptr req; crow::Response res; std::shared_ptr userSession; diff --git a/http/http_request.hpp b/http/http_request.hpp index d1a6ac053a..d2a1e259fc 100644 --- a/http/http_request.hpp +++ b/http/http_request.hpp @@ -18,7 +18,8 @@ namespace crow struct Request { - boost::beast::http::request req; + using Body = boost::beast::http::request; + Body req; private: boost::urls::url urlBase; @@ -32,9 +33,7 @@ struct Request std::shared_ptr session; std::string userRole; - Request(boost::beast::http::request 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 fa6db58bfc..7e481a0dd2 100644 --- a/http/routing.hpp +++ b/http/routing.hpp @@ -491,7 +491,7 @@ class Router return route; } - FindRouteResponse findRoute(Request& req) const + FindRouteResponse findRoute(const Request& req) const { FindRouteResponse findRoute; @@ -529,11 +529,11 @@ class Router } template - void handleUpgrade(Request& req, + void handleUpgrade(const std::shared_ptr& req, const std::shared_ptr& asyncResp, Adaptor&& adaptor) { - std::optional verb = httpVerbFromBoost(req.method()); + std::optional verb = httpVerbFromBoost(req->method()); if (!verb || static_cast(*verb) >= perMethods.size()) { asyncResp->res.result(boost::beast::http::status::not_found); @@ -543,11 +543,12 @@ class Router Trie& trie = perMethod.trie; std::vector& 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 @@ 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(*verb), methods); asyncResp->res.result(boost::beast::http::status::not_found); return; @@ -574,25 +575,24 @@ 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)]( - Request& thisReq) mutable { - rule.handleUpgrade(thisReq, asyncResp, std::move(adaptor)); + validatePrivilege(req, asyncResp, rule, + [req, &rule, asyncResp, + adaptor = std::forward(adaptor)]() mutable { + rule.handleUpgrade(*req, asyncResp, std::move(adaptor)); }); } - void handle(Request& req, + void handle(const std::shared_ptr& req, const std::shared_ptr& asyncResp) { - std::optional verb = httpVerbFromBoost(req.method()); + std::optional verb = httpVerbFromBoost(req->method()); if (!verb || static_cast(*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 @@ class Router // 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 @@ class Router BMCWEB_LOG_DEBUG("Matched rule '{}' {} / {}", rule.rule, static_cast(*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); }); } -- cgit v1.2.3