From 102a4cdacb0a6d8c8c3c97e10bedbb66000ac5dc Mon Sep 17 00:00:00 2001
From: Jonathan Doman <jonathan.doman@intel.com>
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 <jonathan.doman@intel.com>
Signed-off-by: Ed Tanous <ed@tanous.net>
---
 test/http/http2_connection_test.cpp | 13 +++++++------
 test/http/http_connection_test.cpp  | 16 ++++++++--------
 test/http/router_test.cpp           |  9 ++++++---
 3 files changed, 21 insertions(+), 17 deletions(-)

(limited to 'test')

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<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");
     }
 };
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<Request>& /*req*/,
                       const std::shared_ptr<bmcweb::AsyncResp>& /*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<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;
     }
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>(
+            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 @@ 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>(
+        Request::Body{boost::beast::http::verb::get, url, 11}, ec);
 
     router.newRuleTagged<getParameterTag(url)>("/foo/<path>")
         .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>(
+        Request::Body{boost::beast::http::verb::patch, url, 11}, ec);
 
     router.newRuleTagged<getParameterTag(url)>(std::string(url))
         .methods(boost::beast::http::verb::get)(nullCallback);
-- 
cgit v1.2.3