art: Fix bug in VariantMap::Set

Bug: 19295410
Change-Id: I7827583846d710698c0e7bc0ec1a2c3bf901bd50

3 files changed, 41 insertions, 1 deletions

diff --git a/runtime/base/variant_map.h b/runtime/base/variant_map.h
index cf7977edca..c9718fcdd0 100644
--- a/runtime/base/variant_map.h
+++ b/runtime/base/variant_map.h
@@ -271,8 +271,11 @@ struct VariantMap {
   // 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.
diff --git a/runtime/base/variant_map_test.cc b/runtime/base/variant_map_test.cc
index 827de46249..f306a48e53 100644
--- a/runtime/base/variant_map_test.cc
+++ b/runtime/base/variant_map_test.cc
@@ -38,10 +38,12 @@ namespace {
 
     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 @@ TEST(VariantMaps, BasicReadWrite) {
   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 @@ TEST(VariantMaps, BasicReadWrite) {
   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;
diff --git a/runtime/parsed_options_test.cc b/runtime/parsed_options_test.cc
index 79dc2fa626..658b656796 100644
--- a/runtime/parsed_options_test.cc
+++ b/runtime/parsed_options_test.cc
@@ -98,4 +98,20 @@ TEST_F(ParsedOptionsTest, ParsedOptions) {
   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