Skip to content

Commit ab41234

Browse files
c-jimenezc-jimenez
committed
[messages] Fix race condition on allocators used in messages converters
1 parent 8a88ba3 commit ab41234

19 files changed

+108
-40
lines changed

‎src/centralsystem/chargepoint/ChargePointHandler.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,9 @@ class ChargePointHandler
263263
template <typename RequestType, typename ResponseType>
264264
ocpp::types::DataTransferStatus handleMessage(const std::string& type_id, const std::string& request_data, std::string& response_data)
265265
{
266-
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
267-
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
268-
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
266+
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
267+
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
268+
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
269269
try
270270
{
271271
// Parse JSON

‎src/centralsystem/chargepoint/ChargePointProxy.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,8 @@ class ChargePointProxy : public ICentralSystem::IChargePoint, public ocpp::rpc::
341341
bool ret = false;
342342

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

347347
// Prepare request
348348
ocpp::messages::DataTransferReq req;

‎src/chargepoint/iso15118/Iso15118Manager.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,8 @@ class Iso15118Manager : public IDataTransferManager::IDataTransferHandler
175175
bool ret = false;
176176

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

181181
// Prepare request
182182
ocpp::messages::DataTransferReq req;
@@ -240,9 +240,9 @@ class Iso15118Manager : public IDataTransferManager::IDataTransferHandler
240240
template <typename RequestType, typename ResponseType>
241241
ocpp::types::DataTransferStatus handle(const std::string& type_id, const std::string& request_data, std::string& response_data)
242242
{
243-
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
244-
ocpp::messages::IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
245-
ocpp::messages::IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
243+
ocpp::types::DataTransferStatus status = ocpp::types::DataTransferStatus::Rejected;
244+
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(type_id);
245+
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(type_id);
246246
try
247247
{
248248
// Parse JSON

‎src/chargepoint/security/SecurityManager.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ SecurityManager::SecurityManager(const ocpp::config::IChargePointConfig&
9898
m_worker_pool(worker_pool),
9999
m_requests_fifo(requests_fifo),
100100
m_security_event_req_converter(
101-
*messages_converter.getRequestConverter<SecurityEventNotificationReq>(SECURITY_EVENT_NOTIFICATION_ACTION)),
101+
messages_converter.getRequestConverter<SecurityEventNotificationReq>(SECURITY_EVENT_NOTIFICATION_ACTION)),
102102
m_charge_point(charge_point),
103103
m_security_logs_db(stack_config, database),
104104
m_ca_certificates_db(stack_config, database),
@@ -319,8 +319,8 @@ bool SecurityManager::logSecurityEvent(const std::string& type, const std::strin
319319
// Stack is not started, queue the notification
320320
rapidjson::Document payload;
321321
payload.Parse("{}");
322-
m_security_event_req_converter.setAllocator(&payload.GetAllocator());
323-
if (m_security_event_req_converter.toJson(request, payload))
322+
m_security_event_req_converter->setAllocator(&payload.GetAllocator());
323+
if (m_security_event_req_converter->toJson(request, payload))
324324
{
325325
m_requests_fifo.push(0, SECURITY_EVENT_NOTIFICATION_ACTION, payload);
326326
}

‎src/chargepoint/security/SecurityManager.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ class SecurityManager
190190
/** @brief Transaction related requests FIFO */
191191
ocpp::messages::IRequestFifo& m_requests_fifo;
192192
/** @brief Message converter for SecurityEventNotificationReq */
193-
ocpp::messages::IMessageConverter<ocpp::messages::SecurityEventNotificationReq>& m_security_event_req_converter;
193+
std::unique_ptr<ocpp::messages::IMessageConverter<ocpp::messages::SecurityEventNotificationReq>> m_security_event_req_converter;
194194
/** @brief Charge Point */
195195
IChargePoint& m_charge_point;
196196

‎src/messages/GenericMessageHandler.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ class GenericMessageHandler : public IMessageDispatcher::IMessageHandler
3535
public:
3636
/** @brief Constructor */
3737
GenericMessageHandler(const std::string& action, const GenericMessagesConverter& messages_converter)
38-
: m_request_converter(*messages_converter.getRequestConverter<RequestType>(action)),
39-
m_response_converter(*messages_converter.getResponseConverter<ResponseType>(action))
38+
: m_request_converter(messages_converter.getRequestConverter<RequestType>(action)),
39+
m_response_converter(messages_converter.getResponseConverter<ResponseType>(action))
4040
{
4141
}
4242

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

6161
// Convert request
6262
RequestType request;
63-
if (m_request_converter.fromJson(payload, request, error_code, error_message))
63+
if (m_request_converter->fromJson(payload, request, error_code, error_message))
6464
{
6565
// Handle message
6666
ResponseType resp;
6767
if (handleMessage(request, resp, error_code, error_message))
6868
{
6969
// Convert response
70-
m_response_converter.setAllocator(&response.GetAllocator());
71-
ret = m_response_converter.toJson(resp, response);
70+
m_response_converter->setAllocator(&response.GetAllocator());
71+
ret = m_response_converter->toJson(resp, response);
7272
}
7373
}
7474

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

8888
private:
89-
IMessageConverter<RequestType>& m_request_converter;
90-
IMessageConverter<ResponseType>& m_response_converter;
89+
std::unique_ptr<IMessageConverter<RequestType>> m_request_converter;
90+
std::unique_ptr<IMessageConverter<ResponseType>> m_response_converter;
9191
};
9292

9393
} // namespace messages

‎src/messages/GenericMessageSender.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ class GenericMessageSender
129129
CallResult ret = CallResult::Failed;
130130

131131
// Get converters
132-
IMessageConverter<RequestType>* req_converter = m_messages_converter.getRequestConverter<RequestType>(action);
133-
IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
132+
auto req_converter = m_messages_converter.getRequestConverter<RequestType>(action);
133+
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
134134
if (req_converter && resp_converter)
135135
{
136136
// Convert request
@@ -204,7 +204,7 @@ class GenericMessageSender
204204
CallResult ret = CallResult::Failed;
205205

206206
// Get converter
207-
IMessageConverter<ResponseType>* resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
207+
auto resp_converter = m_messages_converter.getResponseConverter<ResponseType>(action);
208208
if (resp_converter)
209209
{
210210
// Execute call

‎src/messages/GenericMessagesConverter.h‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,10 @@ along with OpenOCPP. If not, see <http://www.gnu.org/licenses/>.
1919
#ifndef OPENOCPP_GENERICMESSAGESCONVERTER_H
2020
#define OPENOCPP_GENERICMESSAGESCONVERTER_H
2121

22+
#include <memory>
2223
#include <string>
2324
#include <unordered_map>
25+
2426
namespace ocpp
2527
{
2628
namespace messages
@@ -42,13 +44,13 @@ class GenericMessagesConverter
4244
* @return Pointer to the message converter for the request or nullptr if the converter doesn't exists
4345
*/
4446
template <typename RequestType>
45-
IMessageConverter<RequestType>* getRequestConverter(const std::string& action) const
47+
std::unique_ptr<IMessageConverter<RequestType>> getRequestConverter(const std::string& action) const
4648
{
47-
IMessageConverter<RequestType>* ret = nullptr;
48-
auto it = m_req_converters.find(action);
49+
std::unique_ptr<IMessageConverter<RequestType>> ret;
50+
auto it = m_req_converters.find(action);
4951
if (it != m_req_converters.end())
5052
{
51-
ret = reinterpret_cast<IMessageConverter<RequestType>*>(it->second);
53+
ret.reset(reinterpret_cast<IMessageConverter<RequestType>*>(it->second)->clone());
5254
}
5355
return ret;
5456
}
@@ -59,13 +61,13 @@ class GenericMessagesConverter
5961
* @return Pointer to the message converter for the response or nullptr if the converter doesn't exists
6062
*/
6163
template <typename ResponseType>
62-
IMessageConverter<ResponseType>* getResponseConverter(const std::string& action) const
64+
std::unique_ptr<IMessageConverter<ResponseType>> getResponseConverter(const std::string& action) const
6365
{
64-
IMessageConverter<ResponseType>* ret = nullptr;
65-
auto it = m_resp_converters.find(action);
66+
std::unique_ptr<IMessageConverter<ResponseType>> ret;
67+
auto it = m_resp_converters.find(action);
6668
if (it != m_resp_converters.end())
6769
{
68-
ret = reinterpret_cast<IMessageConverter<ResponseType>*>(it->second);
70+
ret.reset(reinterpret_cast<IMessageConverter<ResponseType>*>(it->second)->clone());
6971
}
7072
return ret;
7173
}
@@ -93,6 +95,34 @@ class GenericMessagesConverter
9395
m_resp_converters[action] = &converter;
9496
}
9597

98+
/**
99+
* @brief Delete a converter for a request
100+
* @param action Ocpp call action corresponding to the request
101+
*/
102+
template <typename RequestType>
103+
void deleteRequestConverter(const std::string& action)
104+
{
105+
auto it = m_req_converters.find(action);
106+
if (it != m_req_converters.end())
107+
{
108+
delete reinterpret_cast<IMessageConverter<RequestType>*>(it->second);
109+
}
110+
}
111+
112+
/**
113+
* @brief Delete a converter for a response
114+
* @param action Ocpp call action corresponding to the response
115+
*/
116+
template <typename ResponseType>
117+
void deleteResponseConverter(const std::string& action)
118+
{
119+
auto it = m_resp_converters.find(action);
120+
if (it != m_resp_converters.end())
121+
{
122+
delete reinterpret_cast<IMessageConverter<ResponseType>*>(it->second);
123+
}
124+
}
125+
96126
private:
97127
/** @brief Request converters */
98128
std::unordered_map<std::string, void*> m_req_converters;

‎src/messages/IMessageConverter.h‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ class IMessageConverter
3737
/** @brief Destructor */
3838
virtual ~IMessageConverter() { }
3939

40+
/**
41+
* Clone a message converter instance
42+
* @return Cloned message converter instance
43+
*/
44+
virtual IMessageConverter* clone() const = 0;
45+
4046
/**
4147
* @brief Convert a JSON object to a C++ data type
4248
* @param json JSON object to convert
@@ -273,17 +279,19 @@ class IMessageConverter
273279
class MessageType##ReqConverter : public IMessageConverter<MessageType##Req> \
274280
{ \
275281
public: \
282+
IMessageConverter<MessageType##Req>* clone() const override { return new MessageType##ReqConverter(); } \
276283
bool fromJson(const rapidjson::Value& json, MessageType##Req& data, std::string& error_code, std::string& error_message) override; \
277284
bool toJson(const MessageType##Req& data, rapidjson::Document& json) override; \
278285
}; \
279286
class MessageType##ConfConverter : public IMessageConverter<MessageType##Conf> \
280287
{ \
281288
public: \
282-
bool fromJson(const rapidjson::Value& json, \
283-
MessageType##Conf& data, \
284-
std::string& error_code, \
285-
std::string& error_message) override; \
286-
bool toJson(const MessageType##Conf& data, rapidjson::Document& json) override; \
289+
IMessageConverter<MessageType##Conf>* clone() const override { return new MessageType##ConfConverter(); } \
290+
bool fromJson(const rapidjson::Value& json, \
291+
MessageType##Conf& data, \
292+
std::string& error_code, \
293+
std::string& error_message) override; \
294+
bool toJson(const MessageType##Conf& data, rapidjson::Document& json) override; \
287295
};
288296

289297
} // namespace messages

‎src/messages/MessagesConverter.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ along with OpenOCPP. If not, see <http://www.gnu.org/licenses/>.
7070
registerResponseConverter<action##Conf>(#action, *new action##ConfConverter())
7171

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

7777
namespace ocpp
7878
{

0 commit comments

Comments
 (0)