PasswordStore: Clean up expectations about rewriting vectors of forms

chrome/browser/password_manager/native_backend_kwallet_x.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 "chrome/browser/password_manager/native_backend_kwallet_x.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/grit/chromium_strings.h"
 #include "components/autofill/core/common/password_form.h"
 #include "content/public/browser/browser_thread.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using autofill::PasswordForm;
 using content::BrowserThread;
 
 namespace {
 
+// In case the fields in the pickle ever change, version them so we can try to
+// read old pickles. (Note: do not eat old pickles past the expiration date.)
+const int kPickleVersion = 5;
+
 // We could localize this string, but then changing your locale would cause
 // you to lose access to all your stored passwords. Maybe best not to do that.
 // Name of the folder to store passwords in.
 const char kKWalletFolder[] = "Chrome Form Data";
 
 // DBus service, path, and interface names for klauncher and kwalletd.
 const char kKWalletServiceName[] = "org.kde.kwalletd";
 const char kKWalletPath[] = "/modules/kwalletd";
 const char kKWalletInterface[] = "org.kde.KWallet";
 const char kKLauncherServiceName[] = "org.kde.klauncher";
@@ skipping 53 matching lines @@
     LOG(WARNING) << "Failed to deserialize version " << version
                  << " KWallet entry (realm: " << signon_realm
                  << ") with native architecture size; will try alternate "
                  << "size.";
   } else {
     LOG(ERROR) << "Failed to deserialize version " << version
                << " KWallet entry (realm: " << signon_realm << ")";
   }
 }
 
+// Deserializes a list of credentials from the wallet to |forms| (replacing
+// |forms| contents). |size_32| controls reading the size field within the
+// pickle as 32 bits. We used to use Pickle::WriteSize() to write the number of
+// password forms, but that has a different size on 32- and 64-bit systems. So,
+// now we always write a 64-bit quantity, but we support trying to read it as
+// either size when reading old pickles that fail to deserialize using the
+// native size. Returns true on success, false on failure (erasing |forms| in
+// that case).
+bool DeserializeValueSize(const std::string& signon_realm,
+                          const PickleIterator& init_iter,
+                          int version,
+                          bool size_32,
+                          bool warn_only,
+                          ScopedVector<autofill::PasswordForm>* forms) {
+  forms->clear();
+  PickleIterator iter = init_iter;
+
+  size_t count = 0;
+  if (size_32) {
+    uint32_t count_32 = 0;
+    if (!iter.ReadUInt32(&count_32)) {
+      LOG(ERROR) << "Failed to deserialize KWallet entry "
+                 << "(realm: " << signon_realm << ")";
+      return false;
+    }
+    count = count_32;
+  } else {
+    if (!iter.ReadSizeT(&count)) {
+      LOG(ERROR) << "Failed to deserialize KWallet entry "
+                 << "(realm: " << signon_realm << ")";
+      return false;
+    }
+  }
+
+  if (count > 0xFFFF) {
+    // Trying to pin down the cause of http://crbug.com/80728 (or fix it).
+    // This is a very large number of passwords to be saved for a single realm.
+    // It is almost certainly a corrupt pickle and not real data. Ignore it.
+    // This very well might actually be http://crbug.com/107701, so if we're
+    // reading an old pickle, we don't even log this the first time we try to
+    // read it. (That is, when we're reading the native architecture size.)
+    if (!warn_only) {
+      LOG(ERROR) << "Suspiciously large number of entries in KWallet entry "
+                 << "(" << count << "; realm: " << signon_realm << ")";
+    }
+    return false;
+  }
+
+  // We'll swap |converted_forms| with |*forms| on success, to make sure we
+  // don't return partial results on failure.
+  ScopedVector<autofill::PasswordForm> converted_forms;
+  converted_forms.reserve(count);
+  for (size_t i = 0; i < count; ++i) {
+    scoped_ptr<PasswordForm> form(new PasswordForm());
+    form->signon_realm.assign(signon_realm);
+
+    int scheme = 0;
+    int64 date_created = 0;
+    int type = 0;
+    // Note that these will be read back in the order listed due to
+    // short-circuit evaluation. This is important.
+    if (!iter.ReadInt(&scheme) ||
+        !ReadGURL(&iter, warn_only, &form->origin) ||
+        !ReadGURL(&iter, warn_only, &form->action) ||
+        !iter.ReadString16(&form->username_element) ||
+        !iter.ReadString16(&form->username_value) ||
+        !iter.ReadString16(&form->password_element) ||
+        !iter.ReadString16(&form->password_value) ||
+        !iter.ReadString16(&form->submit_element) ||
+        !iter.ReadBool(&form->ssl_valid) ||
+        !iter.ReadBool(&form->preferred) ||
+        !iter.ReadBool(&form->blacklisted_by_user) ||
+        !iter.ReadInt64(&date_created)) {
+      LogDeserializationWarning(version, signon_realm, warn_only);
+      return false;
+    }
+    form->scheme = static_cast<PasswordForm::Scheme>(scheme);
+
+    if (version > 1) {
+      if (!iter.ReadInt(&type) ||
+          !iter.ReadInt(&form->times_used) ||
+          !autofill::DeserializeFormData(&iter, &form->form_data)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+      form->type = static_cast<PasswordForm::Type>(type);
+    }
+
+    if (version > 2) {
+      int64 date_synced = 0;
+      if (!iter.ReadInt64(&date_synced)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+      form->date_synced = base::Time::FromInternalValue(date_synced);
+    }
+
+    if (version > 3) {
+      if (!iter.ReadString16(&form->display_name) ||
+          !ReadGURL(&iter, warn_only, &form->avatar_url) ||
+          !ReadGURL(&iter, warn_only, &form->federation_url) ||
+          !iter.ReadBool(&form->skip_zero_click)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+    }
+
+    if (version > 4) {
+      form->date_created = base::Time::FromInternalValue(date_created);
+    } else {
+      form->date_created = base::Time::FromTimeT(date_created);
+    }
+
+    converted_forms.push_back(form.release());
+  }
+
+  forms->swap(converted_forms);
+  return true;
+}
+
+// Serializes a list of PasswordForms to be stored in the wallet.
+void SerializeValue(const std::vector<autofill::PasswordForm*>& forms,
+                    Pickle* pickle) {
+  pickle->WriteInt(kPickleVersion);
+  pickle->WriteSizeT(forms.size());
+  for (autofill::PasswordForm* form : forms) {
+    pickle->WriteInt(form->scheme);
+    pickle->WriteString(form->origin.spec());
+    pickle->WriteString(form->action.spec());
+    pickle->WriteString16(form->username_element);
+    pickle->WriteString16(form->username_value);
+    pickle->WriteString16(form->password_element);
+    pickle->WriteString16(form->password_value);
+    pickle->WriteString16(form->submit_element);
+    pickle->WriteBool(form->ssl_valid);
+    pickle->WriteBool(form->preferred);
+    pickle->WriteBool(form->blacklisted_by_user);
+    pickle->WriteInt64(form->date_created.ToInternalValue());
+    pickle->WriteInt(form->type);
+    pickle->WriteInt(form->times_used);
+    autofill::SerializeFormData(form->form_data, pickle);
+    pickle->WriteInt64(form->date_synced.ToInternalValue());
+    pickle->WriteString16(form->display_name);
+    pickle->WriteString(form->avatar_url.spec());
+    pickle->WriteString(form->federation_url.spec());
+    pickle->WriteBool(form->skip_zero_click);
+  }
+}
+
+// Moves the content of |second| to the end of |first|.
+void AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
+                         ScopedVector<autofill::PasswordForm> second) {
+  first->reserve(first->size() + second.size());
+  first->insert(first->end(), second.begin(), second.end());
+  second.weak_clear();
+}
+
 }  // namespace
 
 NativeBackendKWallet::NativeBackendKWallet(LocalProfileId id)
     : profile_id_(id),
       kwallet_proxy_(NULL),
       app_name_(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME)) {
   folder_name_ = GetProfileSpecificFolderName();
 }
 
 NativeBackendKWallet::~NativeBackendKWallet() {
@@ skipping 262 matching lines @@
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
   return GetLoginsList(form.signon_realm, wallet_handle, forms);
 }
 
 bool NativeBackendKWallet::GetAutofillableLogins(
     ScopedVector<autofill::PasswordForm>* forms) {
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
-  return GetLoginsList(true, wallet_handle, forms);
+  return GetLoginsList(BlacklistOptions::AUTOFILLABLE, wallet_handle, forms);
 }
 
 bool NativeBackendKWallet::GetBlacklistLogins(
     ScopedVector<autofill::PasswordForm>* forms) {
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
-  return GetLoginsList(false, wallet_handle, forms);
+  return GetLoginsList(BlacklistOptions::BLACKLISTED, wallet_handle, forms);
 }
 
 bool NativeBackendKWallet::GetLoginsList(
     const std::string& signon_realm,
     int wallet_handle,
     ScopedVector<autofill::PasswordForm>* forms) {
+  forms->clear();
   // Is there an entry in the wallet?
   {
     dbus::MethodCall method_call(kKWalletInterface, "hasEntry");
     dbus::MessageWriter builder(&method_call);
     builder.AppendInt32(wallet_handle);  // handle
     builder.AppendString(folder_name_);  // folder
     builder.AppendString(signon_realm);  // key
     builder.AppendString(app_name_);     // appid
     scoped_ptr<dbus::Response> response(
         kwallet_proxy_->CallMethodAndBlock(
@@ skipping 42 matching lines @@
     if (!CheckSerializedValue(bytes, length, signon_realm)) {
       // This is weird, but we choose not to call it an error. There is an
       // invalid entry somehow, but by just ignoring it, we make it easier to
       // repair without having to delete it using kwalletmanager (that is, by
       // just saving a new password within this realm to overwrite it).
       return true;
     }
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    DeserializeValue(signon_realm, pickle, forms);
+    *forms = DeserializeValue(signon_realm, pickle);
   }
 
   return true;
 }
 
 bool NativeBackendKWallet::GetLoginsList(
-    bool autofillable,
+    BlacklistOptions options,
     int wallet_handle,
     ScopedVector<autofill::PasswordForm>* forms) {
+  forms->clear();
   ScopedVector<autofill::PasswordForm> all_forms;
   if (!GetAllLogins(wallet_handle, &all_forms))
     return false;
 
   // We have to read all the entries, and then filter them here.
-  forms->reserve(forms->size() + all_forms.size());
+  forms->reserve(all_forms.size());
   for (auto& saved_form : all_forms) {
-    if (saved_form->blacklisted_by_user == !autofillable) {
+    if (saved_form->blacklisted_by_user ==
+        (options == BlacklistOptions::BLACKLISTED)) {
       forms->push_back(saved_form);
       saved_form = nullptr;
     }
   }
 
   return true;
 }
 
 bool NativeBackendKWallet::GetAllLogins(
     int wallet_handle,
     ScopedVector<autofill::PasswordForm>* forms) {
+  forms->clear();
   // We could probably also use readEntryList here.
   std::vector<std::string> realm_list;
   {
     dbus::MethodCall method_call(kKWalletInterface, "entryList");
     dbus::MessageWriter builder(&method_call);
     builder.AppendInt32(wallet_handle);  // handle
     builder.AppendString(folder_name_);  // folder
     builder.AppendString(app_name_);     // appid
     scoped_ptr<dbus::Response> response(
         kwallet_proxy_->CallMethodAndBlock(
             &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
     if (!response.get()) {
       LOG(ERROR) << "Error contacting kwalletd (entryList)";
       return false;
     }
     dbus::MessageReader reader(response.get());
     if (!reader.PopArrayOfStrings(&realm_list)) {
       LOG(ERROR) << "Error reading response from kwalletd (entryList): "
                  << response->ToString();
       return false;
     }
   }
 
-  for (size_t i = 0; i < realm_list.size(); ++i) {
-    const std::string& signon_realm = realm_list[i];
+  // Swapping |collected_forms| with |*forms| just before "return true" makes
+  // sure partial results are not returned on failure.
+  ScopedVector<autofill::PasswordForm> collected_forms;
+  for (const std::string& signon_realm : realm_list) {
     dbus::MethodCall method_call(kKWalletInterface, "readEntry");
     dbus::MessageWriter builder(&method_call);
     builder.AppendInt32(wallet_handle);  // handle
     builder.AppendString(folder_name_);  // folder
     builder.AppendString(signon_realm);  // key
     builder.AppendString(app_name_);     // appid
     scoped_ptr<dbus::Response> response(
         kwallet_proxy_->CallMethodAndBlock(
             &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
     if (!response.get()) {
       LOG(ERROR) << "Error contacting kwalletd (readEntry)";
       return false;
     }
     dbus::MessageReader reader(response.get());
     const uint8_t* bytes = NULL;
     size_t length = 0;
     if (!reader.PopArrayOfBytes(&bytes, &length)) {
       LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
                  << response->ToString();
       return false;
     }
     if (!bytes || !CheckSerializedValue(bytes, length, signon_realm))
       continue;
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    DeserializeValue(signon_realm, pickle, forms);
+    AppendSecondToFirst(&collected_forms,
+                        DeserializeValue(signon_realm, pickle));
   }
+  forms->swap(collected_forms);
   return true;
 }
 
 bool NativeBackendKWallet::SetLoginsList(
     const std::vector<autofill::PasswordForm*>& forms,
     const std::string& signon_realm,
     int wallet_handle) {
   if (forms.empty()) {
     // No items left? Remove the entry from the wallet.
     dbus::MethodCall method_call(kKWalletInterface, "removeEntry");
@@ skipping 115 matching lines @@
     if (!reader.PopArrayOfBytes(&bytes, &length)) {
       LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
                  << response->ToString();
       continue;
     }
     if (!bytes || !CheckSerializedValue(bytes, length, signon_realm))
       continue;
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    ScopedVector<autofill::PasswordForm> all_forms;
-    DeserializeValue(signon_realm, pickle, &all_forms);
+    ScopedVector<autofill::PasswordForm> all_forms =
+        DeserializeValue(signon_realm, pickle);
 
     ScopedVector<autofill::PasswordForm> kept_forms;
     kept_forms.reserve(all_forms.size());
     base::Time autofill::PasswordForm::*date_member =
         date_to_compare == CREATION_TIMESTAMP
             ? &autofill::PasswordForm::date_created
             : &autofill::PasswordForm::date_synced;
     for (auto& saved_form : all_forms) {
       if (delete_begin <= saved_form->*date_member &&
           (delete_end.is_null() || saved_form->*date_member < delete_end)) {
         changes->push_back(password_manager::PasswordStoreChange(
             password_manager::PasswordStoreChange::REMOVE, *saved_form));
       } else {
         kept_forms.push_back(saved_form);
         saved_form = nullptr;
       }
     }
 
     if (!SetLoginsList(kept_forms.get(), signon_realm, wallet_handle)) {
       ok = false;
       changes->clear();
     }
   }
   return ok;
 }
 
 // static
-void NativeBackendKWallet::SerializeValue(
-    const std::vector<autofill::PasswordForm*>& forms,
-    Pickle* pickle) {
-  pickle->WriteInt(kPickleVersion);
-  pickle->WriteSizeT(forms.size());
-  for (autofill::PasswordForm* form : forms) {
-    pickle->WriteInt(form->scheme);
-    pickle->WriteString(form->origin.spec());
-    pickle->WriteString(form->action.spec());
-    pickle->WriteString16(form->username_element);
-    pickle->WriteString16(form->username_value);
-    pickle->WriteString16(form->password_element);
-    pickle->WriteString16(form->password_value);
-    pickle->WriteString16(form->submit_element);
-    pickle->WriteBool(form->ssl_valid);
-    pickle->WriteBool(form->preferred);
-    pickle->WriteBool(form->blacklisted_by_user);
-    pickle->WriteInt64(form->date_created.ToInternalValue());
-    pickle->WriteInt(form->type);
-    pickle->WriteInt(form->times_used);
-    autofill::SerializeFormData(form->form_data, pickle);
-    pickle->WriteInt64(form->date_synced.ToInternalValue());
-    pickle->WriteString16(form->display_name);
-    pickle->WriteString(form->avatar_url.spec());
-    pickle->WriteString(form->federation_url.spec());
-    pickle->WriteBool(form->skip_zero_click);
-  }
-}
-
-// static
-bool NativeBackendKWallet::DeserializeValueSize(
+ScopedVector<autofill::PasswordForm> NativeBackendKWallet::DeserializeValue(
     const std::string& signon_realm,
-    const PickleIterator& init_iter,
-    int version,
-    bool size_32,
-    bool warn_only,
-    ScopedVector<autofill::PasswordForm>* forms) {
-  PickleIterator iter = init_iter;
-
-  size_t count = 0;
-  if (size_32) {
-    uint32_t count_32 = 0;
-    if (!iter.ReadUInt32(&count_32)) {
-      LOG(ERROR) << "Failed to deserialize KWallet entry "
-                 << "(realm: " << signon_realm << ")";
-      return false;
-    }
-    count = count_32;
-  } else {
-    if (!iter.ReadSizeT(&count)) {
-      LOG(ERROR) << "Failed to deserialize KWallet entry "
-                 << "(realm: " << signon_realm << ")";
-      return false;
-    }
-  }
-
-  if (count > 0xFFFF) {
-    // Trying to pin down the cause of http://crbug.com/80728 (or fix it).
-    // This is a very large number of passwords to be saved for a single realm.
-    // It is almost certainly a corrupt pickle and not real data. Ignore it.
-    // This very well might actually be http://crbug.com/107701, so if we're
-    // reading an old pickle, we don't even log this the first time we try to
-    // read it. (That is, when we're reading the native architecture size.)
-    if (!warn_only) {
-      LOG(ERROR) << "Suspiciously large number of entries in KWallet entry "
-                 << "(" << count << "; realm: " << signon_realm << ")";
-    }
-    return false;
-  }
-
-  forms->reserve(forms->size() + count);
-  for (size_t i = 0; i < count; ++i) {
-    scoped_ptr<PasswordForm> form(new PasswordForm());
-    form->signon_realm.assign(signon_realm);
-
-    int scheme = 0;
-    int64 date_created = 0;
-    int type = 0;
-    // Note that these will be read back in the order listed due to
-    // short-circuit evaluation. This is important.
-    if (!iter.ReadInt(&scheme) ||
-        !ReadGURL(&iter, warn_only, &form->origin) ||
-        !ReadGURL(&iter, warn_only, &form->action) ||
-        !iter.ReadString16(&form->username_element) ||
-        !iter.ReadString16(&form->username_value) ||
-        !iter.ReadString16(&form->password_element) ||
-        !iter.ReadString16(&form->password_value) ||
-        !iter.ReadString16(&form->submit_element) ||
-        !iter.ReadBool(&form->ssl_valid) ||
-        !iter.ReadBool(&form->preferred) ||
-        !iter.ReadBool(&form->blacklisted_by_user) ||
-        !iter.ReadInt64(&date_created)) {
-      LogDeserializationWarning(version, signon_realm, warn_only);
-      return false;
-    }
-    form->scheme = static_cast<PasswordForm::Scheme>(scheme);
-
-    if (version > 1) {
-      if (!iter.ReadInt(&type) ||
-          !iter.ReadInt(&form->times_used) ||
-          !autofill::DeserializeFormData(&iter, &form->form_data)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-      form->type = static_cast<PasswordForm::Type>(type);
-    }
-
-    if (version > 2) {
-      int64 date_synced = 0;
-      if (!iter.ReadInt64(&date_synced)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-      form->date_synced = base::Time::FromInternalValue(date_synced);
-    }
-
-    if (version > 3) {
-      if (!iter.ReadString16(&form->display_name) ||
-          !ReadGURL(&iter, warn_only, &form->avatar_url) ||
-          !ReadGURL(&iter, warn_only, &form->federation_url) ||
-          !iter.ReadBool(&form->skip_zero_click)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-    }
-
-    if (version > 4) {
-      form->date_created = base::Time::FromInternalValue(date_created);
-    } else {
-      form->date_created = base::Time::FromTimeT(date_created);
-    }
-
-    forms->push_back(form.release());
-  }
-
-  return true;
-}
-
-// static
-void NativeBackendKWallet::DeserializeValue(
-    const std::string& signon_realm,
-    const Pickle& pickle,
-    ScopedVector<autofill::PasswordForm>* forms) {
+    const Pickle& pickle) {
   PickleIterator iter(pickle);
 
   int version = -1;
   if (!iter.ReadInt(&version) ||
       version < 0 || version > kPickleVersion) {
     LOG(ERROR) << "Failed to deserialize KWallet entry "
                << "(realm: " << signon_realm << ")";
-    return;
+    return ScopedVector<autofill::PasswordForm>();
   }
 
+  ScopedVector<autofill::PasswordForm> forms;
   if (version > 0) {
     // In current pickles, we expect 64-bit sizes. Failure is an error.
-    DeserializeValueSize(signon_realm, iter, version, false, false, forms);
-    return;
+    DeserializeValueSize(signon_realm, iter, version, false, false, &forms);
+    return forms.Pass();
   }
 
-  const size_t saved_forms_size = forms->size();
   const bool size_32 = sizeof(size_t) == sizeof(uint32_t);
   if (!DeserializeValueSize(
-          signon_realm, iter, version, size_32, true, forms)) {
+          signon_realm, iter, version, size_32, true, &forms)) {
     // We failed to read the pickle using the native architecture of the system.
     // Try again with the opposite architecture. Note that we do this even on
     // 32-bit machines, in case we're reading a 64-bit pickle. (Probably rare,
     // since mostly we expect upgrades, not downgrades, but both are possible.)
-    forms->resize(saved_forms_size);
-    DeserializeValueSize(signon_realm, iter, version, !size_32, false, forms);
+    DeserializeValueSize(signon_realm, iter, version, !size_32, false, &forms);
   }
+  return forms.Pass();
 }
 
 int NativeBackendKWallet::WalletHandle() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
   // Open the wallet.
   // TODO(mdm): Are we leaking these handles? Find out.
   int32_t handle = kInvalidKWalletHandle;
   {
     dbus::MethodCall method_call(kKWalletInterface, "open");
@@ skipping 71 matching lines @@
   }
 
   return handle;
 }
 
 std::string NativeBackendKWallet::GetProfileSpecificFolderName() const {
   // Originally, the folder name was always just "Chrome Form Data".
   // Now we use it to distinguish passwords for different profiles.
   return base::StringPrintf("%s (%d)", kKWalletFolder, profile_id_);
 }