Merge pull request #90 from c-jimenez/fix/memory_leaks

c-jimenez · web-flow · commit 1b1a7616de1b · 2022-12-21T00:34:42.000+01:00
[memory] Fix various memory leaks or memory access/init issues
diff --git a/src/chargepoint/connector/Connectors.cpp b/src/chargepoint/connector/Connectors.cpp
@@ -39,6 +39,17 @@ Connectors::Connectors(ocpp::config::IOcppConfig& ocpp_config, ocpp::database::D
 {
 }
 
+/** @brief Destructor */
+Connectors::~Connectors()
+{
+    // Clear connector array
+    for (Connector* connector : m_connectors)
+    {
+        delete connector;
+    }
+    m_connectors.clear();
+}
+
 /** @brief Indicate if a connector id is valid */
 bool Connectors::isValid(unsigned int id) const
 {
diff --git a/src/chargepoint/connector/Connectors.h b/src/chargepoint/connector/Connectors.h
@@ -43,6 +43,9 @@ class Connectors
     /** @brief Constructor */
     Connectors(ocpp::config::IOcppConfig& ocpp_config, ocpp::database::Database& database, ocpp::helpers::ITimerPool& timer_pool);
 
+    /** @brief Destructor */
+    virtual ~Connectors();
+
     /**
      * @brief Indicate if a connector id is valid
      * @param id Id of the connector
diff --git a/src/rpc/RpcBase.cpp b/src/rpc/RpcBase.cpp
@@ -87,7 +87,7 @@ bool RpcBase::call(const std::string&         action,
             // Wait for response
             std::stringstream expected_id;
             expected_id << m_transaction_id;
-            RpcMessage* rpc_message = nullptr;
+            std::shared_ptr<RpcMessage> rpc_message;
             auto        wait_time   = std::chrono::steady_clock().now() + timeout;
             do
             {
@@ -104,12 +104,15 @@ bool RpcBase::call(const std::string&         action,
                         if (rpc_message->unique_id != expected_id.str())
                         {
                             // Wrong message
-                            delete rpc_message;
-                            rpc_message = nullptr;
+                            rpc_message.reset();
                         }
                     }
                 }
-            } while (ret && (rpc_message == nullptr));
+                else
+                {
+                    ret = false;
+                }
+            } while (ret && !rpc_message);
 
             // Extract response
             if (rpc_message)
@@ -126,7 +129,7 @@ bool RpcBase::call(const std::string&         action,
                 {
                     message = rpc_message->message.GetString();
                 }
-                delete rpc_message;
+                rpc_message.reset();
             }
             else
             {
@@ -164,12 +167,8 @@ void RpcBase::start()
         // Initialize transaction id sequence
         m_transaction_id = std::rand();
 
-        // Flush queues
-        m_requests_queue.clear();
-        m_requests_queue.setEnable(true);
-        m_results_queue.clear();
-
         // Start reception thread
+        m_requests_queue.setEnable(true);
         m_rx_thread = new std::thread(std::bind(&RpcBase::rxThread, this));
     }
 }
@@ -185,6 +184,10 @@ void RpcBase::stop()
         m_rx_thread->join();
         delete m_rx_thread;
         m_rx_thread = nullptr;
+
+        // Flush queues
+        m_requests_queue.clear();
+        m_results_queue.clear();
     }
 }
 
@@ -309,7 +312,7 @@ bool RpcBase::decodeCall(const std::string&      unique_id,
     if (action.IsString() && payload.IsObject())
     {
         // Add request to the queue
-        RpcMessage* msg = new RpcMessage(unique_id, action.GetString(), rpc_frame, payload);
+        auto msg = std::make_shared<RpcMessage>(unique_id, action.GetString(), rpc_frame, payload);
         m_requests_queue.push(msg);
 
         ret = true;
@@ -327,7 +330,7 @@ bool RpcBase::decodeCallResult(const std::string& unique_id, rapidjson::Document
     if (payload.IsObject())
     {
         // Add result to the queue
-        RpcMessage* msg = new RpcMessage(unique_id, rpc_frame, payload);
+        auto msg = std::make_shared<RpcMessage>(unique_id, rpc_frame, payload);
         m_results_queue.push(msg);
 
         ret = true;
@@ -349,7 +352,7 @@ bool RpcBase::decodeCallError(const std::string&   unique_id,
     if (error.IsString() && message.IsString() && payload.IsObject())
     {
         // Add error to the queue
-        RpcMessage* msg = new RpcMessage(unique_id, rpc_frame, payload, &error, &message);
+        auto msg = std::make_shared<RpcMessage>(unique_id, rpc_frame, payload, &error, &message);
         m_results_queue.push(msg);
 
         ret = true;
@@ -380,7 +383,7 @@ void RpcBase::sendCallError(const std::string& unique_id, const char* error, con
 void RpcBase::rxThread()
 {
     // Thread loop
-    RpcMessage* rpc_message = nullptr;
+    std::shared_ptr<RpcMessage> rpc_message;
     while (m_requests_queue.pop(rpc_message))
     {
         // Notify call
@@ -417,7 +420,7 @@ void RpcBase::rxThread()
         }
 
         // Free resources
-        delete rpc_message;
+        rpc_message.reset();
     }
 }
 
diff --git a/src/rpc/RpcBase.h b/src/rpc/RpcBase.h
@@ -23,6 +23,7 @@ along with OpenOCPP. If not, see <http://www.gnu.org/licenses/>.
 #include "Queue.h"
 
 #include <condition_variable>
+#include <memory>
 #include <mutex>
 #include <thread>
 #include <vector>
@@ -129,9 +130,9 @@ class RpcBase : public IRpc
     /** @brief Mutex for concurrent call access */
     std::mutex m_call_mutex;
     /** @brief Queue for incomming call requests */
-    ocpp::helpers::Queue<RpcMessage*> m_requests_queue;
+    ocpp::helpers::Queue<std::shared_ptr<RpcMessage>> m_requests_queue;
     /** @brief Queue for incomming call results */
-    ocpp::helpers::Queue<RpcMessage*> m_results_queue;
+    ocpp::helpers::Queue<std::shared_ptr<RpcMessage>> m_results_queue;
     /** @brief Reception thread */
     std::thread* m_rx_thread;
 
diff --git a/src/tools/helpers/Queue.h b/src/tools/helpers/Queue.h
@@ -115,6 +115,32 @@ class Queue
         return ret;
     }
 
+    /**
+     * @brief Adds an item to the queue
+     * @param item Item to add
+     * @return true if the item has been added, fale if the maximum capacity has been reached
+     */
+    bool push(ItemType&& item)
+    {
+        bool ret = false;
+
+        // Lock queue
+        std::unique_lock<std::mutex> lock(m_mutex);
+
+        // Check size
+        if (m_queue.size() < MAX_SIZE)
+        {
+            // Add item
+            m_queue.push(item);
+
+            // Wakeup waiting thread
+            m_cond_var.notify_one();
+            ret = true;
+        }
+
+        return ret;
+    }
+
     /**
      * @brief Get an item from the queue
      * @param item Item retrieved from the queue
diff --git a/src/tools/x509/Certificate.cpp b/src/tools/x509/Certificate.cpp
@@ -232,6 +232,7 @@ void Certificate::convertCertificateRequest(void* request, const void* issuer, v
     ASN1_INTEGER* serial = ASN1_INTEGER_new();
     ASN1_STRING_set(serial, serial_bytes, sizeof(serial_bytes));
     X509_set_serialNumber(cert, serial);
+    ASN1_INTEGER_free(serial);
 
     // Set subject and issuer name
     X509_NAME* issuer_name;
@@ -268,7 +269,7 @@ void Certificate::convertCertificateRequest(void* request, const void* issuer, v
                 }
             }
         }
-        sk_X509_EXTENSION_free(extensions);
+        sk_X509_EXTENSION_pop_free(extensions, X509_EXTENSION_free);
     }
     if (issuer_cert)
     {
@@ -386,13 +387,15 @@ void Certificate::readInfos(Certificate& certificate)
             certificate.m_x509v3_extensions_names.emplace_back(OBJ_nid2ln(extension_obj_nid));
             if (extension_obj_nid == NID_issuer_alt_name)
             {
-                certificate.m_x509v3_extensions.issuer_alternate_names =
-                    convertGeneralNames(X509_get_ext_d2i(cert, NID_issuer_alt_name, nullptr, nullptr));
+                void* ext                                              = X509_get_ext_d2i(cert, NID_issuer_alt_name, nullptr, nullptr);
+                certificate.m_x509v3_extensions.issuer_alternate_names = convertGeneralNames(ext);
+                OPENSSL_free(ext);
             }
             else if (extension_obj_nid == NID_subject_alt_name)
             {
-                certificate.m_x509v3_extensions.subject_alternate_names =
-                    convertGeneralNames(X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr));
+                void* ext                                               = X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr);
+                certificate.m_x509v3_extensions.subject_alternate_names = convertGeneralNames(ext);
+                OPENSSL_free(ext);
             }
             else if (extension_obj_nid == NID_basic_constraints)
             {
@@ -408,6 +411,7 @@ void Certificate::readInfos(Certificate& certificate)
                             certificate.m_x509v3_extensions.basic_constraints.path_length = ASN1_INTEGER_get(basic_constraint->pathlen);
                         }
                     }
+                    OPENSSL_free(basic_constraint);
                 }
             }
             else
diff --git a/src/tools/x509/CertificateRequest.cpp b/src/tools/x509/CertificateRequest.cpp
@@ -269,14 +269,14 @@ void CertificateRequest::create(const Subject& subject, const Extensions& extens
             sk_GENERAL_NAME_push(names, name);
         }
         X509V3_add1_i2d(&exts, NID_subject_alt_name, names, 0, 0);
-        sk_GENERAL_NAME_free(names);
+        sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
     }
 
     // Add extensions to request
     if (exts)
     {
         X509_REQ_add_extensions(x509_req, exts);
-        sk_X509_EXTENSION_free(exts);
+        sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);
     }
 
     // Sign request
diff --git a/src/websockets/libwebsockets/LibWebsocketClient.cpp b/src/websockets/libwebsockets/LibWebsocketClient.cpp
@@ -254,6 +254,7 @@ void LibWebsocketClient::process()
     }
 
     // Destroy context
+    std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Ensure disconnect caller is joining
     lws_context_destroy(m_context);
 }
 
diff --git a/src/websockets/libwebsockets/LibWebsocketServer.cpp b/src/websockets/libwebsockets/LibWebsocketServer.cpp
@@ -242,6 +242,7 @@ void LibWebsocketServer::process()
     }
 
     // Destroy context
+    std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Ensure stop caller is joining
     lws_context_destroy(m_context);
 }
 
diff --git a/tests/chargepoint/smartcharging/test_profile_database.cpp b/tests/chargepoint/smartcharging/test_profile_database.cpp
@@ -81,12 +81,16 @@ TEST_SUITE("Profile database")
         ChargingProfile profile6;
         profile6.chargingProfileId      = 6;
         profile6.stackLevel             = 6;
+        profile6.chargingProfileKind               = ChargingProfileKindType::Absolute;
         profile6.chargingProfilePurpose = ChargingProfilePurposeType::TxProfile;
+        profile6.chargingSchedule.chargingRateUnit = ChargingRateUnitType::A;
 
         // Tx profiles
         for (size_t i = 0; i < profiles.size(); i++)
         {
+            profiles[i]->chargingProfileKind               = ChargingProfileKindType::Absolute;
             profiles[i]->chargingProfilePurpose = ChargingProfilePurposeType::TxProfile;
+            profiles[i]->chargingSchedule.chargingRateUnit = ChargingRateUnitType::A;
             CHECK(profile_db.install(1u, *profiles[i]));
         }
         CHECK_FALSE(profile_db.install(1u, profile6));

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,17 @@ Connectors::Connectors(ocpp::config::IOcppConfig& ocpp_config, ocpp::database::D`
`39`	`39`	`{`
`40`	`40`	`}`
`41`	`41`
	`42`	`+/** @brief Destructor */`
	`43`	`+Connectors::~Connectors()`
	`44`	`+{`
	`45`	`+ // Clear connector array`
	`46`	`+ for (Connector* connector : m_connectors)`
	`47`	`+ {`
	`48`	`+ delete connector;`
	`49`	`+ }`
	`50`	`+ m_connectors.clear();`
	`51`	`+}`
	`52`	`+`
`42`	`53`	`/** @brief Indicate if a connector id is valid */`
`43`	`54`	`bool Connectors::isValid(unsigned int id) const`
`44`	`55`	`{`
Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ bool RpcBase::call(const std::string& action,`
`87`	`87`	`// Wait for response`
`88`	`88`	`std::stringstream expected_id;`
`89`	`89`	`expected_id << m_transaction_id;`
`90`		`- RpcMessage* rpc_message = nullptr;`
	`90`	`+ std::shared_ptr<RpcMessage> rpc_message;`
`91`	`91`	`auto wait_time = std::chrono::steady_clock().now() + timeout;`
`92`	`92`	`do`
`93`	`93`	`{`
`@@ -104,12 +104,15 @@ bool RpcBase::call(const std::string& action,`
`104`	`104`	`if (rpc_message->unique_id != expected_id.str())`
`105`	`105`	`{`
`106`	`106`	`// Wrong message`
`107`		`- delete rpc_message;`
`108`		`- rpc_message = nullptr;`
	`107`	`+ rpc_message.reset();`
`109`	`108`	`}`
`110`	`109`	`}`
`111`	`110`	`}`
`112`		`- } while (ret && (rpc_message == nullptr));`
	`111`	`+ else`
	`112`	`+ {`
	`113`	`+ ret = false;`
	`114`	`+ }`
	`115`	`+ } while (ret && !rpc_message);`
`113`	`116`
`114`	`117`	`// Extract response`
`115`	`118`	`if (rpc_message)`
`@@ -126,7 +129,7 @@ bool RpcBase::call(const std::string& action,`
`126`	`129`	`{`
`127`	`130`	`message = rpc_message->message.GetString();`
`128`	`131`	`}`
`129`		`- delete rpc_message;`
	`132`	`+ rpc_message.reset();`
`130`	`133`	`}`
`131`	`134`	`else`
`132`	`135`	`{`
`@@ -164,12 +167,8 @@ void RpcBase::start()`
`164`	`167`	`// Initialize transaction id sequence`
`165`	`168`	`m_transaction_id = std::rand();`
`166`	`169`
`167`		`- // Flush queues`
`168`		`- m_requests_queue.clear();`
`169`		`- m_requests_queue.setEnable(true);`
`170`		`- m_results_queue.clear();`
`171`		`-`
`172`	`170`	`// Start reception thread`
	`171`	`+ m_requests_queue.setEnable(true);`
`173`	`172`	`m_rx_thread = new std::thread(std::bind(&RpcBase::rxThread, this));`
`174`	`173`	`}`
`175`	`174`	`}`
`@@ -185,6 +184,10 @@ void RpcBase::stop()`
`185`	`184`	`m_rx_thread->join();`
`186`	`185`	`delete m_rx_thread;`
`187`	`186`	`m_rx_thread = nullptr;`
	`187`	`+`
	`188`	`+ // Flush queues`
	`189`	`+ m_requests_queue.clear();`
	`190`	`+ m_results_queue.clear();`
`188`	`191`	`}`
`189`	`192`	`}`
`190`	`193`
`@@ -309,7 +312,7 @@ bool RpcBase::decodeCall(const std::string& unique_id,`
`309`	`312`	`if (action.IsString() && payload.IsObject())`
`310`	`313`	`{`
`311`	`314`	`// Add request to the queue`
`312`		`- RpcMessage* msg = new RpcMessage(unique_id, action.GetString(), rpc_frame, payload);`
	`315`	`+ auto msg = std::make_shared<RpcMessage>(unique_id, action.GetString(), rpc_frame, payload);`
`313`	`316`	`m_requests_queue.push(msg);`
`314`	`317`
`315`	`318`	`ret = true;`
`@@ -327,7 +330,7 @@ bool RpcBase::decodeCallResult(const std::string& unique_id, rapidjson::Document`
`327`	`330`	`if (payload.IsObject())`
`328`	`331`	`{`
`329`	`332`	`// Add result to the queue`
`330`		`- RpcMessage* msg = new RpcMessage(unique_id, rpc_frame, payload);`
	`333`	`+ auto msg = std::make_shared<RpcMessage>(unique_id, rpc_frame, payload);`
`331`	`334`	`m_results_queue.push(msg);`
`332`	`335`
`333`	`336`	`ret = true;`
`@@ -349,7 +352,7 @@ bool RpcBase::decodeCallError(const std::string& unique_id,`
`349`	`352`	`if (error.IsString() && message.IsString() && payload.IsObject())`
`350`	`353`	`{`
`351`	`354`	`// Add error to the queue`
`352`		`- RpcMessage* msg = new RpcMessage(unique_id, rpc_frame, payload, &error, &message);`
	`355`	`+ auto msg = std::make_shared<RpcMessage>(unique_id, rpc_frame, payload, &error, &message);`
`353`	`356`	`m_results_queue.push(msg);`
`354`	`357`
`355`	`358`	`ret = true;`
`@@ -380,7 +383,7 @@ void RpcBase::sendCallError(const std::string& unique_id, const char* error, con`
`380`	`383`	`void RpcBase::rxThread()`
`381`	`384`	`{`
`382`	`385`	`// Thread loop`
`383`		`- RpcMessage* rpc_message = nullptr;`
	`386`	`+ std::shared_ptr<RpcMessage> rpc_message;`
`384`	`387`	`while (m_requests_queue.pop(rpc_message))`
`385`	`388`	`{`
`386`	`389`	`// Notify call`
`@@ -417,7 +420,7 @@ void RpcBase::rxThread()`
`417`	`420`	`}`
`418`	`421`
`419`	`422`	`// Free resources`
`420`		`- delete rpc_message;`
	`423`	`+ rpc_message.reset();`
`421`	`424`	`}`
`422`	`425`	`}`
`423`	`426`
Original file line number	Diff line number	Diff line change
`@@ -232,6 +232,7 @@ void Certificate::convertCertificateRequest(void* request, const void* issuer, v`
`232`	`232`	`ASN1_INTEGER* serial = ASN1_INTEGER_new();`
`233`	`233`	`ASN1_STRING_set(serial, serial_bytes, sizeof(serial_bytes));`
`234`	`234`	`X509_set_serialNumber(cert, serial);`
	`235`	`+ ASN1_INTEGER_free(serial);`
`235`	`236`
`236`	`237`	`// Set subject and issuer name`
`237`	`238`	`X509_NAME* issuer_name;`
`@@ -268,7 +269,7 @@ void Certificate::convertCertificateRequest(void* request, const void* issuer, v`
`268`	`269`	`}`
`269`	`270`	`}`
`270`	`271`	`}`
`271`		`- sk_X509_EXTENSION_free(extensions);`
	`272`	`+ sk_X509_EXTENSION_pop_free(extensions, X509_EXTENSION_free);`
`272`	`273`	`}`
`273`	`274`	`if (issuer_cert)`
`274`	`275`	`{`
`@@ -386,13 +387,15 @@ void Certificate::readInfos(Certificate& certificate)`
`386`	`387`	`certificate.m_x509v3_extensions_names.emplace_back(OBJ_nid2ln(extension_obj_nid));`
`387`	`388`	`if (extension_obj_nid == NID_issuer_alt_name)`
`388`	`389`	`{`
`389`		`- certificate.m_x509v3_extensions.issuer_alternate_names =`
`390`		`- convertGeneralNames(X509_get_ext_d2i(cert, NID_issuer_alt_name, nullptr, nullptr));`
	`390`	`+ void* ext = X509_get_ext_d2i(cert, NID_issuer_alt_name, nullptr, nullptr);`
	`391`	`+ certificate.m_x509v3_extensions.issuer_alternate_names = convertGeneralNames(ext);`
	`392`	`+ OPENSSL_free(ext);`
`391`	`393`	`}`
`392`	`394`	`else if (extension_obj_nid == NID_subject_alt_name)`
`393`	`395`	`{`
`394`		`- certificate.m_x509v3_extensions.subject_alternate_names =`
`395`		`- convertGeneralNames(X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr));`
	`396`	`+ void* ext = X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr);`
	`397`	`+ certificate.m_x509v3_extensions.subject_alternate_names = convertGeneralNames(ext);`
	`398`	`+ OPENSSL_free(ext);`
`396`	`399`	`}`
`397`	`400`	`else if (extension_obj_nid == NID_basic_constraints)`
`398`	`401`	`{`
`@@ -408,6 +411,7 @@ void Certificate::readInfos(Certificate& certificate)`
`408`	`411`	`certificate.m_x509v3_extensions.basic_constraints.path_length = ASN1_INTEGER_get(basic_constraint->pathlen);`
`409`	`412`	`}`
`410`	`413`	`}`
	`414`	`+ OPENSSL_free(basic_constraint);`
`411`	`415`	`}`
`412`	`416`	`}`
`413`	`417`	`else`
Original file line number	Diff line number	Diff line change
`@@ -269,14 +269,14 @@ void CertificateRequest::create(const Subject& subject, const Extensions& extens`
`269`	`269`	`sk_GENERAL_NAME_push(names, name);`
`270`	`270`	`}`
`271`	`271`	`X509V3_add1_i2d(&exts, NID_subject_alt_name, names, 0, 0);`
`272`		`- sk_GENERAL_NAME_free(names);`
	`272`	`+ sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);`
`273`	`273`	`}`
`274`	`274`
`275`	`275`	`// Add extensions to request`
`276`	`276`	`if (exts)`
`277`	`277`	`{`
`278`	`278`	`X509_REQ_add_extensions(x509_req, exts);`
`279`		`- sk_X509_EXTENSION_free(exts);`
	`279`	`+ sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);`
`280`	`280`	`}`
`281`	`281`
`282`	`282`	`// Sign request`
Original file line number	Diff line number	Diff line change
`@@ -254,6 +254,7 @@ void LibWebsocketClient::process()`
`254`	`254`	`}`
`255`	`255`
`256`	`256`	`// Destroy context`
	`257`	`+ std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Ensure disconnect caller is joining`
`257`	`258`	`lws_context_destroy(m_context);`
`258`	`259`	`}`
`259`	`260`
Original file line number	Diff line number	Diff line change
`@@ -242,6 +242,7 @@ void LibWebsocketServer::process()`
`242`	`242`	`}`
`243`	`243`
`244`	`244`	`// Destroy context`
	`245`	`+ std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Ensure stop caller is joining`
`245`	`246`	`lws_context_destroy(m_context);`
`246`	`247`	`}`
`247`	`248`