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>

2 files changed, 22 insertions, 15 deletions

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 <functional>
 #include <memory>
-#include <new>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -31,11 +30,10 @@
 
 namespace redfish
 {
-inline void
-    afterIfMatchRequest(crow::App& app,
-                        const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
-                        crow::Request& req, const std::string& ifMatchHeader,
-                        const crow::Response& resIn)
+inline void afterIfMatchRequest(
+    crow::App& app, const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+    const std::shared_ptr<crow::Request>& 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>(
+        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<bmcweb::AsyncResp> getReqAsyncResp =
         std::make_shared<bmcweb::AsyncResp>();
 
+    // 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<crow::Request>(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>(
+        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<MultiAsyncResp>
             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>(
+                crow::Request::Body{boost::beast::http::verb::get, subQuery,
+                                    11},
+                ec);
             if (ec)
             {
                 messages::internalError(finalRes->res);