Linux: always use 64-bit sizes when writing password pickles to KWallet.

Also, add code to detect failures to read existing pickles and to try reading
them with the other size (32 vs. 64 bit) instead, if they are version 0.

New pickles are written with version number 1 and do not get this treatment.

BUG=107701
Review URL: https://chromiumcodereview.appspot.com/9635006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125897 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 215 insertions, 78 deletions

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.cc b/chrome/browser/password_manager/native_backend_kwallet_x.cc
index 964a70c..3a47d4e 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x.cc
@@ -647,9 +647,9 @@ bool NativeBackendKWallet::CompareForms(const PasswordForm& a,
 void NativeBackendKWallet::SerializeValue(const PasswordFormList& forms,
                                           Pickle* pickle) {
   pickle->WriteInt(kPickleVersion);
-  pickle->WriteSize(forms.size());
-  for (PasswordFormList::const_iterator it = forms.begin() ;
-       it != forms.end() ; ++it) {
+  pickle->WriteUInt64(forms.size());
+  for (PasswordFormList::const_iterator it = forms.begin();
+       it != forms.end(); ++it) {
     const PasswordForm* form = *it;
     pickle->WriteInt(form->scheme);
     pickle->WriteString(form->origin.spec());
@@ -679,37 +679,46 @@ bool NativeBackendKWallet::CheckSerializedValue(const uint8_t* byte_array,
   return true;
 }
 
-void NativeBackendKWallet::DeserializeValue(const std::string& signon_realm,
-                                            const Pickle& pickle,
-                                            PasswordFormList* forms) {
-  PickleIterator iter(pickle);
-
-  int version = -1;
-  if (!pickle.ReadInt(&iter, &version) || version != kPickleVersion) {
-    // This is the only version so far, so anything else is an error.
-    LOG(ERROR) << "Failed to deserialize KWallet entry "
-               << "(realm: " << signon_realm << ")";
-    return;
-  }
-
-  size_t count = 0;
-  if (!pickle.ReadSize(&iter, &count)) {
-    LOG(ERROR) << "Failed to deserialize KWallet entry "
-               << "(realm: " << signon_realm << ")";
-    return;
+bool NativeBackendKWallet::DeserializeValueSize(const std::string& signon_realm,
+                                                const Pickle& pickle,
+                                                const PickleIterator& init_iter,
+                                                bool size_32, bool warn_only,
+                                                PasswordFormList* forms) {
+  PickleIterator iter = init_iter;
+
+  uint64_t count = 0;
+  if (size_32) {
+    uint32_t count_32 = 0;
+    if (!pickle.ReadUInt32(&iter, &count_32)) {
+      LOG(ERROR) << "Failed to deserialize KWallet entry "
+                 << "(realm: " << signon_realm << ")";
+      return false;
+    }
+    count = count_32;
+  } else {
+    if (!pickle.ReadUInt64(&iter, &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.
-    LOG(ERROR) << "Suspiciously large number of entries in KWallet entry "
-               << "(" << count << "; realm: " << signon_realm << ")";
-    return;
+    // 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) {
+  for (uint64_t i = 0; i < count; ++i) {
     scoped_ptr<PasswordForm> form(new PasswordForm());
     form->signon_realm.assign(signon_realm);
 
@@ -718,8 +727,8 @@ void NativeBackendKWallet::DeserializeValue(const std::string& signon_realm,
     // Note that these will be read back in the order listed due to
     // short-circuit evaluation. This is important.
     if (!pickle.ReadInt(&iter, &scheme) ||
-        !ReadGURL(pickle, &iter, &form->origin) ||
-        !ReadGURL(pickle, &iter, &form->action) ||
+        !ReadGURL(pickle, &iter, warn_only, &form->origin) ||
+        !ReadGURL(pickle, &iter, warn_only, &form->action) ||
         !pickle.ReadString16(&iter, &form->username_element) ||
         !pickle.ReadString16(&iter, &form->username_value) ||
         !pickle.ReadString16(&iter, &form->password_element) ||
@@ -729,21 +738,61 @@ void NativeBackendKWallet::DeserializeValue(const std::string& signon_realm,
         !pickle.ReadBool(&iter, &form->preferred) ||
         !pickle.ReadBool(&iter, &form->blacklisted_by_user) ||
         !pickle.ReadInt64(&iter, &date_created)) {
-      LOG(ERROR) << "Failed to deserialize KWallet entry "
-                 << "(realm: " << signon_realm << ")";
-      break;
+      if (warn_only) {
+        LOG(WARNING) << "Failed to deserialize version 0 KWallet entry "
+                     << "(realm: " << signon_realm << ") with native "
+                     << "architecture size; will try alternate size.";
+      } else {
+        LOG(ERROR) << "Failed to deserialize KWallet entry "
+                   << "(realm: " << signon_realm << ")";
+      }
+      return false;
     }
     form->scheme = static_cast<PasswordForm::Scheme>(scheme);
     form->date_created = base::Time::FromTimeT(date_created);
     forms->push_back(form.release());
   }
+
+  return true;
+}
+
+void NativeBackendKWallet::DeserializeValue(const std::string& signon_realm,
+                                            const Pickle& pickle,
+                                            PasswordFormList* forms) {
+  PickleIterator iter(pickle);
+
+  int version = -1;
+  if (!pickle.ReadInt(&iter, &version) ||
+      version < 0 || version > kPickleVersion) {
+    LOG(ERROR) << "Failed to deserialize KWallet entry "
+               << "(realm: " << signon_realm << ")";
+    return;
+  }
+
+  if (version == kPickleVersion) {
+    // In current pickles, we expect 64-bit sizes. Failure is an error.
+    DeserializeValueSize(signon_realm, pickle, iter, false, false, forms);
+    return;
+  }
+
+  const size_t saved_forms_size = forms->size();
+  const bool size_32 = sizeof(size_t) == sizeof(uint32_t);
+  if (!DeserializeValueSize(signon_realm, pickle, iter, 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, pickle, iter, !size_32, false, forms);
+  }
 }
 
 bool NativeBackendKWallet::ReadGURL(const Pickle& pickle, PickleIterator* iter,
-                                    GURL* url) {
+                                    bool warn_only, GURL* url) {
   std::string url_string;
   if (!pickle.ReadString(iter, &url_string)) {
-    LOG(ERROR) << "Failed to deserialize URL";
+    if (!warn_only)
+      LOG(ERROR) << "Failed to deserialize URL.";
     *url = GURL();
     return false;
   }
diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.h b/chrome/browser/password_manager/native_backend_kwallet_x.h
index 6774a8e..9bcae94 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x.h
+++ b/chrome/browser/password_manager/native_backend_kwallet_x.h
@@ -64,6 +64,18 @@ class NativeBackendKWallet : public PasswordStoreX::NativeBackend {
   bool InitWithBus(scoped_refptr<dbus::Bus> optional_bus);
 
   // Deserializes a list of PasswordForms from the wallet.
+  // |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.
+  static bool DeserializeValueSize(const std::string& signon_realm,
+                                   const Pickle& pickle,
+                                   const PickleIterator& iter,
+                                   bool size_32, bool warn_only,
+                                   PasswordFormList* forms);
+
+  // Deserializes a list of PasswordForms from the wallet.
   static void DeserializeValue(const std::string& signon_realm,
                                const Pickle& pickle,
                                PasswordFormList* forms);
@@ -129,12 +141,13 @@ class NativeBackendKWallet : public PasswordStoreX::NativeBackend {
                                    const std::string& realm);
 
   // Convenience function to read a GURL from a Pickle. Assumes the URL has
-  // been written as a std::string. Returns true on success.
-  static bool ReadGURL(const Pickle& pickle, PickleIterator* iter, GURL* url);
+  // been written as a UTF-8 string. Returns true on success.
+  static bool ReadGURL(const Pickle& pickle, PickleIterator* iter,
+                       bool warn_only, GURL* url);
 
   // 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.)
-  static const int kPickleVersion = 0;
+  static const int kPickleVersion = 1;
 
   // Name of the folder to store passwords in.
   static const char kKWalletFolder[];
diff --git a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
index dbdcd20..d13befd 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
@@ -132,7 +132,54 @@ class NativeBackendKWalletStub : public NativeBackendKWallet {
   using NativeBackendKWallet::DeserializeValue;
 };
 
-class NativeBackendKWalletTest : public testing::Test {
+// Provide some test forms to avoid having to set them up in each test.
+class NativeBackendKWalletTestBase : public testing::Test {
+ protected:
+  NativeBackendKWalletTestBase() {
+    form_google_.origin = GURL("http://www.google.com/");
+    form_google_.action = GURL("http://www.google.com/login");
+    form_google_.username_element = UTF8ToUTF16("user");
+    form_google_.username_value = UTF8ToUTF16("joeschmoe");
+    form_google_.password_element = UTF8ToUTF16("pass");
+    form_google_.password_value = UTF8ToUTF16("seekrit");
+    form_google_.submit_element = UTF8ToUTF16("submit");
+    form_google_.signon_realm = "Google";
+
+    form_isc_.origin = GURL("http://www.isc.org/");
+    form_isc_.action = GURL("http://www.isc.org/auth");
+    form_isc_.username_element = UTF8ToUTF16("id");
+    form_isc_.username_value = UTF8ToUTF16("janedoe");
+    form_isc_.password_element = UTF8ToUTF16("passwd");
+    form_isc_.password_value = UTF8ToUTF16("ihazabukkit");
+    form_isc_.submit_element = UTF8ToUTF16("login");
+    form_isc_.signon_realm = "ISC";
+  }
+
+  void CheckPasswordForm(const PasswordForm& expected,
+                         const PasswordForm& actual);
+
+  PasswordForm form_google_;
+  PasswordForm form_isc_;
+};
+
+void NativeBackendKWalletTestBase::CheckPasswordForm(
+    const PasswordForm& expected, const PasswordForm& actual) {
+  EXPECT_EQ(expected.origin, actual.origin);
+  EXPECT_EQ(expected.password_value, actual.password_value);
+  EXPECT_EQ(expected.action, actual.action);
+  EXPECT_EQ(expected.username_element, actual.username_element);
+  EXPECT_EQ(expected.username_value, actual.username_value);
+  EXPECT_EQ(expected.password_element, actual.password_element);
+  EXPECT_EQ(expected.submit_element, actual.submit_element);
+  EXPECT_EQ(expected.signon_realm, actual.signon_realm);
+  EXPECT_EQ(expected.ssl_valid, actual.ssl_valid);
+  EXPECT_EQ(expected.preferred, actual.preferred);
+  // We don't check the date created. It varies.
+  EXPECT_EQ(expected.blacklisted_by_user, actual.blacklisted_by_user);
+  EXPECT_EQ(expected.scheme, actual.scheme);
+}
+
+class NativeBackendKWalletTest : public NativeBackendKWalletTestBase {
  protected:
   NativeBackendKWalletTest()
       : ui_thread_(BrowserThread::UI, &message_loop_),
@@ -158,12 +205,10 @@ class NativeBackendKWalletTest : public testing::Test {
     event->Signal();
   }
 
-  // Utilities to help verify expectations.
+  // Utilities to help verify sets of expectations.
   typedef std::vector<
               std::pair<std::string,
                         std::vector<const PasswordForm*> > > ExpectationArray;
-  void CheckPasswordForm(const PasswordForm& expected,
-                         const PasswordForm& actual);
   void CheckPasswordForms(const std::string& folder,
                           const ExpectationArray& sorted_expected);
 
@@ -186,10 +231,6 @@ class NativeBackendKWalletTest : public testing::Test {
 
   TestKWallet wallet_;
 
-  // Provide some test forms to avoid having to set them up in each test.
-  PasswordForm form_google_;
-  PasswordForm form_isc_;
-
  private:
   dbus::Response* KLauncherMethodCall(
       dbus::MethodCall* method_call, testing::Unused);
@@ -201,24 +242,6 @@ class NativeBackendKWalletTest : public testing::Test {
 void NativeBackendKWalletTest::SetUp() {
   ASSERT_TRUE(db_thread_.Start());
 
-  form_google_.origin = GURL("http://www.google.com/");
-  form_google_.action = GURL("http://www.google.com/login");
-  form_google_.username_element = UTF8ToUTF16("user");
-  form_google_.username_value = UTF8ToUTF16("joeschmoe");
-  form_google_.password_element = UTF8ToUTF16("pass");
-  form_google_.password_value = UTF8ToUTF16("seekrit");
-  form_google_.submit_element = UTF8ToUTF16("submit");
-  form_google_.signon_realm = "Google";
-
-  form_isc_.origin = GURL("http://www.isc.org/");
-  form_isc_.action = GURL("http://www.isc.org/auth");
-  form_isc_.username_element = UTF8ToUTF16("id");
-  form_isc_.username_value = UTF8ToUTF16("janedoe");
-  form_isc_.password_element = UTF8ToUTF16("passwd");
-  form_isc_.password_value = UTF8ToUTF16("ihazabukkit");
-  form_isc_.submit_element = UTF8ToUTF16("login");
-  form_isc_.signon_realm = "ISC";
-
   dbus::Bus::Options options;
   options.bus_type = dbus::Bus::SESSION;
   mock_session_bus_ = new dbus::MockBus(options);
@@ -416,23 +439,6 @@ dbus::Response* NativeBackendKWalletTest::KWalletMethodCall(
   return response;
 }
 
-void NativeBackendKWalletTest::CheckPasswordForm(const PasswordForm& expected,
-                                                 const PasswordForm& actual) {
-  EXPECT_EQ(expected.origin, actual.origin);
-  EXPECT_EQ(expected.password_value, actual.password_value);
-  EXPECT_EQ(expected.action, actual.action);
-  EXPECT_EQ(expected.username_element, actual.username_element);
-  EXPECT_EQ(expected.username_value, actual.username_value);
-  EXPECT_EQ(expected.password_element, actual.password_element);
-  EXPECT_EQ(expected.submit_element, actual.submit_element);
-  EXPECT_EQ(expected.signon_realm, actual.signon_realm);
-  EXPECT_EQ(expected.ssl_valid, actual.ssl_valid);
-  EXPECT_EQ(expected.preferred, actual.preferred);
-  // We don't check the date created. It varies.
-  EXPECT_EQ(expected.blacklisted_by_user, actual.blacklisted_by_user);
-  EXPECT_EQ(expected.scheme, actual.scheme);
-}
-
 void NativeBackendKWalletTest::CheckPasswordForms(
     const std::string& folder, const ExpectationArray& sorted_expected) {
   EXPECT_TRUE(wallet_.hasFolder(folder));
@@ -1011,3 +1017,72 @@ TEST_F(NativeBackendKWalletTest, DISABLED_DeleteMigratedPasswordIsIsolated) {
     CheckPasswordForms("Chrome Form Data (24)", expected);
   }
 }
+
+class NativeBackendKWalletPickleTest : public NativeBackendKWalletTestBase {
+ protected:
+  void CreateVersion0Pickle(bool size_32,
+                            const PasswordForm& form,
+                            Pickle* pickle);
+  void CheckVersion0Pickle(bool size_32, PasswordForm::Scheme scheme);
+};
+
+void NativeBackendKWalletPickleTest::CreateVersion0Pickle(
+    bool size_32, const PasswordForm& form, Pickle* pickle) {
+  const int kPickleVersion0 = 0;
+  pickle->WriteInt(kPickleVersion0);
+  if (size_32)
+    pickle->WriteUInt32(1);  // Size of form list. 32 bits.
+  else
+    pickle->WriteUInt64(1);  // Size of form list. 64 bits.
+  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.ToTimeT());
+}
+
+void NativeBackendKWalletPickleTest::CheckVersion0Pickle(
+    bool size_32, PasswordForm::Scheme scheme) {
+  Pickle pickle;
+  PasswordForm form = form_google_;
+  form.scheme = scheme;
+  CreateVersion0Pickle(size_32, form, &pickle);
+  std::vector<PasswordForm*> form_list;
+  NativeBackendKWalletStub::DeserializeValue(form.signon_realm,
+                                             pickle, &form_list);
+  EXPECT_EQ(1u, form_list.size());
+  if (form_list.size() > 0)
+    CheckPasswordForm(form, *form_list[0]);
+  STLDeleteElements(&form_list);
+}
+
+// We try both SCHEME_HTML and SCHEME_BASIC since the scheme is stored right
+// after the size in the pickle, so it's what gets read as part of the count
+// when reading 32-bit pickles on 64-bit systems. SCHEME_HTML is 0 (so we'll
+// detect errors later) while SCHEME_BASIC is 1 (so we'll detect it then). We
+// try both 32-bit and 64-bit pickles since only one will be the "other" size
+// for whatever architecture we're running on, but we want to make sure we can
+// read all combinations in any event.
+
+TEST_F(NativeBackendKWalletPickleTest, ReadsOld32BitHTMLPickles) {
+  CheckVersion0Pickle(true, PasswordForm::SCHEME_HTML);
+}
+
+TEST_F(NativeBackendKWalletPickleTest, ReadsOld32BitHTTPPickles) {
+  CheckVersion0Pickle(true, PasswordForm::SCHEME_BASIC);
+}
+
+TEST_F(NativeBackendKWalletPickleTest, ReadsOld64BitHTMLPickles) {
+  CheckVersion0Pickle(false, PasswordForm::SCHEME_HTML);
+}
+
+TEST_F(NativeBackendKWalletPickleTest, ReadsOld64BitHTTPPickles) {
+  CheckVersion0Pickle(false, PasswordForm::SCHEME_BASIC);
+}