crostini: Fix crash construction ContainerId

ContainerId constructed from a "bad" base::Value should no
longer crash. This change also cleans up the return type of
ContainerId::ToDictValue() for better safety/performance.

Fixed: 1323866
Change-Id: I38f9204b718e4d92eef967f7c3a2996a9c59a07f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3636960
Auto-Submit: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Christopher Lam <calamity@chromium.org>
Reviewed-by: David Munro <davidmunro@google.com>
Reviewed-by: Christopher Lam <calamity@chromium.org>
Commit-Queue: Fergus Dall <sidereal@google.com>
Reviewed-by: Fergus Dall <sidereal@google.com>
Cr-Commit-Position: refs/heads/main@{#1001332}

chrome/browser/ash/crostini/crostini_port_forwarder.cc

@@ -365,12 +365,10 @@ void CrostiniPortForwarder::RemoveAllPorts(const ContainerId& container_id) {
 base::ListValue CrostiniPortForwarder::GetActivePorts() {
   base::ListValue forwarded_ports_list;
   for (const auto& port : forwarded_ports_) {
-    base::Value port_info(base::Value::Type::DICTIONARY);
-    port_info.SetKey(kPortNumberKey, base::Value(port.first.port_number));
-    port_info.SetKey(kPortProtocolKey,
-                     base::Value(static_cast<int>(port.first.protocol_type)));
-    port_info.SetKey(kPortContainerIdKey,
-                     port.first.container_id.ToDictValue());
+    base::Value::Dict port_info;
+    port_info.Set(kPortNumberKey, port.first.port_number);
+    port_info.Set(kPortProtocolKey, static_cast<int>(port.first.protocol_type));
+    port_info.Set(kPortContainerIdKey, port.first.container_id.ToDictValue());
     forwarded_ports_list.Append(std::move(port_info));
   }
   return forwarded_ports_list;

chrome/browser/ash/crostini/crostini_terminal.cc

@@ -421,8 +421,8 @@ std::string ShortcutIdForSSH(const std::string& profileId) {
 }
 
 std::string ShortcutIdFromContainerId(const crostini::ContainerId& id) {
-  base::Value dict = id.ToDictValue();
-  dict.SetKey(kShortcutKey, base::Value(kShortcutValueTerminal));
+  base::Value::Dict dict = id.ToDictValue();
+  dict.Set(kShortcutKey, base::Value(kShortcutValueTerminal));
   std::string shortcut_id;
   base::JSONWriter::Write(dict, &shortcut_id);
   return shortcut_id;

chrome/browser/ash/crostini/crostini_util.cc

@@ -193,9 +193,14 @@ ContainerId::ContainerId(std::string vm_name,
                          std::string container_name) noexcept
     : vm_name(std::move(vm_name)), container_name(std::move(container_name)) {}
 
-ContainerId::ContainerId(const base::Value& dict) noexcept {
-  const std::string* vm = dict.FindStringKey(prefs::kVmKey);
-  const std::string* container = dict.FindStringKey(prefs::kContainerKey);
+ContainerId::ContainerId(const base::Value& value) noexcept {
+  const base::Value::Dict* dict = value.GetIfDict();
+  const std::string* vm = nullptr;
+  const std::string* container = nullptr;
+  if (dict != nullptr) {
+    vm = dict->FindString(prefs::kVmKey);
+    container = dict->FindString(prefs::kContainerKey);
+  }
   vm_name = vm ? *vm : "";
   container_name = container ? *container : "";
 }
@@ -207,10 +212,10 @@ base::flat_map<std::string, std::string> ContainerId::ToMap() const {
   return extras;
 }
 
-base::Value ContainerId::ToDictValue() const {
-  base::Value dict(base::Value::Type::DICTIONARY);
-  dict.SetStringKey(prefs::kVmKey, vm_name);
-  dict.SetStringKey(prefs::kContainerKey, container_name);
+base::Value::Dict ContainerId::ToDictValue() const {
+  base::Value::Dict dict;
+  dict.Set(prefs::kVmKey, vm_name);
+  dict.Set(prefs::kContainerKey, container_name);
   return dict;
 }
 

chrome/browser/ash/crostini/crostini_util.h

@@ -76,7 +76,7 @@ struct ContainerId {
   explicit ContainerId(const base::Value&) noexcept;
 
   base::flat_map<std::string, std::string> ToMap() const;
-  base::Value ToDictValue() const;
+  base::Value::Dict ToDictValue() const;
 
   static ContainerId GetDefault();
 

chrome/browser/ash/crostini/crostini_util_unittest.cc

@@ -120,6 +120,18 @@ TEST_F(CrostiniUtilTest, ContainerIdEquality) {
   ASSERT_FALSE(container2 == container3);
 }
 
+TEST_F(CrostiniUtilTest, ContainerIdFromDictValue) {
+  base::Value dict(base::Value::Type::DICT);
+  dict.SetStringKey(prefs::kVmKey, "foo");
+  dict.SetStringKey(prefs::kContainerKey, "bar");
+  EXPECT_TRUE(ContainerId(dict) == ContainerId("foo", "bar"));
+}
+
+TEST_F(CrostiniUtilTest, ContainerIdFromNonDictValue) {
+  base::Value non_dict("not a dict value");
+  EXPECT_TRUE(ContainerId(non_dict) == ContainerId("", ""));
+}
+
 TEST_F(CrostiniUtilTest, LaunchCallbackRunsOnRestartError) {
   // Set Restart to fail.
   fake_concierge_client_->set_start_vm_response({});
@@ -137,49 +149,46 @@ TEST_F(CrostiniUtilTest, LaunchCallbackRunsOnRestartError) {
 
 TEST_F(CrostiniUtilTest, DuplicateContainerNamesInPrefsAreRemoved) {
   ContainerId container1("test1", "test1");
-  base::Value dictionary1 = container1.ToDictValue();
-  dictionary1.SetKey(prefs::kContainerOsPrettyNameKey,
-                     base::Value("Test OS Name 1"));
-  dictionary1.SetKey(prefs::kContainerOsVersionKey, base::Value(1));
+  base::Value::Dict dictionary1 = container1.ToDictValue();
+  dictionary1.Set(prefs::kContainerOsPrettyNameKey, "Test OS Name 1");
+  dictionary1.Set(prefs::kContainerOsVersionKey, 1);
 
   ContainerId container2("test1", "test2");
-  base::Value dictionary2 = container2.ToDictValue();
-  dictionary2.SetKey(prefs::kContainerOsPrettyNameKey,
-                     base::Value("Test OS Name 2"));
-  dictionary2.SetKey(prefs::kContainerOsVersionKey, base::Value(2));
+  base::Value::Dict dictionary2 = container2.ToDictValue();
+  dictionary2.Set(prefs::kContainerOsPrettyNameKey, "Test OS Name 2");
+  dictionary2.Set(prefs::kContainerOsVersionKey, 2);
 
   ContainerId container3("test2", "test1");
-  base::Value dictionary3 = container3.ToDictValue();
-  dictionary3.SetKey(prefs::kContainerOsPrettyNameKey,
-                     base::Value("Test OS Name 3"));
-  dictionary3.SetKey(prefs::kContainerOsVersionKey, base::Value(3));
+  base::Value::Dict dictionary3 = container3.ToDictValue();
+  dictionary3.Set(prefs::kContainerOsPrettyNameKey, "Test OS Name 3");
+  dictionary3.Set(prefs::kContainerOsVersionKey, 3);
 
-  base::Value containers(base::Value::Type::LIST);
+  base::Value::List containers;
   containers.Append(dictionary1.Clone());
   containers.Append(dictionary2.Clone());
   containers.Append(dictionary1.Clone());
   containers.Append(dictionary2.Clone());
   containers.Append(dictionary3.Clone());
 
   PrefService* prefs = profile_->GetPrefs();
-  prefs->Set(prefs::kCrostiniContainers, std::move(containers));
+  prefs->SetList(prefs::kCrostiniContainers, std::move(containers));
 
   RemoveDuplicateContainerEntries(prefs);
 
   const base::Value::List& result =
       prefs->Get(prefs::kCrostiniContainers)->GetList();
 
   ASSERT_EQ(result.size(), 3);
-  EXPECT_EQ(result[0], dictionary1);
-  EXPECT_EQ(result[1], dictionary2);
-  EXPECT_EQ(result[2], dictionary3);
+  EXPECT_EQ(result[0].GetDict(), dictionary1);
+  EXPECT_EQ(result[1].GetDict(), dictionary2);
+  EXPECT_EQ(result[2].GetDict(), dictionary3);
 }
 
 TEST_F(CrostiniUtilTest, ShouldStopVm) {
   CrostiniManager* manager = CrostiniManager::GetForProfile(profile_.get());
   ContainerId containera("apple", "banana");
   ContainerId containerb("potato", "strawberry");
-  base::Value containers(base::Value::Type::LIST);
+  base::Value::List containers;
   containers.Append(containera.ToDictValue().Clone());
   containers.Append(containerb.ToDictValue().Clone());
 
@@ -193,7 +202,8 @@ TEST_F(CrostiniUtilTest, ShouldStopVm) {
   ASSERT_TRUE(manager->IsVmRunning("apple"));
   ASSERT_TRUE(manager->IsVmRunning("potato"));
 
-  profile_->GetPrefs()->Set(prefs::kCrostiniContainers, std::move(containers));
+  profile_->GetPrefs()->SetList(prefs::kCrostiniContainers,
+                                std::move(containers));
 
   EXPECT_TRUE(ShouldStopVm(profile_.get(), containera));
 }
@@ -202,7 +212,7 @@ TEST_F(CrostiniUtilTest, ShouldNotStopVm) {
   CrostiniManager* manager = CrostiniManager::GetForProfile(profile_.get());
   ContainerId containera("apple", "banana");
   ContainerId containerb("apple", "strawberry");
-  base::Value containers(base::Value::Type::LIST);
+  base::Value::List containers;
   containers.Append(containera.ToDictValue().Clone());
   containers.Append(containerb.ToDictValue().Clone());
 
@@ -214,7 +224,8 @@ TEST_F(CrostiniUtilTest, ShouldNotStopVm) {
 
   ASSERT_TRUE(manager->IsVmRunning("apple"));
 
-  profile_->GetPrefs()->Set(prefs::kCrostiniContainers, std::move(containers));
+  profile_->GetPrefs()->SetList(prefs::kCrostiniContainers,
+                                std::move(containers));
 
   EXPECT_FALSE(ShouldStopVm(profile_.get(), containera));
 }

chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc

@@ -748,35 +748,36 @@ void CrostiniHandler::HandleRequestContainerInfo(
   if (!crostini::CrostiniFeatures::Get()->IsMultiContainerAllowed(profile_)) {
     return;
   }
-  base::Value container_info_list(base::Value::Type::LIST);
+  base::Value::List container_info_list;
 
-  base::Value::ConstListView containers =
+  const base::Value::List& containers =
       profile_->GetPrefs()
-          ->GetList(crostini::prefs::kCrostiniContainers)
-          ->GetListDeprecated();
+          ->Get(crostini::prefs::kCrostiniContainers)
+          ->GetList();
 
   for (const auto& dict : containers) {
     crostini::ContainerId container_id(dict);
-    base::Value container_info_value(base::Value::Type::DICTIONARY);
-    container_info_value.SetKey(kIdKey, container_id.ToDictValue());
+    base::Value::Dict container_info_value;
+    container_info_value.Set(kIdKey, container_id.ToDictValue());
     auto info =
         crostini::CrostiniManager::GetForProfile(profile_)->GetContainerInfo(
             container_id);
     if (info) {
-      container_info_value.SetStringKey(kIpv4Key, info->ipv4_address);
+      container_info_value.Set(kIpv4Key, info->ipv4_address);
     }
 
     SkColor badge_color =
         crostini::GetContainerBadgeColor(profile_, container_id);
     std::string badge_color_str =
         base::StringPrintf("#%02x%02x%02x", SkColorGetR(badge_color),
                            SkColorGetG(badge_color), SkColorGetB(badge_color));
-    container_info_value.SetStringKey("badge_color", badge_color_str);
+    container_info_value.Set("badge_color", badge_color_str);
 
     container_info_list.Append(std::move(container_info_value));
   }
 
-  FireWebUIListener("crostini-container-info", container_info_list);
+  FireWebUIListener("crostini-container-info",
+                    base::Value(std::move(container_info_list)));
 }
 
 void CrostiniHandler::HandleSetContainerBadgeColor(