commit 39662a3be1877afd893b70d78a449f91ba36c260
author Ed Tanous <edtanous@google.com> Mon Feb 06 15:09:46 2023 -0800
committer Ed Tanous <ed@tanous.net> Sat Mar 11 00:50:32 2023 +0000
tree d2f28439ca44d16d292f7879efbf8a8577295514
parent 33c6b58026e5dd3bc600d152a07b95f863bfa1a7

Make url by value in Request

There's some tough-to-track-down safety problems in http Request.  This
commit is an attempt to make things more safe, even if it isn't clear
how the old code was wrong.

Previously, the old code took a url_view from the target() string for a
given URI.  This was effectively a pointer, and needed to be updated in
custom move/copy constructors that were error prone to write.

This commit moves to taking the URI by non-view, which involves a copy,
but allows us to use the default move and copy constructors, as well as
have no internal references within Request, which should improve the
safety and reviewability.

There's already so many string copies in bmcweb, that this is unlikely
to show up as any sort of performance regression, and simple code is
much better in this case.

Note, because of a bug in boost::url, we have to explicitly construct a
url_view in any case where we want to use segments() or query() on a
const Request.  This has been reported to the boost maintainers, and is
being worked for a long term solution.

https://github.com/boostorg/url/pull/704

Tested: Redfish service validator passed on last commit in series.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I49a7710e642dff624d578ec1dde088428f284627

redfish-core/include/query.hpp

diff --git a/redfish-core/include/query.hpp b/redfish-core/include/query.hpp
index 0fa6c18..c5d537b 100644
--- a/redfish-core/include/query.hpp
+++ b/redfish-core/include/query.hpp
@@ -78,7 +78,8 @@
     boost::system::error_code ec;
 
     // Try to GET the same resource
-    crow::Request newReq({boost::beast::http::verb::get, req.url, 11}, ec);
+    crow::Request newReq(
+        {boost::beast::http::verb::get, req.url().encoded_path(), 11}, ec);
 
     if (ec)
     {
@@ -127,7 +128,7 @@
     asyncResp->res.addHeader("OData-Version", "4.0");
 
     std::optional<query_param::Query> queryOpt =
-        query_param::parseParameters(req.urlView.params(), asyncResp->res);
+        query_param::parseParameters(req.url().params(), asyncResp->res);
     if (queryOpt == std::nullopt)
     {
         return false;

redfish-core/include/redfish_aggregator.hpp

diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index 7ace802..de51ca6 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -482,7 +482,7 @@
         }
 
         // We didn't recognize the prefix and need to return a 404
-        std::string nameStr = req.urlView.segments().back();
+        std::string nameStr = req.url().segments().back();
         messages::resourceNotFound(asyncResp->res, "", nameStr);
     }
 
@@ -507,7 +507,7 @@
             // don't need to write an error code
             if (isCollection == AggregationType::Resource)
             {
-                std::string nameStr = sharedReq->urlView.segments().back();
+                std::string nameStr = sharedReq->url().segments().back();
                 messages::resourceNotFound(asyncResp->res, "", nameStr);
             }
             return;
@@ -529,8 +529,7 @@
             return;
         }
 
-        const boost::urls::segments_view urlSegments =
-            thisReq.urlView.segments();
+        const boost::urls::segments_view urlSegments = thisReq.url().segments();
         boost::urls::url currentUrl("/");
         boost::urls::segments_view::iterator it = urlSegments.begin();
         const boost::urls::segments_view::const_iterator end =
@@ -823,7 +822,7 @@
     {
         using crow::utility::OrMorePaths;
         using crow::utility::readUrlSegments;
-        const boost::urls::url_view url = thisReq.urlView;
+        const boost::urls::url_view url = thisReq.url();
 
         // We don't need to aggregate JsonSchemas due to potential issues such
         // as version mismatches between aggregator and satellite BMCs.  For