Replace new/delete with RAII. (#321)

* Replace new/delete with RAII.

* Fix include order lint issue.

* Remove some mamnual memeory managment an reduce the number time things are copied.

* Avoid using temporary buffers.

* Use std::make_shared<T>() rather than std::shared_ptr<T>(new T())

* Make http_request::~http_request public.

* Cleanup new/delete for details::modded_request.

* Convert details::modded_request::upload_ostrm to RAII.

* Convert details::modded_request::standardized_url to RAII.

* Convert details::modded_request::complete_uri to RAII.

* Convert details::modded_request::dhr to RAII.

* Make details::modded_request move-only.

* Remove details::modded_request::second.

Author: bcsgh

src/http_request.cpp

@@ -187,24 +187,22 @@ MHD_Result http_request::build_request_args(void *cls, enum MHD_ValueKind kind,
     return MHD_YES;
 }
 
-MHD_Result http_request::build_request_querystring(void *cls, enum MHD_ValueKind kind, const char *key, const char *arg_value) {
+MHD_Result http_request::build_request_querystring(void *cls, enum MHD_ValueKind kind, const char *key_value, const char *arg_value) {
     // Parameters needed to respect MHD interface, but not used in the implementation.
     std::ignore = kind;
 
     std::string* querystring = static_cast<std::string*>(cls);
-    std::string value = ((arg_value == nullptr) ? "" : arg_value);
 
-    int buffer_size = std::string(key).size() + value.size() + 3;
-    char* buf = new char[buffer_size];
-    if (*querystring == "") {
-        snprintf(buf, buffer_size, "?%s=%s", key, value.c_str());
-        *querystring = std::string(buf);
-    } else {
-        snprintf(buf, buffer_size, "&%s=%s", key, value.c_str());
-        *querystring += std::string(buf);
-    }
+    std::string_view key = key_value;
+    std::string_view value = ((arg_value == nullptr) ? "" : arg_value);
+
+    // Limit to a single allocation.
+    querystring->reserve(querystring->size() + key.size() + value.size() + 3);
 
-    delete[] buf;
+    *querystring += ((*querystring == "") ? "?" : "&");
+    *querystring += key;
+    *querystring += "=";
+    *querystring += value;
 
     return MHD_YES;
 }

src/http_resource.cpp

@@ -21,6 +21,7 @@
 #include "httpserver/http_resource.hpp"
 #include <microhttpd.h>
 #include <iosfwd>
+#include <memory>
 #include "httpserver/string_response.hpp"
 
 namespace httpserver { class http_response; }
@@ -43,7 +44,7 @@ void resource_init(std::map<std::string, bool>* method_state) {
 namespace details {
 
 std::shared_ptr<http_response> empty_render(const http_request&) {
-    return std::shared_ptr<http_response>(new string_response());
+    return std::make_shared<string_response>();
 }
 
 }  // namespace details

src/httpserver/details/modded_request.hpp

@@ -37,39 +37,32 @@ namespace details {
 
 struct modded_request {
     struct MHD_PostProcessor *pp = nullptr;
-    std::string* complete_uri = nullptr;
-    std::string* standardized_url = nullptr;
+    std::unique_ptr<std::string> complete_uri;
+    std::unique_ptr<std::string> standardized_url;
     webserver* ws = nullptr;
 
     std::shared_ptr<http_response> (httpserver::http_resource::*callback)(const httpserver::http_request&);
 
-    http_request* dhr = nullptr;
+    std::unique_ptr<http_request> dhr = nullptr;
     std::shared_ptr<http_response> dhrs;
-    bool second = false;
     bool has_body = false;
 
     std::string upload_key;
     std::string upload_filename;
-    std::ofstream* upload_ostrm = nullptr;
+    std::unique_ptr<std::ofstream> upload_ostrm;
 
     modded_request() = default;
 
-    modded_request(const modded_request& b) = default;
+    modded_request(const modded_request& b) = delete;
     modded_request(modded_request&& b) = default;
 
-    modded_request& operator=(const modded_request& b) = default;
+    modded_request& operator=(const modded_request& b) = delete;
     modded_request& operator=(modded_request&& b) = default;
 
     ~modded_request() {
         if (nullptr != pp) {
             MHD_destroy_post_processor(pp);
         }
-        if (second) {
-            delete dhr;
-        }
-        delete complete_uri;
-        delete standardized_url;
-        delete upload_ostrm;
     }
 };
 

src/httpserver/http_request.hpp

@@ -252,6 +252,8 @@ class http_request {
 
      friend std::ostream &operator<< (std::ostream &os, http_request &r);
 
+     ~http_request();
+
  private:
      /**
       * Default constructor of the class. It is a specific responsibility of apis to initialize this type of objects.
@@ -286,8 +288,6 @@ class http_request {
      http_request& operator=(const http_request& b) = delete;
      http_request& operator=(http_request&& b) = default;
 
-     ~http_request();
-
      std::string path;
      std::string method;
      std::map<std::string, std::map<std::string, http::file_info>> files;

src/webserver.cpp

@@ -182,11 +182,7 @@ void webserver::request_completed(void *cls, struct MHD_Connection *connection,
     std::ignore = connection;
     std::ignore = toe;
 
-    details::modded_request* mr = static_cast<details::modded_request*>(*con_cls);
-    if (mr == nullptr) return;
-
-    delete mr;
-    mr = nullptr;
+    delete static_cast<details::modded_request*>(*con_cls);
 }
 
 bool webserver::register_resource(const std::string& resource, http_resource* hrm, bool family) {
@@ -422,10 +418,9 @@ void* uri_log(void* cls, const char* uri) {
     // Parameter needed to respect MHD interface, but not needed here.
     std::ignore = cls;
 
-    struct details::modded_request* mr = new details::modded_request();
-    mr->complete_uri = new string(uri);
-    mr->second = false;
-    return reinterpret_cast<void*>(mr);
+    auto mr = std::make_unique<details::modded_request>();
+    mr->complete_uri = std::make_unique<string>(uri);
+    return reinterpret_cast<void*>(mr.release());
 }
 
 void error_log(void* cls, const char* fmt, va_list ap) {
@@ -521,8 +516,7 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind,
             if (mr->upload_ostrm == nullptr || !mr->upload_ostrm->is_open()) {
                 mr->upload_key = key;
                 mr->upload_filename = filename;
-                delete mr->upload_ostrm;
-                mr->upload_ostrm = new std::ofstream();
+                mr->upload_ostrm = std::make_unique<std::ofstream>();
                 mr->upload_ostrm->open(file.get_file_system_file_name(), std::ios::binary | std::ios::app);
             }
 
@@ -550,29 +544,28 @@ std::shared_ptr<http_response> webserver::not_found_page(details::modded_request
     if (not_found_resource != nullptr) {
         return not_found_resource(*mr->dhr);
     } else {
-        return std::shared_ptr<http_response>(new string_response(NOT_FOUND_ERROR, http_utils::http_not_found));
+        return std::make_shared<string_response>(NOT_FOUND_ERROR, http_utils::http_not_found);
     }
 }
 
 std::shared_ptr<http_response> webserver::method_not_allowed_page(details::modded_request* mr) const {
     if (method_not_allowed_resource != nullptr) {
         return method_not_allowed_resource(*mr->dhr);
     } else {
-        return std::shared_ptr<http_response>(new string_response(METHOD_ERROR, http_utils::http_method_not_allowed));
+        return std::make_shared<string_response>(METHOD_ERROR, http_utils::http_method_not_allowed);
     }
 }
 
 std::shared_ptr<http_response> webserver::internal_error_page(details::modded_request* mr, bool force_our) const {
     if (internal_error_resource != nullptr && !force_our) {
         return internal_error_resource(*mr->dhr);
     } else {
-        return std::shared_ptr<http_response>(new string_response(GENERIC_ERROR, http_utils::http_internal_server_error, "text/plain"));
+        return std::make_shared<string_response>(GENERIC_ERROR, http_utils::http_internal_server_error);
     }
 }
 
 MHD_Result webserver::requests_answer_first_step(MHD_Connection* connection, struct details::modded_request* mr) {
-    mr->second = true;
-    mr->dhr = new http_request(connection, unescaper);
+    mr->dhr.reset(new http_request(connection, unescaper));
 
     if (!mr->has_body) {
         return MHD_YES;
@@ -748,7 +741,7 @@ MHD_Result webserver::answer_to_connection(void* cls, MHD_Connection* connection
         const char* version, const char* upload_data, size_t* upload_data_size, void** con_cls) {
     struct details::modded_request* mr = static_cast<struct details::modded_request*>(*con_cls);
 
-    if (mr->second != false) {
+    if (mr->dhr) {
         return static_cast<webserver*>(cls)->requests_answer_second_step(connection, method, version, upload_data, upload_data_size, mr);
     }
 
@@ -762,7 +755,7 @@ MHD_Result webserver::answer_to_connection(void* cls, MHD_Connection* connection
     std::string t_url = url;
 
     base_unescaper(&t_url, static_cast<webserver*>(cls)->unescaper);
-    mr->standardized_url = new string(http_utils::standardize_url(t_url));
+    mr->standardized_url = std::make_unique<std::string>(http_utils::standardize_url(t_url));
 
     mr->has_body = false;
 

test/integ/authentication.cpp

@@ -31,6 +31,7 @@
 #endif
 
 #include <curl/curl.h>
+#include <memory>
 
 #include "./httpserver.hpp"
 #include "./littletest.hpp"
@@ -66,24 +67,24 @@ class user_pass_resource : public http_resource {
  public:
      shared_ptr<http_response> render_GET(const http_request& req) {
          if (req.get_user() != "myuser" || req.get_pass() != "mypass") {
-             return shared_ptr<basic_auth_fail_response>(new basic_auth_fail_response("FAIL", "examplerealm"));
+             return std::make_shared<basic_auth_fail_response>("FAIL", "examplerealm");
          }
-         return shared_ptr<string_response>(new string_response(std::string(req.get_user()) + " " + std::string(req.get_pass()), 200, "text/plain"));
+         return std::make_shared<string_response>(std::string(req.get_user()) + " " + std::string(req.get_pass()), 200, "text/plain");
      }
 };
 
 class digest_resource : public http_resource {
  public:
      shared_ptr<http_response> render_GET(const http_request& req) {
          if (req.get_digested_user() == "") {
-             return shared_ptr<digest_auth_fail_response>(new digest_auth_fail_response("FAIL", "examplerealm", MY_OPAQUE, true));
+             return std::make_shared<digest_auth_fail_response>("FAIL", "examplerealm", MY_OPAQUE, true);
          } else {
              bool reload_nonce = false;
              if (!req.check_digest_auth("examplerealm", "mypass", 300, &reload_nonce)) {
-                 return shared_ptr<digest_auth_fail_response>(new digest_auth_fail_response("FAIL", "examplerealm", MY_OPAQUE, reload_nonce));
+                 return std::make_shared<digest_auth_fail_response>("FAIL", "examplerealm", MY_OPAQUE, reload_nonce);
              }
          }
-         return shared_ptr<string_response>(new string_response("SUCCESS", 200, "text/plain"));
+         return std::make_shared<string_response>("SUCCESS", 200, "text/plain");
      }
 };
 

test/integ/ban_system.cpp

@@ -20,6 +20,7 @@
 
 #include <curl/curl.h>
 #include <map>
+#include <memory>
 #include <string>
 
 #include "./httpserver.hpp"
@@ -54,7 +55,7 @@ size_t writefunc(void *ptr, size_t size, size_t nmemb, std::string *s) {
 class ok_resource : public http_resource {
  public:
      shared_ptr<http_response> render_GET(const http_request&) {
-         return shared_ptr<string_response>(new string_response("OK", 200, "text/plain"));
+         return std::make_shared<string_response>("OK", 200, "text/plain");
      }
 };