From 2dc0bc36767a453732b2bdc45d1b5a44cc531708 Mon Sep 17 00:00:00 2001 From: mkamonMdt Date: Thu, 21 Jan 2021 13:53:12 +0100 Subject: [PATCH] [EGD-4850] Fix of statics between USB CDC and ServiceDesktop Handling queues with static std::string implementation caused problem of double-free memory on turning off simulator. Applied solution to the problem is to pass ownership of queued string to a receiver side. --- module-bsp/board/linux/usb_cdc/usb_cdc.cpp | 3 +- .../service-desktop/ServiceDesktop.cpp | 2 +- .../service-desktop/WorkerDesktop.cpp | 11 ++--- .../service-desktop/parser/MessageHandler.cpp | 6 +-- .../service-desktop/parser/MessageHandler.hpp | 25 +++++----- .../service-desktop/parser/ParserFSM.cpp | 46 +++++++++---------- .../service-desktop/parser/ParserFSM.hpp | 12 ++--- .../service-desktop/tests/unittest.cpp | 46 ++++++++++--------- 8 files changed, 78 insertions(+), 73 deletions(-) diff --git a/module-bsp/board/linux/usb_cdc/usb_cdc.cpp b/module-bsp/board/linux/usb_cdc/usb_cdc.cpp index c9223e463db29db2d018965d718a813feae2ac9c..163d0e6edef9963e130fafcf585bde59f3752bf8 100644 --- a/module-bsp/board/linux/usb_cdc/usb_cdc.cpp +++ b/module-bsp/board/linux/usb_cdc/usb_cdc.cpp @@ -32,7 +32,6 @@ namespace bsp { LOG_INFO("[ServiceDesktop:BSP_Driver] Start reading on fd:%d", fd); char inputData[SERIAL_BUFFER_LEN]; - static std::string receiveMsg; while (1) { if (uxQueueSpacesAvailable(USBReceiveQueue) != 0) { @@ -61,7 +60,7 @@ namespace bsp continue; } #endif - receiveMsg = std::string(inputData, inputData + length); + std::string *receiveMsg = new std::string(inputData, inputData + length); xQueueSend(USBReceiveQueue, &receiveMsg, portMAX_DELAY); } else { diff --git a/module-services/service-desktop/ServiceDesktop.cpp b/module-services/service-desktop/ServiceDesktop.cpp index 8d6dd71b2bf2e9c103138227b667d9937b0b2ce5..61d54278372982aa5fb1f1c8368774c9bb6f265a 100644 --- a/module-services/service-desktop/ServiceDesktop.cpp +++ b/module-services/service-desktop/ServiceDesktop.cpp @@ -42,7 +42,7 @@ sys::ReturnCodes ServiceDesktop::InitHandler() { desktopWorker = std::make_unique(this); const bool ret = desktopWorker->init( - {{sdesktop::RECEIVE_QUEUE_BUFFER_NAME, sizeof(std::string), sdesktop::cdc_queue_len}, + {{sdesktop::RECEIVE_QUEUE_BUFFER_NAME, sizeof(std::string *), sdesktop::cdc_queue_len}, {sdesktop::SEND_QUEUE_BUFFER_NAME, sizeof(std::string *), sdesktop::cdc_queue_object_size}}); if (ret == false) { diff --git a/module-services/service-desktop/WorkerDesktop.cpp b/module-services/service-desktop/WorkerDesktop.cpp index f6cd9ba2a0a6b33dc76f5974bc3dfb0fe1aba306..b377d1a899bb23c05767bf77aad2bf8e1882ffde 100644 --- a/module-services/service-desktop/WorkerDesktop.cpp +++ b/module-services/service-desktop/WorkerDesktop.cpp @@ -30,7 +30,7 @@ bool WorkerDesktop::init(std::list queues) Worker::init(queues); receiveQueue = Worker::getQueueHandleByName(sdesktop::RECEIVE_QUEUE_BUFFER_NAME); - parserFSM::MessageHandler::sendQueue = Worker::getQueueHandleByName(sdesktop::SEND_QUEUE_BUFFER_NAME); + parserFSM::MessageHandler::setSendQueueHandle(Worker::getQueueHandleByName(sdesktop::SEND_QUEUE_BUFFER_NAME)); return (bsp::usbInit(receiveQueue, this) < 0) ? false : true; } @@ -58,16 +58,17 @@ bool WorkerDesktop::handleMessage(uint32_t queueID) LOG_INFO("handleMessage received data from queue: %s", qname.c_str()); - static std::string *sendMsg = nullptr; - static std::string receivedMsg; if (qname == sdesktop::RECEIVE_QUEUE_BUFFER_NAME) { + std::string *receivedMsg = nullptr; if (!queue->Dequeue(&receivedMsg, 0)) { LOG_ERROR("handleMessage failed to receive from \"%s\"", sdesktop::RECEIVE_QUEUE_BUFFER_NAME); return false; } - parser.processMessage(receivedMsg); + parser.processMessage(std::move(*receivedMsg)); + delete receivedMsg; } else if (qname == sdesktop::SEND_QUEUE_BUFFER_NAME) { + std::string *sendMsg = nullptr; if (!queue->Dequeue(&sendMsg, 0)) { LOG_ERROR("handleMessage xQueueReceive failed for %s.", sdesktop::SEND_QUEUE_BUFFER_NAME); return false; @@ -75,9 +76,7 @@ bool WorkerDesktop::handleMessage(uint32_t queueID) LOG_DEBUG("handeMessage sending %d bytes using usbCDCSend", static_cast(sendMsg->length())); bsp::usbCDCSend(sendMsg); - delete sendMsg; - sendMsg = nullptr; } else { LOG_INFO("handeMessage got message on an unhandled queue"); diff --git a/module-services/service-desktop/parser/MessageHandler.cpp b/module-services/service-desktop/parser/MessageHandler.cpp index 573a7bf87a43051ce4760fcf4393662b857e188d..2b7b552b5f134a355b1ee593da4dd0c1f22be990 100644 --- a/module-services/service-desktop/parser/MessageHandler.cpp +++ b/module-services/service-desktop/parser/MessageHandler.cpp @@ -23,7 +23,7 @@ using namespace parserFSM; xQueueHandle MessageHandler::sendQueue; -MessageHandler::MessageHandler(std::string &message, sys::Service *OwnerService) : OwnerServicePtr(OwnerService) +MessageHandler::MessageHandler(const std::string &message, sys::Service *OwnerService) : OwnerServicePtr(OwnerService) { try { messageJson = json11::Json::parse(message, JsonErrorMsg); @@ -53,10 +53,10 @@ void MessageHandler::processMessage() } } -void MessageHandler::putToSendQueue(const std::string msg) +void MessageHandler::putToSendQueue(const std::string &msg) { if (uxQueueSpacesAvailable(sendQueue) != 0) { - auto *responseString = new std::string(msg); + auto responseString = new std::string(msg); xQueueSend(sendQueue, &responseString, portMAX_DELAY); } } diff --git a/module-services/service-desktop/parser/MessageHandler.hpp b/module-services/service-desktop/parser/MessageHandler.hpp index 191500dbfb7f23fb8994c101adfa7e0400e0e1e0..765fdec84310dc7e920a5312d675811dd8ecd934 100644 --- a/module-services/service-desktop/parser/MessageHandler.hpp +++ b/module-services/service-desktop/parser/MessageHandler.hpp @@ -24,29 +24,32 @@ namespace parserFSM { class MessageHandler { + static xQueueHandle sendQueue; + + json11::Json messageJson; + std::string JsonErrorMsg; + sys::Service *OwnerServicePtr = nullptr; public: - MessageHandler(std::string &message, sys::Service *OwnerService); - static xQueueHandle sendQueue; + MessageHandler(const std::string &message, sys::Service *OwnerService); - bool isJSONNull() + [[nodiscard]] auto isJSONNull() const -> bool { return messageJson.is_null(); }; - bool isValid() + [[nodiscard]] auto isValid() const noexcept -> bool { return JsonErrorMsg.empty(); } - std::string &getErrorString() + [[nodiscard]] auto getErrorString() const -> const std::string & { return JsonErrorMsg; }; void processMessage(); - static void putToSendQueue(const std::string msg); - - private: - json11::Json messageJson; - std::string JsonErrorMsg; - sys::Service *OwnerServicePtr = nullptr; + static void putToSendQueue(const std::string &msg); + static void setSendQueueHandle(xQueueHandle handle) + { + sendQueue = handle; + } }; } // namespace parserFSM diff --git a/module-services/service-desktop/parser/ParserFSM.cpp b/module-services/service-desktop/parser/ParserFSM.cpp index 226174d39d993af8616ee35a69b47eaf26b08f51..59e7d4a39b65dcfdc78c82b83417617bdfff13ae 100644 --- a/module-services/service-desktop/parser/ParserFSM.cpp +++ b/module-services/service-desktop/parser/ParserFSM.cpp @@ -28,10 +28,10 @@ using namespace parserFSM; StateMachine::StateMachine(sys::Service *OwnerService) : OwnerServicePtr(OwnerService) {} -void StateMachine::processMessage(std::string &msg) +void StateMachine::processMessage(std::string &&msg) { - receivedMsgPtr = &msg; - LOG_DEBUG("Msg: %s", receivedMsgPtr->c_str()); + receivedMsg = std::move(msg); + LOG_DEBUG("Msg: %s", receivedMsg.c_str()); switch (state) { case State::ReceivedPayload: @@ -58,20 +58,20 @@ void StateMachine::parseHeader() header.clear(); payloadLength = 0; - auto messageStart = receivedMsgPtr->find(message::endpointChar); + auto messageStart = receivedMsg.find(message::endpointChar); if (messageStart == std::string::npos) { - LOG_ERROR("This is not a valid endpoint message! Type=%c", receivedMsgPtr->at(0)); + LOG_ERROR("This is not a valid endpoint message! Type=%c", receivedMsg.at(0)); return; } - if (receivedMsgPtr->size() < message::size_header) // header divided in few parts + if (receivedMsg.size() < message::size_header) // header divided in few parts { state = State::ReceivedPartialHeader; - header.append(*receivedMsgPtr); // append to whole header string + header.append(receivedMsg); // append to whole header string return; } - header = message::getHeader(*receivedMsgPtr); + header = message::getHeader(receivedMsg); payloadLength = message::calcPayloadLength(header); if (payloadLength == 0) // failed to obtain payload length from msg { @@ -82,7 +82,7 @@ void StateMachine::parseHeader() LOG_DEBUG("Payload length: %lu", payloadLength); - message::removeHeader(*receivedMsgPtr); + message::removeHeader(receivedMsg); parseNewMessage(); } @@ -91,39 +91,39 @@ void StateMachine::parsePartialHeader() auto previousHeaderLength = header.size(); auto missingHeaderLength = message::size_header - previousHeaderLength; - if (receivedMsgPtr->size() >= missingHeaderLength) // rest of the message is here + if (receivedMsg.size() >= missingHeaderLength) // rest of the message is here { - header.append(receivedMsgPtr->substr(0, missingHeaderLength)); + header.append(receivedMsg.substr(0, missingHeaderLength)); LOG_DEBUG("Header: %s\n", header.c_str()); payloadLength = message::calcPayloadLength(header); LOG_DEBUG("Payload length: %lu\n", payloadLength); - message::eraseFront(*receivedMsgPtr, missingHeaderLength); + message::eraseFront(receivedMsg, missingHeaderLength); parseNewMessage(); } else // the message is even longer :( { - header.append(*receivedMsgPtr); + header.append(receivedMsg); } } void StateMachine::parseNewMessage() { - if (receivedMsgPtr->size() >= payloadLength) { + if (receivedMsg.size() >= payloadLength) { - payload = message::extractPayload(*receivedMsgPtr, payloadLength); + payload = message::extractPayload(receivedMsg, payloadLength); parsePayload(); - if (receivedMsgPtr->size() > payloadLength) { // contains part of new header - message::eraseFront(*receivedMsgPtr, payloadLength); + if (receivedMsg.size() > payloadLength) { // contains part of new header + message::eraseFront(receivedMsg, payloadLength); parseHeader(); } } else // message divided in 2 or more packets { - payload = receivedMsgPtr->substr(0, std::string::npos); // take rest of the message + payload = receivedMsg.substr(0, std::string::npos); // take rest of the message state = State::ReceivedPartialPayload; } } @@ -133,20 +133,20 @@ void StateMachine::parsePartialMessage() auto previousPayloadLength = payload.size(); auto missingPayloadLength = payloadLength - previousPayloadLength; - if (receivedMsgPtr->size() >= missingPayloadLength) // rest of the message is here + if (receivedMsg.size() >= missingPayloadLength) // rest of the message is here { - payload.append(message::extractPayload(*receivedMsgPtr, missingPayloadLength)); + payload.append(message::extractPayload(receivedMsg, missingPayloadLength)); parsePayload(); - if (receivedMsgPtr->size() > missingPayloadLength) { - message::eraseFront(*receivedMsgPtr, missingPayloadLength); + if (receivedMsg.size() > missingPayloadLength) { + message::eraseFront(receivedMsg, missingPayloadLength); parseHeader(); } } else // the message is even longer { - payload.append(*receivedMsgPtr); + payload.append(receivedMsg); } } diff --git a/module-services/service-desktop/parser/ParserFSM.hpp b/module-services/service-desktop/parser/ParserFSM.hpp index 19784297948365aabc82653684675458e4e8c0a2..6661808e36c187aa818438ad1f6be5a28cdae4b1 100644 --- a/module-services/service-desktop/parser/ParserFSM.hpp +++ b/module-services/service-desktop/parser/ParserFSM.hpp @@ -23,21 +23,21 @@ namespace parserFSM class StateMachine { public: - StateMachine(sys::Service *OwnerService); - void processMessage(std::string &msg); - State getCurrentState() + explicit StateMachine(sys::Service *OwnerService); + void processMessage(std::string &&msg); + [[nodiscard]] auto getCurrentState() const noexcept -> State { return state; }; - void setState(const parserFSM::State newState) + void setState(const parserFSM::State newState) noexcept { state = newState; } private: - std::string *receivedMsgPtr = nullptr; - parserFSM::State state = State::NoMsg; + std::string receivedMsg; + parserFSM::State state = State::NoMsg; std::string payload; std::string header; unsigned long payloadLength = 0; diff --git a/module-services/service-desktop/tests/unittest.cpp b/module-services/service-desktop/tests/unittest.cpp index af522aa2cd365437948d4ab3fc2fefc56d0c6df2..7534a194a1c4f8b1e7e892878b126a636cbe7b92 100644 --- a/module-services/service-desktop/tests/unittest.cpp +++ b/module-services/service-desktop/tests/unittest.cpp @@ -68,76 +68,80 @@ using namespace parserFSM; TEST_CASE("Parser Test") { StateMachine parser(nullptr); - std::string testMessage("#00000"); SECTION("Parse message with divided header and payload") { - parser.processMessage(testMessage); + std::string testMessage("#00000"); + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPartialHeader); testMessage = R"(0050{"endpo)"; - parser.processMessage(testMessage); + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPartialPayload); testMessage = R"(int":1, "method":1, "body":{"test":"test"}})"; - parser.processMessage(testMessage); + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPayload); } SECTION("Parse whole message") { - testMessage = R"(#000000050{"endpoint":1, "method":1, "body":{"test":"test"}})"; - parser.processMessage(testMessage); + std::string testMessage = R"(#000000050{"endpoint":1, "method":1, "body":{"test":"test"}})"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPayload); } SECTION("Parse message with start char detached from mesage") { - testMessage = R"(#)"; - parser.processMessage(testMessage); + std::string testMessage = R"(#)"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPartialHeader); testMessage = R"(000000050{"en)"; - parser.processMessage(testMessage); + testMessage = R"(000000050{"en)"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPartialPayload); testMessage = R"(dpoint":1, "method":1, "body":{"test":"test"}})"; - parser.processMessage(testMessage); + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPayload); } SECTION("Parse message with beginning of another one") { - testMessage = R"(#000000050{"endpoint":1, "method":1, "body":{"test":"test"}}#000000050{"end)"; - parser.processMessage(testMessage); + std::string testMessage = R"(#000000050{"endpoint":1, "method":1, "body":{"test":"test"}}#000000050{"end)"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPartialPayload); testMessage = R"(point":1, "method":1, "body":{"test":"test"}})"; - parser.processMessage(testMessage); + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::ReceivedPayload); } SECTION("Parse junk message") { - testMessage = R"({"address": "6 Czeczota St.\n02600 Warsaw"})"; - parser.processMessage(testMessage); + std::string testMessage = R"({"address": "6 Czeczota St.\n02600 Warsaw"})"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::NoMsg); } SECTION("Parse message with incorrect header length") { - testMessage = R"(#000000072{"endpoint":7, "method":2, "uuid":3, "body":{"threadID":1,"unread":false}})"; - parser.processMessage(testMessage); + std::string testMessage = + R"(#000000072{"endpoint":7, "method":2, "uuid":3, "body":{"threadID":1,"unread":false}})"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::NoMsg); } SECTION("Parse message with damaged json ") { - testMessage = R"(#000000074{"endpoint":7, "method":2, "uuid":3, "bo}}dy":{"threadID":1,"unread":false)"; - parser.processMessage(testMessage); + std::string testMessage = + R"(#000000074{"endpoint":7, "method":2, "uuid":3, "bo}}dy":{"threadID":1,"unread":false)"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::NoMsg); } SECTION("Parse message with damaged json and incorrect header length") { - testMessage = R"(#000000072{"endpoint":7, "method":2, "uuid":3, "bo}}dy":{"threadID":1,"unread":false)"; - parser.processMessage(testMessage); + std::string testMessage = + R"(#000000072{"endpoint":7, "method":2, "uuid":3, "bo}}dy":{"threadID":1,"unread":false)"; + parser.processMessage(std::move(testMessage)); REQUIRE(parser.getCurrentState() == State::NoMsg); } }