buffet: Fix crash when CommandInstance::SetResults() is invoked The crash was a result of use-after-delete in CloudCommandProxy::OnResultsChanged() where TypedValueToJson returned std::unique_ptr while the JSON value was set to Dictionary by using unique_ptr::get() on the temporary. Should have called release() instead to transfer the ownership to the dictionary. Added a unit test to buffet/device_registration_info_unittest.cc to test this specific scenario. BUG=brillo:770 TEST=`FEATURES=test emerge-link buffet` Change-Id: I34a28ce610a06f2b77af1cc75e358b3fb3562a13 Reviewed-on: https://chromium-review.googlesource.com/264465 Trybot-Ready: Alex Vakulenko <avakulenko@chromium.org> Tested-by: Alex Vakulenko <avakulenko@chromium.org> Reviewed-by: Vitaly Buka <vitalybuka@chromium.org> Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/cloud_command_proxy.cc b/buffet/commands/cloud_command_proxy.cc index d3cd5a6..341401e 100644 --- a/buffet/commands/cloud_command_proxy.cc +++ b/buffet/commands/cloud_command_proxy.cc
@@ -22,7 +22,7 @@ void CloudCommandProxy::OnResultsChanged(const native_types::Object& results) { base::DictionaryValue patch; patch.Set(commands::attributes::kCommand_Results, - TypedValueToJson(results, nullptr).get()); + TypedValueToJson(results, nullptr).release()); device_registration_info_->UpdateCommand(command_instance_->GetID(), patch); }
diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc index afdd518..f7533ca 100644 --- a/buffet/commands/command_manager.cc +++ b/buffet/commands/command_manager.cc
@@ -26,6 +26,10 @@ command_queue_.SetCommandDispachInterface(&command_dispatcher_); } +CommandManager::CommandManager(CommandDispachInterface* dispatch_interface) { + command_queue_.SetCommandDispachInterface(dispatch_interface); +} + const CommandDictionary& CommandManager::GetCommandDictionary() const { return dictionary_; }
diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h index 8088e26..a703ab3 100644 --- a/buffet/commands/command_manager.h +++ b/buffet/commands/command_manager.h
@@ -36,6 +36,8 @@ explicit CommandManager( const base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager>& object_manager); + // Special constructor to help mock out command dispatcher for testing. + explicit CommandManager(CommandDispachInterface* dispatch_interface); // Sets callback which is called when command definitions is changed. void SetOnCommandDefChanged(const base::Closure& on_command_defs_changed) {
diff --git a/buffet/device_registration_info_unittest.cc b/buffet/device_registration_info_unittest.cc index 91dc8b1..6136eeb 100644 --- a/buffet/device_registration_info_unittest.cc +++ b/buffet/device_registration_info_unittest.cc
@@ -5,6 +5,7 @@ #include "buffet/device_registration_info.h" #include <base/json/json_reader.h> +#include <base/json/json_writer.h> #include <base/values.h> #include <chromeos/bind_lambda.h> #include <chromeos/http/http_request.h> @@ -179,6 +180,10 @@ static bool Save(DeviceRegistrationInfo* info) { return info->Save(); } + static void PublishCommands(DeviceRegistrationInfo* info, + const base::ListValue& commands) { + return info->PublishCommands(commands); + } }; class DeviceRegistrationInfoTest : public ::testing::Test { @@ -188,7 +193,7 @@ storage_ = std::make_shared<MemStorage>(); storage_->Save(&data_); transport_ = std::make_shared<chromeos::http::fake::Transport>(); - command_manager_ = std::make_shared<CommandManager>(); + command_manager_ = std::make_shared<CommandManager>(nullptr); state_manager_ = std::make_shared<StateManager>(&mock_state_change_queue_); chromeos::KeyValueStore config_store; config_store.SetString("client_id", test_data::kClientId); @@ -213,6 +218,8 @@ transport_, storage_, true, mock_callback)); + EXPECT_CALL(*this, OnRegistrationStatusChange()) + .Times(testing::AnyNumber()); } MOCK_METHOD0(OnRegistrationStatusChange, void()); @@ -503,4 +510,52 @@ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus()); } +TEST_F(DeviceRegistrationInfoTest, UpdateCommand) { + EXPECT_TRUE(dev_reg_->Load()); + auto json_cmds = buffet::unittests::CreateDictionaryValue(R"({ + 'robot': { + '_jump': { + 'parameters': {'_height': 'integer'}, + 'results': {'status': 'string'} + } + } + })"); + EXPECT_TRUE(command_manager_->LoadCommands(*json_cmds, "", nullptr)); + + auto commands_json = buffet::unittests::CreateValue(R"([{ + 'name':'robot._jump', + 'id':'1234', + 'parameters': {'_height': 100} + }])"); + ASSERT_NE(nullptr, commands_json.get()); + const base::ListValue* command_list = nullptr; + ASSERT_TRUE(commands_json->GetAsList(&command_list)); + DeviceRegistrationInfo::TestHelper::PublishCommands(dev_reg_.get(), + *command_list); + auto command = command_manager_->FindCommand("1234"); + ASSERT_NE(nullptr, command); + StringPropType string_type; + native_types::Object results{ + {"status", string_type.CreateValue(std::string{"Ok"}, nullptr)} + }; + + auto update_command = [](const ServerRequest& request, + ServerResponse* response) { + // Make sure we serialize the JSON back without any pretty print so + // the string comparison works correctly. + auto json = request.GetDataAsJson(); + EXPECT_NE(nullptr, json.get()); + std::string value; + ASSERT_TRUE(base::JSONWriter::Write(json.get(), &value)); + EXPECT_EQ(R"({"results":{"status":"Ok"}})", value); + }; + + transport_->AddHandler(dev_reg_->GetServiceURL("commands/1234"), + chromeos::http::request_type::kPatch, + base::Bind(update_command)); + + command->SetResults(results); +} + + } // namespace buffet
diff --git a/buffet/utils.cc b/buffet/utils.cc index 9cd7271..8e29ebe 100644 --- a/buffet/utils.cc +++ b/buffet/utils.cc
@@ -20,7 +20,6 @@ std::unique_ptr<const base::DictionaryValue> LoadJsonDict( const base::FilePath& json_file_path, chromeos::ErrorPtr* error) { - std::unique_ptr<const base::DictionaryValue> result; std::string json_string; if (!base::ReadFileToString(json_file_path, &json_string)) { chromeos::errors::system::AddSystemError(error, FROM_HERE, errno); @@ -28,8 +27,14 @@ kFileReadError, "Failed to read file '%s'", json_file_path.value().c_str()); - return result; + return {}; } + return LoadJsonDict(json_string, error); +} + +std::unique_ptr<const base::DictionaryValue> LoadJsonDict( + const std::string& json_string, chromeos::ErrorPtr* error) { + std::unique_ptr<const base::DictionaryValue> result; std::string error_message; base::Value* value = base::JSONReader::ReadAndReturnError( json_string, base::JSON_PARSE_RFC, nullptr, &error_message); @@ -37,8 +42,8 @@ chromeos::Error::AddToPrintf(error, FROM_HERE, chromeos::errors::json::kDomain, chromeos::errors::json::kParseError, - "Error parsing content of JSON file '%s': %s", - json_file_path.value().c_str(), + "Error parsing JSON string '%s': %s", + json_string.c_str(), error_message.c_str()); return result; } @@ -48,8 +53,8 @@ chromeos::Error::AddToPrintf(error, FROM_HERE, chromeos::errors::json::kDomain, chromeos::errors::json::kObjectExpected, - "Content of file '%s' is not a JSON object", - json_file_path.value().c_str()); + "JSON string '%s' is not a JSON object", + json_string.c_str()); return result; } result.reset(dict_value);
diff --git a/buffet/utils.h b/buffet/utils.h index a776e0c..32738d2 100644 --- a/buffet/utils.h +++ b/buffet/utils.h
@@ -6,6 +6,7 @@ #define BUFFET_UTILS_H_ #include <memory> +#include <string> #include <base/values.h> #include <base/files/file_path.h> @@ -32,6 +33,10 @@ std::unique_ptr<const base::DictionaryValue> LoadJsonDict( const base::FilePath& json_file_path, chromeos::ErrorPtr* error); +// Helper function to load a JSON dictionary from a string. +std::unique_ptr<const base::DictionaryValue> LoadJsonDict( + const std::string& json_string, chromeos::ErrorPtr* error); + } // namespace buffet #endif // BUFFET_UTILS_H_