Skip to content

Fix some race conditions which lead to memory errors #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Fix some race conditions which lead to memory errors #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a88ba3
[logger] Fix race condition on log database table
c-jimenez Jan 25, 2023
ab41234
[messages] Fix race condition on allocators used in messages converters
c-jimenez Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/centralsystem/chargepoint/ChargePointHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,9 @@ class ChargePointHandler
template <typename RequestType, typename ResponseType>
ocpp::types::DataTransferStatus handleMessage(const std::string& type_id, const std::string& request_data, std::string& response_data)
{
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
try
{
// Parse JSON
Expand Down
4 changes: 2 additions & 2 deletions src/centralsystem/chargepoint/ChargePointProxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,8 @@ class ChargePointProxy : public ICentralSystem::IChargePoint, public ocpp::rpc::
bool ret = false;

// Get converters
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);

// Prepare request
ocpp::messages::DataTransferReq req;
Expand Down
10 changes: 5 additions & 5 deletions src/chargepoint/iso15118/Iso15118Manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ class Iso15118Manager : public IDataTransferManager::IDataTransferHandler
bool ret = false;

// Get converters
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);

// Prepare request
ocpp::messages::DataTransferReq req;
Expand Down Expand Up @@ -240,9 +240,9 @@ class Iso15118Manager : public IDataTransferManager::IDataTransferHandler
template <typename RequestType, typename ResponseType>
ocpp::types::DataTransferStatus handle(const std::string& type_id, const std::string& request_data, std::string& response_data)
{
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
try
{
// Parse JSON
Expand Down
6 changes: 3 additions & 3 deletions src/chargepoint/security/SecurityManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ SecurityManager::SecurityManager(const ocpp::config::IChargePointConfig&
m_worker_pool(worker_pool),
m_requests_fifo(requests_fifo),
m_security_event_req_converter(
*messages_converter.getRequestConverter<SecurityEventNotificationReq>(SECURITY_EVENT_NOTIFICATION_ACTION)),
messages_converter.getRequestConverter<SecurityEventNotificationReq>(SECURITY_EVENT_NOTIFICATION_ACTION)),
m_charge_point(charge_point),
m_security_logs_db(stack_config, database),
m_ca_certificates_db(stack_config, database),
Expand Down Expand Up @@ -319,8 +319,8 @@ bool SecurityManager::logSecurityEvent(const std::string& type, const std::strin
// Stack is not started, queue the notification
rapidjson::Document payload;
payload.Parse("{}");
m_security_event_req_converter.setAllocator(&payload.GetAllocator());
if (m_security_event_req_converter.toJson(request, payload))
m_security_event_req_converter->setAllocator(&payload.GetAllocator());
if (m_security_event_req_converter->toJson(request, payload))
{
m_requests_fifo.push(0, SECURITY_EVENT_NOTIFICATION_ACTION, payload);
}
Expand Down
2 changes: 1 addition & 1 deletion src/chargepoint/security/SecurityManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ class SecurityManager
/** @brief Transaction related requests FIFO */
ocpp::messages::IRequestFifo& m_requests_fifo;
/** @brief Message converter for SecurityEventNotificationReq */
ocpp::messages::IMessageConverter<ocpp::messages::SecurityEventNotificationReq>& m_security_event_req_converter;
std::unique_ptr<ocpp::messages::IMessageConverter<ocpp::messages::SecurityEventNotificationReq>> m_security_event_req_converter;
/** @brief Charge Point */
IChargePoint& m_charge_point;

Expand Down
14 changes: 7 additions & 7 deletions src/messages/GenericMessageHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class GenericMessageHandler : public IMessageDispatcher::IMessageHandler
public:
/** @brief Constructor */
GenericMessageHandler(const std::string& action, const GenericMessagesConverter& messages_converter)
: m_request_converter(*messages_converter.getRequestConverter<RequestType>(action)),
m_response_converter(*messages_converter.getResponseConverter<ResponseType>(action))
: m_request_converter(messages_converter.getRequestConverter<RequestType>(action)),
m_response_converter(messages_converter.getResponseConverter<ResponseType>(action))
{
}

Expand All @@ -60,15 +60,15 @@ class GenericMessageHandler : public IMessageDispatcher::IMessageHandler

// Convert request
RequestType request;
if (m_request_converter.fromJson(payload, request, error_code, error_message))
if (m_request_converter->fromJson(payload, request, error_code, error_message))
{
// Handle message
ResponseType resp;
if (handleMessage(request, resp, error_code, error_message))
{
// Convert response
m_response_converter.setAllocator(&response.GetAllocator());
ret = m_response_converter.toJson(resp, response);
m_response_converter->setAllocator(&response.GetAllocator());
ret = m_response_converter->toJson(resp, response);
}
}

Expand All @@ -86,8 +86,8 @@ class GenericMessageHandler : public IMessageDispatcher::IMessageHandler
virtual bool handleMessage(const RequestType& request, ResponseType& response, std::string& error_code, std::string& error_message) = 0;

private:
IMessageConverter<RequestType>& m_request_converter;
IMessageConverter<ResponseType>& m_response_converter;
std::unique_ptr<IMessageConverter<RequestType>> m_request_converter;
std::unique_ptr<IMessageConverter<ResponseType>> m_response_converter;
};

} // namespace messages
Expand Down
6 changes: 3 additions & 3 deletions src/messages/GenericMessageSender.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ class GenericMessageSender
CallResult ret = CallResult::Failed;

// Get converters
IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(action);
IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(action);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
if (req_converter && resp_converter)
{
// Convert request
Expand Down Expand Up @@ -204,7 +204,7 @@ class GenericMessageSender
CallResult ret = CallResult::Failed;

// Get converter
IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
if (resp_converter)
{
// Execute call
Expand Down
46 changes: 38 additions & 8 deletions src/messages/GenericMessagesConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ along with OpenOCPP. If not, see <http://www.gnu.org/licenses/>.
#ifndef OPENOCPP_GENERICMESSAGESCONVERTER_H
#define OPENOCPP_GENERICMESSAGESCONVERTER_H

#include <memory>
#include <string>
#include <unordered_map>

namespace ocpp
{
namespace messages
Expand All @@ -42,13 +44,13 @@ class GenericMessagesConverter
* @return Pointer to the message converter for the request or nullptr if the converter doesn't exists
*/
template <typename RequestType>
IMessageConverter<RequestType>* getRequestConverter(const std::string& action) const
std::unique_ptr<IMessageConverter<RequestType>> getRequestConverter(const std::string& action) const
{
IMessageConverter<RequestType>* ret = nullptr;
auto it = m_req_converters.find(action);
std::unique_ptr<IMessageConverter<RequestType>> ret;
auto it = m_req_converters.find(action);
if (it != m_req_converters.end())
{
ret = reinterpret_cast<IMessageConverter<RequestType>*>(it->second);
ret.reset(reinterpret_cast<IMessageConverter<RequestType>*>(it->second)->clone());
}
return ret;
}
Expand All @@ -59,13 +61,13 @@ class GenericMessagesConverter
* @return Pointer to the message converter for the response or nullptr if the converter doesn't exists
*/
template <typename ResponseType>
IMessageConverter<ResponseType>* getResponseConverter(const std::string& action) const
std::unique_ptr<IMessageConverter<ResponseType>> getResponseConverter(const std::string& action) const
{
IMessageConverter<ResponseType>* ret = nullptr;
auto it = m_resp_converters.find(action);
std::unique_ptr<IMessageConverter<ResponseType>> ret;
auto it = m_resp_converters.find(action);
if (it != m_resp_converters.end())
{
ret = reinterpret_cast<IMessageConverter<ResponseType>*>(it->second);
ret.reset(reinterpret_cast<IMessageConverter<ResponseType>*>(it->second)->clone());
}
return ret;
}
Expand Down Expand Up @@ -93,6 +95,34 @@ class GenericMessagesConverter
m_resp_converters[action] = &converter;
}

/**
* @brief Delete a converter for a request
* @param action Ocpp call action corresponding to the request
*/
template <typename RequestType>
void deleteRequestConverter(const std::string& action)
{
auto it = m_req_converters.find(action);
if (it != m_req_converters.end())
{
delete reinterpret_cast<IMessageConverter<RequestType>*>(it->second);
}
}

/**
* @brief Delete a converter for a response
* @param action Ocpp call action corresponding to the response
*/
template <typename ResponseType>
void deleteResponseConverter(const std::string& action)
{
auto it = m_resp_converters.find(action);
if (it != m_resp_converters.end())
{
delete reinterpret_cast<IMessageConverter<ResponseType>*>(it->second);
}
}

private:
/** @brief Request converters */
std::unordered_map<std::string, void*> m_req_converters;
Expand Down
18 changes: 13 additions & 5 deletions src/messages/IMessageConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ class IMessageConverter
/** @brief Destructor */
virtual ~IMessageConverter() { }

/**
* Clone a message converter instance
* @return Cloned message converter instance
*/
virtual IMessageConverter* clone() const = 0;

/**
* @brief Convert a JSON object to a C++ data type
* @param json JSON object to convert
Expand Down Expand Up @@ -273,17 +279,19 @@ class IMessageConverter
class MessageType##ReqConverter : public IMessageConverter<MessageType##Req> \
{ \
public: \
IMessageConverter<MessageType##Req>* clone() const override { return new MessageType##ReqConverter(); } \
bool fromJson(const rapidjson::Value& json, MessageType##Req& data, std::string& error_code, std::string& error_message) override; \
bool toJson(const MessageType##Req& data, rapidjson::Document& json) override; \
}; \
class MessageType##ConfConverter : public IMessageConverter<MessageType##Conf> \
{ \
public: \
bool fromJson(const rapidjson::Value& json, \
MessageType##Conf& data, \
std::string& error_code, \
std::string& error_message) override; \
bool toJson(const MessageType##Conf& data, rapidjson::Document& json) override; \
IMessageConverter<MessageType##Conf>* clone() const override { return new MessageType##ConfConverter(); } \
bool fromJson(const rapidjson::Value& json, \
MessageType##Conf& data, \
std::string& error_code, \
std::string& error_message) override; \
bool toJson(const MessageType##Conf& data, rapidjson::Document& json) override; \
};

} // namespace messages
Expand Down
6 changes: 3 additions & 3 deletions src/messages/MessagesConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ along with OpenOCPP. If not, see <http://www.gnu.org/licenses/>.
registerResponseConverter<action##Conf>(#action, *new action##ConfConverter())

/** @brief Macro to delete a message converter for an OCPP action */
#define DELETE_CONVERTER(action) \
delete getRequestConverter<action##Req>(#action); \
delete getResponseConverter<action##Conf>(#action)
#define DELETE_CONVERTER(action) \
deleteRequestConverter<action##Req>(#action); \
deleteResponseConverter<action##Conf>(#action)

namespace ocpp
{
Expand Down
3 changes: 3 additions & 0 deletions src/messages/types/AuthorizationDataConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ namespace messages
class AuthorizationDataConverter : public IMessageConverter<ocpp::types::AuthorizationData>
{
public:
/** @copydoc IMessageConverter<ocpp::types::AuthorizationData>* IMessageConverter<ocpp::types::AuthorizationData>::clone() const */
IMessageConverter<ocpp::types::AuthorizationData>* clone() const override { return new AuthorizationDataConverter(); }

/** @copydoc bool IMessageConverter<ocpp::types::AuthorizationData>::fromJson(const rapidjson::Value&,
* ocpp::types::AuthorizationData&,
* std::string&,
Expand Down
6 changes: 6 additions & 0 deletions src/messages/types/CertificateHashDataChainTypeConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ namespace messages
class CertificateHashDataChainTypeConverter : public IMessageConverter<ocpp::types::CertificateHashDataChainType>
{
public:
/** @copydoc IMessageConverter<ocpp::types::CertificateHashDataChainType>* IMessageConverter<ocpp::types::CertificateHashDataChainType>::clone() const */
IMessageConverter<ocpp::types::CertificateHashDataChainType>* clone() const override
{
return new CertificateHashDataChainTypeConverter();
}

/** @copydoc bool IMessageConverter<ocpp::types::CertificateHashDataChainType>::fromJson(const rapidjson::Value&,
* ocpp::types::CertificateHashDataChainType&,
* std::string&,
Expand Down
3 changes: 3 additions & 0 deletions src/messages/types/CertificateHashDataTypeConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ namespace messages
class CertificateHashDataTypeConverter : public IMessageConverter<ocpp::types::CertificateHashDataType>
{
public:
/** @copydoc IMessageConverter<ocpp::types::CertificateHashDataType>* IMessageConverter<ocpp::types::CertificateHashDataType>::clone() const */
IMessageConverter<ocpp::types::CertificateHashDataType>* clone() const override { return new CertificateHashDataTypeConverter(); }

/** @copydoc bool IMessageConverter<ocpp::types::CertificateHashDataType>::fromJson(const rapidjson::Value&,
* ocpp::types::CertificateHashDataType&,
* std::string&,
Expand Down
3 changes: 3 additions & 0 deletions src/messages/types/ChargingProfileConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ namespace messages
class ChargingProfileConverter : public IMessageConverter<ocpp::types::ChargingProfile>
{
public:
/** @copydoc IMessageConverter<ocpp::types::ChargingProfile>* IMessageConverter<ocpp::types::ChargingProfile>::clone() const */
IMessageConverter<ocpp::types::ChargingProfile>* clone() const override { return new ChargingProfileConverter(); }

/** @copydoc bool IMessageConverter<ocpp::types::ChargingProfile>::fromJson(const rapidjson::Value&,
* ocpp::types::ChargingProfile&,
* std::string&,
Expand Down
3 changes: 3 additions & 0 deletions src/messages/types/ChargingScheduleConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ namespace messages
class ChargingScheduleConverter : public IMessageConverter<ocpp::types::ChargingSchedule>
{
public:
/** @copydoc IMessageConverter<ocpp::types::ChargingSchedule>* IMessageConverter<ocpp::types::ChargingSchedule>::clone() const */
IMessageConverter<ocpp::types::ChargingSchedule>* clone() const override { return new ChargingScheduleConverter(); }

/** @copydoc bool IMessageConverter<ocpp::types::ChargingSchedule>::fromJson(const rapidjson::Value&,
* ocpp::types::ChargingSchedule&,
* std::string&,
Expand Down
3 changes: 3 additions & 0 deletions src/messages/types/IdTagInfoConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ namespace messages
class IdTagInfoConverter : public IMessageConverter<ocpp::types::IdTagInfo>
{
public:
/** @copydoc IMessageConverter<ocpp::types::IdTagInfo>* IMessageConverter<ocpp::types::IdTagInfo>::clone() const */
IMessageConverter<ocpp::types::IdTagInfo>* clone() const override { return new IdTagInfoConverter(); }

/** @copydoc bool IMessageConverter<ocpp::types::IdTagInfo>::fromJson(const rapidjson::Value&,
* ocpp::types::IdTagInfo&,
* std::string&,
Expand Down
Loading