commit 2798da180524dde1f923b4ee964877546bd6bb44
author Igor Murashkin <iam@google.com> Fri Feb 06 17:59:39 2015 -0800
committer Igor Murashkin <iam@google.com> Tue Feb 17 10:57:35 2015 -0800
tree 9636c049c56402a7cc0196ff3657a8c8ec1322a0
parent 6e27f82193a8f54cd8ecdc8fb2c4c1adadafbaf4

art: Fix bug in VariantMap::Set

Bug: 19295410
Change-Id: I7827583846d710698c0e7bc0ec1a2c3bf901bd50

runtime/base/variant_map.h

diff --git a/runtime/base/variant_map.h b/runtime/base/variant_map.h
index cf7977e..c9718fc 100644
--- a/runtime/base/variant_map.h
+++ b/runtime/base/variant_map.h
@@ -271,8 +271,11 @@
   // Set a value for a given key, overwriting the previous value if any.
   template <typename TValue>
   void Set(const TKey<TValue>& key, const TValue& value) {
+    // Clone the value first, to protect against &value == GetValuePtr(key).
+    auto* new_value = new TValue(value);
+
     Remove(key);
-    storage_map_.insert({{key.Clone(), new TValue(value)}});
+    storage_map_.insert({{key.Clone(), new_value}});
   }
 
   // Set a value for a given key, only if there was no previous value before.

runtime/base/variant_map_test.cc

diff --git a/runtime/base/variant_map_test.cc b/runtime/base/variant_map_test.cc
index 827de46..f306a48 100644
--- a/runtime/base/variant_map_test.cc
+++ b/runtime/base/variant_map_test.cc
@@ -38,10 +38,12 @@
 
     static const Key<int> Apple;
     static const Key<double> Orange;
+    static const Key<std::string> Label;
   };
 
   const FruitMap::Key<int> FruitMap::Apple;
   const FruitMap::Key<double> FruitMap::Orange;
+  const FruitMap::Key<std::string> FruitMap::Label;
 }  // namespace
 
 TEST(VariantMaps, BasicReadWrite) {
@@ -67,6 +69,7 @@
   EXPECT_DOUBLE_EQ(555.0, *fm.Get(FruitMap::Orange));
   EXPECT_EQ(size_t(2), fm.Size());
 
+  // Simple remove
   fm.Remove(FruitMap::Apple);
   EXPECT_FALSE(fm.Exists(FruitMap::Apple));
 
@@ -75,6 +78,24 @@
   EXPECT_FALSE(fm.Exists(FruitMap::Orange));
 }
 
+TEST(VariantMaps, SetPreviousValue) {
+  FruitMap fm;
+
+  // Indirect remove by setting yourself again
+  fm.Set(FruitMap::Label, std::string("hello_world"));
+  auto* ptr = fm.Get(FruitMap::Label);
+  ASSERT_TRUE(ptr != nullptr);
+  *ptr = "foobar";
+
+  // Set the value to the same exact pointer which we got out of the map.
+  // This should cleanly 'just work' and not try to delete the value too early.
+  fm.Set(FruitMap::Label, *ptr);
+
+  auto* new_ptr = fm.Get(FruitMap::Label);
+  ASSERT_TRUE(ptr != nullptr);
+  EXPECT_EQ(std::string("foobar"), *new_ptr);
+}
+
 TEST(VariantMaps, RuleOfFive) {
   // Test empty constructor
   FruitMap fmEmpty;

runtime/parsed_options_test.cc

diff --git a/runtime/parsed_options_test.cc b/runtime/parsed_options_test.cc
index 79dc2fa..658b656 100644
--- a/runtime/parsed_options_test.cc
+++ b/runtime/parsed_options_test.cc
@@ -98,4 +98,20 @@
   EXPECT_EQ("baz=qux", properties_list[1]);
 }
 
+TEST_F(ParsedOptionsTest, ParsedOptionsGc) {
+  RuntimeOptions options;
+  options.push_back(std::make_pair("-Xgc:MC", nullptr));
+
+  RuntimeArgumentMap map;
+  std::unique_ptr<ParsedOptions> parsed(ParsedOptions::Create(options, false, &map));
+  ASSERT_TRUE(parsed.get() != NULL);
+  ASSERT_NE(0u, map.Size());
+
+  using Opt = RuntimeArgumentMap;
+
+  EXPECT_TRUE(map.Exists(Opt::GcOption));
+
+  XGcOption xgc = map.GetOrDefault(Opt::GcOption);
+  EXPECT_EQ(gc::kCollectorTypeMC, xgc.collector_type_);}
+
 }  // namespace art