From d42cfe8434ade15c75c07427f53a1cc70cd79c49 Mon Sep 17 00:00:00 2001 From: jacobkeeler Date: Thu, 29 Mar 2018 10:56:18 -0400 Subject: Fix EventDispatcher crash by rejecting duplicate correlation_ids --- src/components/application_manager/src/request_controller.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/application_manager/src/request_controller.cc b/src/components/application_manager/src/request_controller.cc index 1b9bd7ffb9..a6d78f24de 100644 --- a/src/components/application_manager/src/request_controller.cc +++ b/src/components/application_manager/src/request_controller.cc @@ -474,7 +474,15 @@ void RequestController::Worker::threadMain() { RequestInfoPtr request_info_ptr = utils::MakeShared(request_ptr, timeout_in_mseconds); - request_controller_->waiting_for_response_.Add(request_info_ptr); + if (!request_controller_->waiting_for_response_.Add(request_info_ptr)) { + commands::CommandRequestImpl* cmd_request = + dynamic_cast(request_ptr.get()); + if (cmd_request != NULL) { + cmd_request->SendResponse( + false, mobile_apis::Result::INVALID_ID, "Duplicate correlation_id"); + } + continue; + } LOG4CXX_DEBUG(logger_, "timeout_in_mseconds " << timeout_in_mseconds); if (0 != timeout_in_mseconds) { -- cgit v1.2.1 From b33bffd77d428d59612c9fc3267767d21542579d Mon Sep 17 00:00:00 2001 From: jacobkeeler Date: Thu, 29 Mar 2018 16:37:26 -0400 Subject: Add test case for duplicate correlation_ids --- .../request_controller/request_controller_test.cc | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/components/application_manager/test/request_controller/request_controller_test.cc b/src/components/application_manager/test/request_controller/request_controller_test.cc index e053d51c34..c2912fdc0b 100644 --- a/src/components/application_manager/test/request_controller/request_controller_test.cc +++ b/src/components/application_manager/test/request_controller/request_controller_test.cc @@ -62,6 +62,7 @@ using ::application_manager::request_controller::RequestInfo; using ::testing::Return; using ::testing::ReturnRef; using ::testing::NiceMock; +using ::testing::_; typedef NiceMock MRequest; typedef utils::SharedPtr RequestPtr; @@ -114,6 +115,7 @@ class RequestControllerTestClass : public ::testing::Test { RequestPtr output = utils::MakeShared(connection_key, correlation_id); ON_CALL(*output, default_timeout()).WillByDefault(Return(default_timeout)); + ON_CALL(*output, CheckPermissions()).WillByDefault(Return(true)); return output; } @@ -158,6 +160,40 @@ class RequestControllerTestClass : public ::testing::Test { const TestSettings default_settings_; }; +TEST_F(RequestControllerTestClass, + AddMobileRequest_DuplicateCorrelationId_INVALID_ID) { + RequestPtr request_valid = GetMockRequest(); + TestAsyncWaiter waiter_valid; + ON_CALL(*request_valid, default_timeout()).WillByDefault(Return(0)); + EXPECT_CALL(*request_valid, Init()).WillOnce(Return(true)); + EXPECT_CALL(*request_valid, Run()) + .Times(1) + .WillRepeatedly(NotifyTestAsyncWaiter(&waiter_valid)); + + EXPECT_EQ(RequestController::SUCCESS, + AddRequest(default_settings_, + request_valid, + RequestInfo::RequestType::MobileRequest, + mobile_apis::HMILevel::HMI_NONE)); + EXPECT_TRUE(waiter_valid.WaitFor(1, 1000)); + + // The command should not be run if another command with the same + // correlation_id is waiting for a response + RequestPtr request_dup_corr_id = GetMockRequest(); + TestAsyncWaiter waiter_dup; + ON_CALL(*request_dup_corr_id, default_timeout()).WillByDefault(Return(0)); + EXPECT_CALL(*request_dup_corr_id, Init()).WillOnce(Return(true)); + ON_CALL(*request_dup_corr_id, Run()) + .WillByDefault(NotifyTestAsyncWaiter(&waiter_dup)); + + EXPECT_EQ(RequestController::SUCCESS, + AddRequest(default_settings_, + request_dup_corr_id, + RequestInfo::RequestType::MobileRequest, + mobile_apis::HMILevel::HMI_NONE)); + EXPECT_FALSE(waiter_dup.WaitFor(1, 1000)); +} + TEST_F(RequestControllerTestClass, CheckPosibilitytoAdd_ZeroValueLimiters_SUCCESS) { // Test case than pending_requests_amount, -- cgit v1.2.1 From 028ca237522c1c1693ca54049cf462a5122598e4 Mon Sep 17 00:00:00 2001 From: jacobkeeler Date: Mon, 14 May 2018 13:55:32 -0400 Subject: Prevent `INVALID_ID`responses from terminating valid requests --- .../application_manager/request_controller.h | 7 ++++++ .../application_manager/src/request_controller.cc | 25 ++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/components/application_manager/include/application_manager/request_controller.h b/src/components/application_manager/include/application_manager/request_controller.h index d3a5a0b821..c0bae1aac8 100644 --- a/src/components/application_manager/include/application_manager/request_controller.h +++ b/src/components/application_manager/include/application_manager/request_controller.h @@ -289,6 +289,13 @@ class RequestController { */ std::list notification_list_; + /** + * @brief Map keeping track of how many duplicate messages were sent for a + * given correlation id, to prevent early termination of a request + */ + std::map duplicate_message_count_; + sync_primitives::Lock duplicate_message_count_lock_; + /* * timer for checking requests timeout */ diff --git a/src/components/application_manager/src/request_controller.cc b/src/components/application_manager/src/request_controller.cc index a6d78f24de..f341967842 100644 --- a/src/components/application_manager/src/request_controller.cc +++ b/src/components/application_manager/src/request_controller.cc @@ -50,6 +50,7 @@ RequestController::RequestController(const RequestControlerSettings& settings) : pool_state_(UNDEFINED) , pool_size_(settings.thread_pool_size()) , request_tracker_(settings) + , duplicate_message_count_() , timer_("AM RequestCtrlTimer", new timer::TimerTaskImpl( this, &RequestController::TimeoutThread)) @@ -230,6 +231,21 @@ void RequestController::TerminateRequest(const uint32_t correlation_id, << correlation_id << " connection_key = " << connection_key << " function_id = " << function_id << " force_terminate = " << force_terminate); + { + AutoLock auto_lock(duplicate_message_count_lock_); + auto dup_it = duplicate_message_count_.find(correlation_id); + if (duplicate_message_count_.end() != dup_it) { + duplicate_message_count_[correlation_id]--; + if (0 == duplicate_message_count_[correlation_id]) { + duplicate_message_count_.erase(dup_it); + } + LOG4CXX_DEBUG(logger_, + "Ignoring termination request due to duplicate correlation " + "ID being sent"); + return; + } + } + RequestInfoPtr request = waiting_for_response_.Find(connection_key, correlation_id); if (!request) { @@ -478,6 +494,15 @@ void RequestController::Worker::threadMain() { commands::CommandRequestImpl* cmd_request = dynamic_cast(request_ptr.get()); if (cmd_request != NULL) { + uint32_t corr_id = cmd_request->correlation_id(); + request_controller_->duplicate_message_count_lock_.Acquire(); + auto dup_it = + request_controller_->duplicate_message_count_.find(corr_id); + if (request_controller_->duplicate_message_count_.end() == dup_it) { + request_controller_->duplicate_message_count_[corr_id] = 0; + } + request_controller_->duplicate_message_count_[corr_id]++; + request_controller_->duplicate_message_count_lock_.Release(); cmd_request->SendResponse( false, mobile_apis::Result::INVALID_ID, "Duplicate correlation_id"); } -- cgit v1.2.1