commit 102a4cdacb0a6d8c8c3c97e10bedbb66000ac5dc
author Jonathan Doman <jonathan.doman@intel.com> Mon Apr 15 16:56:23 2024 -0700
committer Ed Tanous <ed@tanous.net> Fri May 03 15:24:02 2024 +0000
tree 56eef176e8bd7446995556b90c8216e4a88a701e
parent 1aa375b80075c7e1acdc9188440a62bab21b8651

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>

test/http/http2_connection_test.cpp

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");
     }
 };

test/http/http_connection_test.cpp

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

test/http/router_test.cpp

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