Fix race condition on OSCrypt linux

components/os_crypt/os_crypt_linux.cc

Index: components/os_crypt/os_crypt_linux.cc
diff --git a/components/os_crypt/os_crypt_linux.cc b/components/os_crypt/os_crypt_linux.cc
index 86503382b4f43466b918746aabb6097aee847413..c2a17a5bdd8028a5f713fda00c86880f2c1ef2a1 100644
--- a/components/os_crypt/os_crypt_linux.cc
+++ b/components/os_crypt/os_crypt_linux.cc
@@ -72,6 +72,10 @@ KeyStorageLinux* GetKeyStorage() {
   return g_cache.Get().key_storage_cache.get();
 }
 
+// Pointer to a function that creates and returns the |KeyStorage| instance to
+// be used. The function maintains ownership of the pointer.
+KeyStorageLinux* (*g_key_storage_provider)() = &GetKeyStorage;
+
 // Returns a cached string of "peanuts". Is thread-safe.
 std::string* GetPasswordV10() {
   base::AutoLock auto_lock(g_cache.Get().lock);
@@ -87,16 +91,14 @@ std::string* GetPasswordV11() {
   base::AutoLock auto_lock(g_cache.Get().lock);
   if (!g_cache.Get().is_password_v11_cached) {
     g_cache.Get().password_v11_cache.reset(
-        GetKeyStorage() ? new std::string(GetKeyStorage()->GetKey()) : nullptr);
+        g_key_storage_provider()
+            ? new std::string(g_key_storage_provider()->GetKey())
+            : nullptr);
     g_cache.Get().is_password_v11_cached = true;
   }
   return g_cache.Get().password_v11_cache.get();
 }
 
-// Pointer to a function that creates and returns the |KeyStorage| instance to
-// be used. The function maintains ownership of the pointer.
-KeyStorageLinux* (*g_key_storage_provider)() = &GetKeyStorage;
-
 // Pointers to functions that return a password for deriving the encryption key.
 // One function for each supported password version (see Version enum).
 std::string* (*g_get_password[])() = {
@@ -152,7 +154,8 @@ bool OSCrypt::EncryptString(const std::string& plaintext,
 
   // If a |KeyStorage| is available, use a password backed by the |KeyStorage|.

[Lei Zhang, 2016/09/28 15:35:06]

The comment is a bit out of date.

[cfroussios, 2016/09/28 16:08:17]

Done.

   // Otherwise use the hardcoded password.
-  Version version = g_key_storage_provider() ? Version::V11 : Version::V10;
+  Version version =
+      GetEncryptionKey(Version::V11) ? Version::V11 : Version::V10;
 
   std::unique_ptr<crypto::SymmetricKey> encryption_key(
       GetEncryptionKey(version));

[Lei Zhang, 2016/09/28 15:35:06]

Aren't we calling GetEncryptionKey() twice in a row now?

[Lei Zhang, 2016/09/28 15:43:21]

So I was thinking the code below does the same but avoids the second
GetEncryptionKey() call if the first succeeds. WDYT?

Version version = Version::V11;
std::unique_ptr<crypto::SymmetricKey> encryption_key =
    GetEncryptionKey(version);
if (!encryption_key) {
  version = Version::V10;
  encryption_key = GetEncryptionKey(version);
}
if (!encryption_key)
  return false;

[cfroussios, 2016/09/28 16:08:16]

I had a fix ready for this, but I was thinking that I wanted |encryption_key| to
continue and explicitly depend on |version|. I'm going to go with your code.