[OSCrypt] KeyStorageLinux supports provided TaskRunners

While we need to jump to a specific thread and back, OSCrypt remains a
synchronous API, therefore we need to wait on the result. An exception
for KeyStorageLinux is made to allow for blocking.

We wrap the Init() and GetKey() methods, such that they are called on
preferred TaskRunner for the targeted backend.

OSCrypt itself has no expectation for which thread it's going to be called
on. It might even already be the thread that the backend requires (e.g. UI).
We avoid a deadlock by only posting the task if the TaskRunner is
different than the current one.

Bug: 782851
Change-Id: I48ae623ce04da23a5e79d0c1e52890df8a8ef79e
Reviewed-on: https://chromium-review.googlesource.com/758854
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520898}

base/threading/thread_restrictions.h

@@ -13,6 +13,7 @@
 class BrowserProcessImpl;
 class HistogramSynchronizer;
 class NativeBackendKWallet;
+class KeyStorageLinux;
 
 namespace android_webview {
 class AwFormDatabaseService;
@@ -288,6 +289,7 @@ class BASE_EXPORT ScopedAllowBaseSyncPrimitivesOutsideBlockingScope {
   FRIEND_TEST_ALL_PREFIXES(
       ThreadRestrictionsTest,
       ScopedAllowBaseSyncPrimitivesOutsideBlockingScopeResetsState);
+  friend class ::KeyStorageLinux;
 
   ScopedAllowBaseSyncPrimitivesOutsideBlockingScope()
       EMPTY_BODY_IF_DCHECK_IS_OFF;

components/os_crypt/BUILD.gn

@@ -157,13 +157,15 @@ source_set("unit_tests") {
     ":os_crypt",
     ":test_support",
     "//base",
+    "//base/test:test_support",
     "//crypto",
     "//testing/gmock",
     "//testing/gtest",
   ]
 
   if (is_desktop_linux) {
     sources += [
+      "key_storage_linux_unittest.cc",
       "key_storage_util_linux_unittest.cc",
       "os_crypt_linux_unittest.cc",
     ]

components/os_crypt/key_storage_linux.cc

@@ -4,9 +4,14 @@
 
 #include "components/os_crypt/key_storage_linux.h"
 
+#include "base/bind.h"
 #include "base/environment.h"
 #include "base/logging.h"
 #include "base/nix/xdg_util.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task_runner_util.h"
+#include "base/threading/thread_restrictions.h"
 #include "components/os_crypt/key_storage_config_linux.h"
 #include "components/os_crypt/key_storage_util_linux.h"
 
@@ -28,6 +33,30 @@ const char KeyStorageLinux::kFolderName[] = "Chromium Keys";
 const char KeyStorageLinux::kKey[] = "Chromium Safe Storage";
 #endif
 
+namespace {
+
+// Copies the password value from |result| to |password| and notifies on
+// |on_password_received| that the result is ready.
+void OnPasswordReceived(base::WaitableEvent* on_password_received,
+                        std::string* password,
+                        const std::string& result) {
+  *password = result;
+  if (on_password_received)
+    on_password_received->Signal();
+}
+
+// Copies the initialisation result from |result| to |success| and notifies on
+// |on_initialized| that the result is ready.
+void OnInitialized(base::WaitableEvent* on_initialized,
+                   bool* success,
+                   const bool& result) {
+  *success = result;
+  if (on_initialized)
+    on_initialized->Signal();
+}
+
+}  // namespace
+
 // static
 std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
     const os_crypt::Config& config) {
@@ -52,7 +81,7 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
   if (selected_backend == os_crypt::SelectedLinuxBackend::GNOME_ANY ||
       selected_backend == os_crypt::SelectedLinuxBackend::GNOME_LIBSECRET) {
     key_storage.reset(new KeyStorageLibsecret());
-    if (key_storage->Init()) {
+    if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using Libsecret as backend.";
       return key_storage;
     }
@@ -63,7 +92,7 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
   if (selected_backend == os_crypt::SelectedLinuxBackend::GNOME_ANY ||
       selected_backend == os_crypt::SelectedLinuxBackend::GNOME_KEYRING) {
     key_storage.reset(new KeyStorageKeyring(config.main_thread_runner));
-    if (key_storage->Init()) {
+    if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using Keyring as backend.";
       return key_storage;
     }
@@ -80,7 +109,7 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
             : base::nix::DESKTOP_ENVIRONMENT_KDE5;
     key_storage.reset(
         new KeyStorageKWallet(used_desktop_env, config.product_name));
-    if (key_storage->Init()) {
+    if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using KWallet as backend.";
       return key_storage;
     }
@@ -94,8 +123,48 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
   return nullptr;
 }
 
+bool KeyStorageLinux::WaitForInitOnTaskRunner() {
+  base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_sync_primitives;
+  base::SequencedTaskRunner* task_runner = GetTaskRunner();
+
+  // We don't need to change threads if the backend has no preference or if we
+  // are already on the right thread.
+  if (!task_runner || task_runner->RunsTasksInCurrentSequence())
+    return Init();
+
+  base::WaitableEvent initialized(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  bool success;
+  PostTaskAndReplyWithResult(
+      task_runner, FROM_HERE,
+      base::BindOnce(&KeyStorageLinux::Init, base::Unretained(this)),
+      base::BindOnce(&OnInitialized, &initialized, &success));
+  initialized.Wait();
+  return success;
+}
+
 std::string KeyStorageLinux::GetKey() {
-  // TODO(crbug.com/782851) Schedule this operation on the backend's favourite
-  // thread.
-  return GetKeyImpl();
+  base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_sync_primitives;
+  base::SequencedTaskRunner* task_runner = GetTaskRunner();
+
+  // We don't need to change threads if the backend has no preference or if we
+  // are already on the right thread.
+  if (!task_runner || task_runner->RunsTasksInCurrentSequence())
+    return GetKeyImpl();
+
+  base::WaitableEvent password_loaded(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  std::string password;
+  PostTaskAndReplyWithResult(
+      task_runner, FROM_HERE,
+      base::BindOnce(&KeyStorageLinux::GetKeyImpl, base::Unretained(this)),
+      base::BindOnce(&OnPasswordReceived, &password_loaded, &password));
+  password_loaded.Wait();
+  return password;
+}
+
+base::SequencedTaskRunner* KeyStorageLinux::GetTaskRunner() {
+  return nullptr;
 }

components/os_crypt/key_storage_linux.h

@@ -10,6 +10,10 @@
 
 #include "base/macros.h"
 
+namespace base {
+class SequencedTaskRunner;
+}
+
 namespace os_crypt {
 struct Config;
 }
@@ -30,7 +34,11 @@ class KeyStorageLinux {
   std::string GetKey();
 
  protected:
+  // Get the backend's favourite task runner, or nullptr for no preference.
+  virtual base::SequencedTaskRunner* GetTaskRunner();
+
   // Loads the key storage. Returns false if the service is not available.
+  // This iwill be called on the backend's preferred thread.
   virtual bool Init() = 0;
 
   // The implementation of GetKey() for a specific backend. This will be called
@@ -43,6 +51,9 @@ class KeyStorageLinux {
   static const char kKey[];
 
  private:
+  // Performs Init() on the backend's preferred thread.
+  bool WaitForInitOnTaskRunner();
+
   DISALLOW_COPY_AND_ASSIGN(KeyStorageLinux);
 };
 

components/os_crypt/key_storage_linux_unittest.cc

@@ -0,0 +1,57 @@
+// Copyright 2017 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/os_crypt/key_storage_linux.h"
+
+#include "base/bind.h"
+#include "base/sequenced_task_runner.h"
+#include "base/test/test_simple_task_runner.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// We use a fake to avoid calling a real backend. We'll make calls to it and
+// test that the wrapping methods post accordingly.
+class FakeKeyStorageLinux : public KeyStorageLinux {
+ public:
+  explicit FakeKeyStorageLinux(base::SequencedTaskRunner* task_runner)
+      : task_runner_(task_runner) {}
+  ~FakeKeyStorageLinux() override = default;
+
+ protected:
+  bool Init() override { return true; }
+  std::string GetKeyImpl() override { return std::string("1234"); }
+
+  base::SequencedTaskRunner* GetTaskRunner() override { return task_runner_; }
+
+ private:
+  base::SequencedTaskRunner* task_runner_;
+  DISALLOW_COPY_AND_ASSIGN(FakeKeyStorageLinux);
+};
+
+class KeyStorageLinuxTest : public testing::Test {
+ public:
+  KeyStorageLinuxTest() = default;
+  ~KeyStorageLinuxTest() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(KeyStorageLinuxTest);
+};
+
+TEST_F(KeyStorageLinuxTest, SkipPostingToSameTaskRunner) {
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner(
+      new base::TestSimpleTaskRunner());
+  FakeKeyStorageLinux key_storage(task_runner.get());
+
+  task_runner->PostTask(FROM_HERE,
+                        base::Bind(base::IgnoreResult(&KeyStorageLinux::GetKey),
+                                   base::Unretained(&key_storage)));
+
+  // This should not deadlock.
+  task_runner->RunUntilIdle();
+}
+
+TEST_F(KeyStorageLinuxTest, IgnoreTaskRunnerIfNull) {
+  FakeKeyStorageLinux key_storage(nullptr);
+  // This should not deadlock or crash.
+  ASSERT_EQ(std::string("1234"), key_storage.GetKey());
+}