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 ++++++++++++------------- include/dbus_privileges.hpp | 35 +++++++++++---------- redfish-core/include/query.hpp | 28 ++++++++++------- redfish-core/include/utils/query_param.hpp | 9 ++++-- test/http/http2_connection_test.cpp | 13 ++++---- test/http/http_connection_test.cpp | 16 +++++----- test/http/router_test.cpp | 9 ++++-- 11 files changed, 121 insertions(+), 109 deletions(-) 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); }); } diff --git a/include/dbus_privileges.hpp b/include/dbus_privileges.hpp index a58f9bebd0..4cafe52ec6 100644 --- a/include/dbus_privileges.hpp +++ b/include/dbus_privileges.hpp @@ -106,19 +106,18 @@ inline bool return true; } -template -void afterGetUserInfo(Request& req, - const std::shared_ptr& 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& 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 @@ void afterGetUserInfo(Request& req, 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 @@ void afterGetUserInfo(Request& req, // 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 -void validatePrivilege(Request& req, +void validatePrivilege(const std::shared_ptr& req, const std::shared_ptr& 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(callback)]( + [req, asyncResp, &rule, callback = std::forward(callback)]( const boost::system::error_code& ec, const dbus::utility::DBusPropertiesMap& userInfoMap) mutable { - afterGetUserInfo(req, asyncResp, rule, - std::forward(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 063f1a8ef2..2d280cb787 100644 --- a/redfish-core/include/query.hpp +++ b/redfish-core/include/query.hpp @@ -16,7 +16,6 @@ #include #include -#include #include #include #include @@ -31,11 +30,10 @@ namespace redfish { -inline void - afterIfMatchRequest(crow::App& app, - const std::shared_ptr& asyncResp, - crow::Request& req, const std::string& ifMatchHeader, - const crow::Response& resIn) +inline void afterIfMatchRequest( + crow::App& app, const std::shared_ptr& asyncResp, + const std::shared_ptr& 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 @@ inline void 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 @@ inline bool handleIfMatch(crow::App& app, const crow::Request& req, 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::Body{boost::beast::http::verb::get, + req.url().encoded_path(), 11}, + ec); if (ec) { @@ -94,17 +94,21 @@ inline bool handleIfMatch(crow::App& app, const crow::Request& req, } // 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 getReqAsyncResp = std::make_shared(); + // 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(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 eadb66ed5c..3b04e21991 100644 --- a/redfish-core/include/utils/query_param.hpp +++ b/redfish-core/include/utils/query_param.hpp @@ -503,7 +503,8 @@ inline bool processOnly(crow::App& app, crow::Response& res, // 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::Body{boost::beast::http::verb::get, *url, 11}, ec); if (ec) { messages::internalError(res); @@ -854,8 +855,10 @@ class MultiAsyncResp : public std::enable_shared_from_this 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::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 34dc7cd20e..0385ed62b9 100644 --- a/test/http/http2_connection_test.cpp +++ b/test/http/http2_connection_test.cpp @@ -38,16 +38,17 @@ using ::testing::UnorderedElementsAre; struct FakeHandler { bool called = false; - void handle(Request& req, + void handle(const std::shared_ptr& req, const std::shared_ptr& 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 caf50c8a62..393e209b8a 100644 --- a/test/http/http_connection_test.cpp +++ b/test/http/http_connection_test.cpp @@ -23,7 +23,7 @@ namespace crow struct FakeHandler { static void - handleUpgrade(Request& /*req*/, + handleUpgrade(const std::shared_ptr& /*req*/, const std::shared_ptr& /*asyncResp*/, boost::beast::test::stream&& /*adaptor*/) { @@ -31,16 +31,16 @@ struct FakeHandler EXPECT_FALSE(true); } - void handle(Request& req, + void handle(const std::shared_ptr& req, const std::shared_ptr& /*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 f23331782b..23725416af 100644 --- a/test/http/router_test.cpp +++ b/test/http/router_test.cpp @@ -87,7 +87,8 @@ TEST(Router, OverlapingRoutes) { constexpr std::string_view url = "/foo/bar"; - Request req{{boost::beast::http::verb::get, url, 11}, ec}; + auto req = std::make_shared( + Request::Body{boost::beast::http::verb::get, url, 11}, ec); std::shared_ptr asyncResp = std::make_shared(); @@ -112,7 +113,8 @@ TEST(Router, 404) constexpr std::string_view url = "/foo/bar"; - Request req{{boost::beast::http::verb::get, url, 11}, ec}; + auto req = std::make_shared( + Request::Body{boost::beast::http::verb::get, url, 11}, ec); router.newRuleTagged("/foo/") .notFound()(nullCallback); @@ -142,7 +144,8 @@ TEST(Router, 405) constexpr std::string_view url = "/foo/bar"; - Request req{{boost::beast::http::verb::patch, url, 11}, ec}; + auto req = std::make_shared( + Request::Body{boost::beast::http::verb::patch, url, 11}, ec); router.newRuleTagged(std::string(url)) .methods(boost::beast::http::verb::get)(nullCallback); -- cgit v1.2.3