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_
