Change base::Value::ListStorage to std::vector<base::Value>

components/sync/driver/about_sync_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync/driver/about_sync_util.h"
 
 #include <string>
 #include <utility>
 
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 #include "components/sync/engine/sync_status.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/model/time.h"
@@ skipping 48 matching lines @@
 // Creates a 'section' for display on about:sync, consisting of a title and a
 // list of fields.  Returns a pointer to the new section.  Note that
 // |parent_list|, not the caller, owns the newly added section.
 base::ListValue* AddSection(base::ListValue* parent_list,
                             const std::string& title) {
   std::unique_ptr<base::DictionaryValue> section(new base::DictionaryValue());
   base::ListValue* section_contents = new base::ListValue();
   section->SetString("title", title);
   section->Set("data", section_contents);
   section->SetBoolean("is_sensitive", false);
+  // If the following |Append| results in a reallocation, pointers to
+  // |parent_list| and its members will be invalidated. This would result in

[skym, 2017/03/16 19:52:39]

Wait, I thought I understood this, but now I'm confused. I thought raw pointers
to the ListValue |parent_list| object would still valid. Only pointers to the
contents of |parent_list| would be invalidated, right? But this comment seems to
indicate otherwise. Or am I confused?

[jdoerrie, 2017/03/23 18:11:17]

My bad, your understanding is correct. I updated the comment now.

+  // use-after-free in |*SyncStat::SetValue|. This is why the following CHECK is
+  // necessary to ensure no reallocation takes place.
+  CHECK_LT(parent_list->GetSize(), parent_list->capacity());
   parent_list->Append(std::move(section));
   return section_contents;
 }
 
 // Same as AddSection, but for data that should be elided when dumped into text
 // form and posted in a public forum (e.g. unique identifiers).
 base::ListValue* AddSensitiveSection(base::ListValue* parent_list,
                                      const std::string& title) {
   std::unique_ptr<base::DictionaryValue> section(new base::DictionaryValue());
   base::ListValue* section_contents = new base::ListValue();
   section->SetString("title", title);
   section->Set("data", section_contents);
   section->SetBoolean("is_sensitive", true);
+  // If the following |Append| results in a reallocation, pointers to
+  // |parent_list| and its members will be invalidated. This would result in
+  // use-after-free in |*SyncStat::SetValue|. This is why the following CHECK is
+  // necessary to ensure no reallocation takes place.
+  CHECK_LT(parent_list->GetSize(), parent_list->capacity());
   parent_list->Append(std::move(section));
   return section_contents;
 }
 
 // The following helper classes help manage the about:sync fields which will be
 // populated in method in ConstructAboutInformation.
 //
 // Each instance of one of thse classes indicates a field in about:sync.  Each
 // field will be serialized to a DictionaryValue with entries for 'stat_name',
 // 'stat_value' and 'is_valid'.
 
 class StringSyncStat {
  public:
   StringSyncStat(base::ListValue* section, const std::string& key);
   void SetValue(const std::string& value);
   void SetValue(const base::string16& value);
 
  private:
   // Owned by the |section| passed in during construction.
   base::DictionaryValue* stat_;
 };
 
 StringSyncStat::StringSyncStat(base::ListValue* section,
                                const std::string& key) {
   stat_ = new base::DictionaryValue();
   stat_->SetString("stat_name", key);
   stat_->SetString("stat_value", "Uninitialized");
   stat_->SetBoolean("is_valid", false);
+  // |stat_| will be invalidated by |Append|, so it needs to be reset.
+  // Furthermore, if |Append| results in a reallocation, |stat_| members of
+  // other SyncStats will be invalidated. This is why the following check is
+  // necessary, so that it is guaranteed that a reallocation will not happen.
+  CHECK_LT(section->GetSize(), section->capacity());
   section->Append(base::WrapUnique(stat_));
+  section->GetDictionary(section->GetSize() - 1, &stat_);

[skym, 2017/03/15 16:57:23]

Are these GetDictionary(x, DictionaryValue**) methods even sticking around or
being removed as part of 646113?

[brettw, 2017/03/15 22:14:14]

These should all be removed, see the design doc I linked in a different comment.

 }
 
 void StringSyncStat::SetValue(const std::string& value) {
   stat_->SetString("stat_value", value);
   stat_->SetBoolean("is_valid", true);
 }
 
 void StringSyncStat::SetValue(const base::string16& value) {
   stat_->SetString("stat_value", value);
   stat_->SetBoolean("is_valid", true);
 }
 
 class BoolSyncStat {
  public:
   BoolSyncStat(base::ListValue* section, const std::string& key);
   void SetValue(bool value);
 
  private:
   // Owned by the |section| passed in during construction.
   base::DictionaryValue* stat_;
 };
 
 BoolSyncStat::BoolSyncStat(base::ListValue* section, const std::string& key) {
   stat_ = new base::DictionaryValue();
   stat_->SetString("stat_name", key);
   stat_->SetBoolean("stat_value", false);
   stat_->SetBoolean("is_valid", false);
+  // |stat_| will be invalidated by |Append|, so it needs to be reset.
+  // Furthermore, if |Append| results in a reallocation, |stat_| members of
+  // other SyncStats will be invalidated. This is why the following check is
+  // necessary, so that it is guaranteed that a reallocation will not happen.
+  CHECK_LT(section->GetSize(), section->capacity());
   section->Append(base::WrapUnique(stat_));
+  section->GetDictionary(section->GetSize() - 1, &stat_);
 }
 
 void BoolSyncStat::SetValue(bool value) {
   stat_->SetBoolean("stat_value", value);
   stat_->SetBoolean("is_valid", true);
 }
 
 class IntSyncStat {
  public:
   IntSyncStat(base::ListValue* section, const std::string& key);
   void SetValue(int value);
 
  private:
   // Owned by the |section| passed in during construction.
   base::DictionaryValue* stat_;
 };
 
 IntSyncStat::IntSyncStat(base::ListValue* section, const std::string& key) {
   stat_ = new base::DictionaryValue();
   stat_->SetString("stat_name", key);
   stat_->SetInteger("stat_value", 0);
   stat_->SetBoolean("is_valid", false);
+  // |stat_| will be invalidated by |Append|, so it needs to be reset.
+  // Furthermore, if |Append| results in a reallocation, |stat_| members of
+  // other SyncStats will be invalidated. This is why the following check is
+  // necessary, so that it is guaranteed that a reallocation will not happen.
+  CHECK_LT(section->GetSize(), section->capacity());
   section->Append(base::WrapUnique(stat_));
+  section->GetDictionary(section->GetSize() - 1, &stat_);
 }
 
 void IntSyncStat::SetValue(int value) {
   stat_->SetInteger("stat_value", value);
   stat_->SetBoolean("is_valid", true);
 }
 
 // Returns a string describing the chrome version environment. Version format:
 // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">
 // If version information is unavailable, returns "invalid."
@@ skipping 62 matching lines @@
 // classes defined above.
 std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
     SyncService* service,
     SigninManagerBase* signin,
     version_info::Channel channel) {
   std::unique_ptr<base::DictionaryValue> about_info(
       new base::DictionaryValue());
 
   // 'details': A list of sections.
   base::ListValue* stats_list = new base::ListValue();
+  stats_list->Reserve(12);

[skym, 2017/03/15 16:57:23]

I've always really disliked how this file was implemented. Your approach seems
like a reasonable surgical change to make things work, but I'd really like to
rewrite it to avoid holding raw pointers. Can you create a bug and assign it to
me to remove the Reserve usage from this file.

[brettw, 2017/03/15 22:14:14]

Can you add TODOs referencing said bug on the Stat object constructors you
changed above, as well as all the Reserve() calls you added? That way skym can
find all the places to be sure to fix :)

[jdoerrie, 2017/03/23 18:11:17]

Done.

 
   // The following lines define the sections and their fields.  For each field,
   // a class is instantiated, which allows us to reference the fields in
   // 'setter' code later on in this function.
   base::ListValue* section_summary = AddSection(stats_list, "Summary");
+  section_summary->Reserve(1);
   StringSyncStat summary_string(section_summary, "Summary");
 
   base::ListValue* section_version = AddSection(stats_list, "Version Info");
+  section_version->Reserve(2);
   StringSyncStat client_version(section_version, "Client Version");
   StringSyncStat server_url(section_version, "Server URL");
 
   base::ListValue* section_identity =
       AddSensitiveSection(stats_list, kIdentityTitle);
+  section_identity->Reserve(3);
   StringSyncStat sync_id(section_identity, "Sync Client ID");
   StringSyncStat invalidator_id(section_identity, "Invalidator Client ID");
   StringSyncStat username(section_identity, "Username");
 
   base::ListValue* section_credentials = AddSection(stats_list, "Credentials");
+  section_credentials->Reserve(4);
   StringSyncStat request_token_time(section_credentials, "Requested Token");
   StringSyncStat receive_token_time(section_credentials, "Received Token");
   StringSyncStat token_request_status(section_credentials,
                                       "Token Request Status");
   StringSyncStat next_token_request(section_credentials, "Next Token Request");
 
   base::ListValue* section_local = AddSection(stats_list, "Local State");
+  section_local->Reserve(7);
   StringSyncStat server_connection(section_local, "Server Connection");
   StringSyncStat last_synced(section_local, "Last Synced");
   BoolSyncStat is_setup_complete(section_local,
                                  "Sync First-Time Setup Complete");
   StringSyncStat backend_initialization(section_local,
                                         "Sync Backend Initialization");
   BoolSyncStat is_syncing(section_local, "Syncing");
   BoolSyncStat is_local_sync_enabled(section_local,
                                      "Local sync backend enabled");
   StringSyncStat local_backend_path(section_local, "Local backend path");
 
   base::ListValue* section_network = AddSection(stats_list, "Network");
+  section_network->Reserve(3);
   BoolSyncStat is_throttled(section_network, "Throttled");
   StringSyncStat retry_time(section_network, "Retry time (maybe stale)");
   BoolSyncStat are_notifications_enabled(section_network,
                                          "Notifications Enabled");
 
   base::ListValue* section_encryption = AddSection(stats_list, "Encryption");
+  section_encryption->Reserve(9);
   BoolSyncStat is_using_explicit_passphrase(section_encryption,
                                             "Explicit Passphrase");
   BoolSyncStat is_passphrase_required(section_encryption,
                                       "Passphrase Required");
   BoolSyncStat is_cryptographer_ready(section_encryption,
                                       "Cryptographer Ready");
   BoolSyncStat has_pending_keys(section_encryption,
                                 "Cryptographer Has Pending Keys");
   StringSyncStat encrypted_types(section_encryption, "Encrypted Types");
   BoolSyncStat has_keystore_key(section_encryption, "Has Keystore Key");
   StringSyncStat keystore_migration_time(section_encryption,
                                          "Keystore Migration Time");
   StringSyncStat passphrase_type(section_encryption, "Passphrase Type");
   StringSyncStat passphrase_time(section_encryption, "Passphrase Time");
 
   base::ListValue* section_last_session =
       AddSection(stats_list, "Status from Last Completed Session");
+  section_last_session->Reserve(4);
   StringSyncStat session_source(section_last_session, "Sync Source");
   StringSyncStat get_key_result(section_last_session, "GetKey Step Result");
   StringSyncStat download_result(section_last_session, "Download Step Result");
   StringSyncStat commit_result(section_last_session, "Commit Step Result");
 
   base::ListValue* section_counters = AddSection(stats_list, "Running Totals");
+  section_counters->Reserve(7);
   IntSyncStat notifications_received(section_counters,
                                      "Notifications Received");
   IntSyncStat updates_received(section_counters, "Updates Downloaded");
   IntSyncStat tombstone_updates(section_counters, "Tombstone Updates");
   IntSyncStat reflected_updates(section_counters, "Reflected Updates");
   IntSyncStat successful_commits(section_counters, "Successful Commits");
   IntSyncStat conflicts_resolved_local_wins(section_counters,
                                             "Conflicts Resolved: Client Wins");
   IntSyncStat conflicts_resolved_server_wins(section_counters,
                                              "Conflicts Resolved: Server Wins");
 
   base::ListValue* section_this_cycle =
       AddSection(stats_list, "Transient Counters (this cycle)");
+  section_this_cycle->Reserve(4);
   IntSyncStat encryption_conflicts(section_this_cycle, "Encryption Conflicts");
   IntSyncStat hierarchy_conflicts(section_this_cycle, "Hierarchy Conflicts");
   IntSyncStat server_conflicts(section_this_cycle, "Server Conflicts");
   IntSyncStat committed_items(section_this_cycle, "Committed Items");
 
   base::ListValue* section_that_cycle = AddSection(
       stats_list, "Transient Counters (last cycle of last completed session)");
+  section_that_cycle->Reserve(3);
   IntSyncStat updates_downloaded(section_that_cycle, "Updates Downloaded");
   IntSyncStat committed_count(section_that_cycle, "Committed Count");
   IntSyncStat entries(section_that_cycle, "Entries");
 
   base::ListValue* section_nudge_info =
       AddSection(stats_list, "Nudge Source Counters");
+  section_nudge_info->Reserve(3);
   IntSyncStat nudge_source_notification(section_nudge_info,
                                         "Server Invalidations");
   IntSyncStat nudge_source_local(section_nudge_info, "Local Changes");
   IntSyncStat nudge_source_local_refresh(section_nudge_info, "Local Refreshes");
 
   // This list of sections belongs in the 'details' field of the returned
   // message.
   about_info->Set(kDetailsKey, stats_list);
 
   // Populate all the fields we declared above.
@@ skipping 128 matching lines @@
       full_status.sync_protocol_error.error_type != UNKNOWN_ERROR &&
       full_status.sync_protocol_error.error_type != SYNC_SUCCESS;
 
   about_info->SetBoolean("actionable_error_detected",
                          actionable_error_detected);
 
   // NOTE: We won't bother showing any of the following values unless
   // actionable_error_detected is set.
 
   base::ListValue* actionable_error = new base::ListValue();
+  actionable_error->Reserve(4);
   about_info->Set("actionable_error", actionable_error);
 
   StringSyncStat error_type(actionable_error, "Error Type");
   StringSyncStat action(actionable_error, "Action");
   StringSyncStat url(actionable_error, "URL");
   StringSyncStat description(actionable_error, "Error Description");
 
   if (actionable_error_detected) {
     error_type.SetValue(
         GetSyncErrorTypeString(full_status.sync_protocol_error.error_type));
@@ skipping 18 matching lines @@
   }
 
   about_info->Set("type_status", service->GetTypeStatusMap());
 
   return about_info;
 }
 
 }  // namespace sync_ui_util
 
 }  // namespace syncer