Remove base::ListValue::Set(size_t, base::Value*)

tools/ipc_fuzzer/fuzzer/fuzzer.cc

Index: tools/ipc_fuzzer/fuzzer/fuzzer.cc
diff --git a/tools/ipc_fuzzer/fuzzer/fuzzer.cc b/tools/ipc_fuzzer/fuzzer/fuzzer.cc
index a913dc23eb78bd86a18506e2345ed007ad8e2310..2474860fd67d9549185612c1625eda734cceaa44 100644
--- a/tools/ipc_fuzzer/fuzzer/fuzzer.cc
+++ b/tools/ipc_fuzzer/fuzzer/fuzzer.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/shared_memory_handle.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
@@ -510,28 +511,28 @@ struct FuzzTraits<base::ListValue> {
           bool tmp;
           p->GetBoolean(index, &tmp);
           fuzzer->FuzzBool(&tmp);
-          p->Set(index, new base::Value(tmp));
+          p->Set(index, base::MakeUnique<base::Value>(tmp));
           break;
         }
         case base::Value::Type::INTEGER: {
           int tmp;
           p->GetInteger(index, &tmp);
           fuzzer->FuzzInt(&tmp);
-          p->Set(index, new base::Value(tmp));
+          p->Set(index, base::MakeUnique<base::Value>(tmp));
           break;
         }
         case base::Value::Type::DOUBLE: {
           double tmp;
           p->GetDouble(index, &tmp);
           fuzzer->FuzzDouble(&tmp);
-          p->Set(index, new base::Value(tmp));
+          p->Set(index, base::MakeUnique<base::Value>(tmp));
           break;
         }
         case base::Value::Type::STRING: {
           std::string tmp;
           p->GetString(index, &tmp);
           fuzzer->FuzzString(&tmp);
-          p->Set(index, new base::Value(tmp));
+          p->Set(index, base::MakeUnique<base::Value>(tmp));
           break;
         }
         case base::Value::Type::BINARY: {
@@ -545,14 +546,14 @@ struct FuzzTraits<base::ListValue> {
           base::DictionaryValue* tmp = new base::DictionaryValue();
           p->GetDictionary(index, &tmp);

[vabr (Chromium), 2017/04/25 17:13:15]

This seems wrong: GetDictionary fills |tmp| with a pointer to a DictionaryValue
owned by |p|. Note that the target of |tmp| after line 547 was not allocated
through new, it is part of a vector. Then line 549 wraps |tmp| in a unique_ptr,
which should attempt to destroy it after line 549 completes. And the value
assigned on line 546 is never deleted.

I'm probably wrong, because the sanitizers would have to found this already. But
if I'm correct, then the same issue is also below for the LIST case.

[brettw, 2017/04/25 19:55:45]

This looks wrong to me too.

[jdoerrie, 2017/04/26 11:34:37]

I agree, looking at this more closely this seems wrong. This code would be fine
if line 547 didn't exist or there is no dictionary at p[index], in which case
GetDictionary is a no-op. However, if p[index] contains a dictionary already,
this leaks the memory allocated in line 546. Furthermore, line 549 is then a
self move-assignment, which in the current implementation clears the contents of
the Value, rendering the Fuzzing in line 548 useless. Lastly, when |Set|
concludes, the passed in unique ptr will attempt to destruct p[index], while p
is still alive. This should result in all sort of errors. Given that this was
never caught by the sanitizers, it is possible that p[index] never contained a
previously existing dictionary.

I changed it now for both the Dict and the List, let me know what you think.

           FuzzParam(tmp, fuzzer);
-          p->Set(index, tmp);
+          p->Set(index, base::WrapUnique(tmp));
           break;
         }
         case base::Value::Type::LIST: {
           base::ListValue* tmp = new base::ListValue();
           p->GetList(index, &tmp);
           FuzzParam(tmp, fuzzer);
-          p->Set(index, tmp);
+          p->Set(index, base::WrapUnique(tmp));
           break;
         }
         case base::Value::Type::NONE: