From 343f003df46a3abfbc3c88e0716c37c5a2638559 Mon Sep 17 00:00:00 2001 From: Sho Amano Date: Sun, 7 Jun 2020 03:19:43 +0900 Subject: Bugfix/protocol handler memory issues (#2270) * fix: memory leak in ProtocolHandlerImpl::NotifySessionStarted() This is detected by valgrind. * fix: buffer over-run in ProtocolPayloadTest This is an issue of unit test implementation and only affects unit testing. * fix: invalid memory accesses in ProtocolHandlerImplTest These affect unit testing only. Invalid memory accesses occurred because the mock class didn't configure protocol version field properly. * fix: valgrind warning in IncomingDataHandlerTest This is not actually an issue. valgrind reported a warning since the memory area was not initialized. Co-authored-by: Collin --- src/components/protocol_handler/src/protocol_handler_impl.cc | 6 +++++- .../protocol_handler/test/incoming_data_handler_test.cc | 12 +++++++++--- .../protocol_handler/test/protocol_handler_tm_test.cc | 4 ++-- .../protocol_handler/test/protocol_payload_test.cc | 5 +++-- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/components/protocol_handler/src/protocol_handler_impl.cc b/src/components/protocol_handler/src/protocol_handler_impl.cc index ace27fb307..b502a924af 100644 --- a/src/components/protocol_handler/src/protocol_handler_impl.cc +++ b/src/components/protocol_handler/src/protocol_handler_impl.cc @@ -1835,7 +1835,11 @@ void ProtocolHandlerImpl::NotifySessionStarted( } std::shared_ptr start_session_ack_params( - new BsonObject(), [](BsonObject* obj) { bson_object_deinitialize(obj); }); + new BsonObject(), + [](BsonObject* obj) { + bson_object_deinitialize(obj); + delete obj; + }); bson_object_initialize_default(start_session_ack_params.get()); // when video service is successfully started, copy input parameters // ("width", "height", "videoProtocol", "videoCodec") to the ACK packet diff --git a/src/components/protocol_handler/test/incoming_data_handler_test.cc b/src/components/protocol_handler/test/incoming_data_handler_test.cc index 3f1b28cbe2..c9bfa748dd 100644 --- a/src/components/protocol_handler/test/incoming_data_handler_test.cc +++ b/src/components/protocol_handler/test/incoming_data_handler_test.cc @@ -55,7 +55,14 @@ class IncomingDataHandlerTest : public ::testing::Test { some_data_size = 4; some_data2_size = 512; some_data = new uint8_t[some_data_size]; + for (size_t i = 0; i < some_data_size; i++) { + // put some data in the buffer + some_data[i] = i % 256; + } some_data2 = new uint8_t[some_data2_size]; + for (size_t i = 0; i < some_data2_size; i++) { + some_data2[i] = i % 256; + } protov1_message_id = 0x0; some_message_id = 0xABCDEF0; some_session_id = 0xFEDCBA0; @@ -254,9 +261,8 @@ TEST_F(IncomingDataHandlerTest, MixedPayloadData_TwoConnections) { for (FrameList::const_iterator it = actual_frames.begin(); it != actual_frames.end(); ++it, ++it_exp) { - // TODO(EZamakhov): investigate valgrind warning (unitialized value) - EXPECT_EQ(**it, **it_exp) - << "Element number " << std::distance(mobile_packets.begin(), it_exp); + EXPECT_EQ(**it, **it_exp) << "Element number " + << std::distance(mobile_packets.begin(), it_exp); } } diff --git a/src/components/protocol_handler/test/protocol_handler_tm_test.cc b/src/components/protocol_handler/test/protocol_handler_tm_test.cc index b87e31774b..312ec88930 100644 --- a/src/components/protocol_handler/test/protocol_handler_tm_test.cc +++ b/src/components/protocol_handler/test/protocol_handler_tm_test.cc @@ -3554,7 +3554,7 @@ TEST_F(ProtocolHandlerImplTest, // Expect check connection with ProtocolVersionUsed EXPECT_CALL(session_observer_mock, ProtocolVersionUsed(connection_id, session_id, _)) - .WillOnce(Return(true)); + .WillOnce(DoAll(SetArgReferee<2>(PROTOCOL_VERSION_1), Return(true))); // Expect send End Service EXPECT_CALL(transport_manager_mock, SendMessageToDevice(ExpectedMessage(FRAME_TYPE_CONTROL, @@ -3593,7 +3593,7 @@ TEST_F(ProtocolHandlerImplTest, SendHeartBeat_Successful) { // Expect check connection with ProtocolVersionUsed EXPECT_CALL(session_observer_mock, ProtocolVersionUsed(connection_id, session_id, _)) - .WillOnce(Return(true)); + .WillOnce(DoAll(SetArgReferee<2>(PROTOCOL_VERSION_3), Return(true))); // Expect send HeartBeat EXPECT_CALL( transport_manager_mock, diff --git a/src/components/protocol_handler/test/protocol_payload_test.cc b/src/components/protocol_handler/test/protocol_payload_test.cc index e89e6c7395..84f6e2b2c5 100644 --- a/src/components/protocol_handler/test/protocol_payload_test.cc +++ b/src/components/protocol_handler/test/protocol_payload_test.cc @@ -242,8 +242,9 @@ TEST(ProtocolPayloadTest, ExtractProtocolWithJSONWithDataWithWrongPayloadSize) { prot_payload_test.json = expect_output_json_string; prot_payload_test.header.json_size = prot_payload_test.json.length(); - const size_t data_for_sending_size = - PROTOCOL_HEADER_V2_SIZE + prot_payload_test.json.length(); + const size_t data_for_sending_size = PROTOCOL_HEADER_V2_SIZE + + prot_payload_test.json.length() + + prot_payload_test.data.size(); uint8_t* data_for_sending = new uint8_t[data_for_sending_size]; prepare_data(data_for_sending, prot_payload_test); -- cgit v1.2.1