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>

http/http2_connection.hpp

diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp
index 2c60c12..efe66b6 100644
--- a/http/http2_connection.hpp
+++ b/http/http2_connection.hpp
@@ -37,7 +37,7 @@
 
 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;
@@ -170,7 +170,7 @@
         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 @@
                 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),
@@ -285,7 +285,7 @@
             {
                 asyncResp->res.setExpectedHash(expected);
             }
-            handler->handle(thisReq, asyncResp);
+            handler->handle(it->second.req, asyncResp);
         }
         return 0;
     }
@@ -306,8 +306,8 @@
         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 @@
             return -1;
         }
 
-        crow::Request& thisReq = thisStream->second.req;
+        crow::Request& thisReq = *thisStream->second.req;
 
         if (nameSv == ":path")
         {
@@ -493,7 +493,7 @@
 
             Http2StreamData& stream = streams[frame.hd.stream_id];
             // http2 is by definition always tls
-            stream.req.isSecure = true;
+            stream.req->isSecure = true;
         }
         return 0;
     }