Avoid use-after-free in FormFetcherImpl

components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc

Index: components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
diff --git a/components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc b/components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..64e9511de092a325497ee74df27589be058d41bf
--- /dev/null
+++ b/components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
@@ -0,0 +1,94 @@
+// Copyright 2016 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/password_manager/core/browser/credential_manager_password_form_manager.h"
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/fake_form_fetcher.h"
+#include "components/password_manager/core/browser/stub_form_saver.h"
+#include "components/password_manager/core/browser/stub_password_manager_client.h"
+#include "components/password_manager/core/browser/stub_password_manager_driver.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using autofill::PasswordForm;
+using ::testing::Invoke;
+
+namespace password_manager {
+
+namespace {
+
+class MockDelegate : public CredentialManagerPasswordFormManagerDelegate {
+ public:
+  MOCK_METHOD0(OnProvisionalSaveComplete, void());
+};
+
+// This is a special version of FakeFormFetcher, which cheks that it is still
+// alive after it passes results to its consumers.
+class CheckedAliveFetcher : public FakeFormFetcher {
+ public:
+  CheckedAliveFetcher() : is_alive_(true) {}
+
+  ~CheckedAliveFetcher() override { is_alive_ = false; }
+
+  void SetEmptyNonFederatedAndCheck() {
+    SetNonFederated(std::vector<const PasswordForm*>(), 0u);
+    EXPECT_TRUE(is_alive_);
+  }
+
+ private:
+  bool is_alive_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckedAliveFetcher);
+};
+
+}  // namespace
+
+class CredentialManagerPasswordFormManagerTest : public testing::Test {
+ public:
+  CredentialManagerPasswordFormManagerTest() = default;
+
+ protected:
+  // Necessary for callbacks, and for TestAutofillDriver.
+  base::MessageLoop message_loop_;
+
+  StubPasswordManagerClient client_;
+  StubPasswordManagerDriver driver_;
+
+  DISALLOW_COPY_AND_ASSIGN(CredentialManagerPasswordFormManagerTest);
+};
+
+// Test that aborting early does not cause use after free.
+TEST_F(CredentialManagerPasswordFormManagerTest, AbortEarly) {
+  PasswordForm observed_form;
+  MockDelegate delegate;
+  auto form_fetcher = base::MakeUnique<CheckedAliveFetcher>();
+  form_fetcher->Fetch();

[vasilii, 2016/12/21 11:00:49]

Fetch is called from the constructor below. What's the point?

[vabr (Chromium), 2016/12/21 11:35:25]

It is not called in the constructor. The constructor of PasswordFormManager only
calls Fetch on the FormFetcherImpl which it creates itself. If it gets another
FormFetcher passed in, it does not call Fetch() on it. The idea is that the
owner of the fetcher should control when it fetches, to avoid unnecessary
re-fetching.

[vasilii, 2016/12/21 12:10:29]

Acknowledged.

+  CredentialManagerPasswordFormManager form_manager(
+      &client_, driver_.AsWeakPtr(), observed_form,
+      base::MakeUnique<PasswordForm>(observed_form), &delegate,
+      base::MakeUnique<StubFormSaver>(), form_fetcher.get());
+
+  auto deleter = [&form_fetcher]() { form_fetcher = nullptr; };

[vasilii, 2016/12/21 11:00:49]

form_fetcher.reset();

[vabr (Chromium), 2016/12/21 11:35:25]

These two are "Effectively the same" [1]. What is the reason behind your
preference?


[1] http://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D

[vasilii, 2016/12/21 12:10:29]

So that I understand that it's a smart pointer without looking at #72. It's
optional.

[vabr (Chromium), 2016/12/21 13:41:23]

That's a pretty good reason! I never thought of that one, but it makes sense to
me. Thanks for sharing, Vasilii.

+
+  // Simulate that the PasswordStore responded to the FormFetcher. As a result,
+  // |form_manager| should call the delegate's OnProvisionalSaveComplete, which
+  // in turn should delete |form_manager|.
+  EXPECT_CALL(delegate, OnProvisionalSaveComplete()).WillOnce(Invoke(deleter));
+  form_fetcher->SetEmptyNonFederatedAndCheck();

[vasilii, 2016/12/21 11:00:49]

I'm afraid that you current test may suddenly pass on non-ASAN bots with the
current implementation if the framework sets does something with the memory
after destructor.

The object is alive iff form_fetcher != 0. Please drop CheckedAliveFetcher
entirely and use FakeFormFetcher, SetNonFederated and |form_fetcher| here.

[vabr (Chromium), 2016/12/21 11:35:25]

Let me check if I understand the tradeoff:
I agree: the current test is not guarantee to fail without the fix on non-ASAN
builds. However, what you suggest is actually guaranteed to pass without the fix
on non-ASAN builds. So this is not a drawback of the current code compared to
your proposal.

I see an advantage in your proposal in the whole test being simpler (no need to
define CheckedAliveFetcher). However, I see also a drawback of the proposal in
that it relies on FakeFormFetcher to do the same iteration over the consumers as
FormFetcherImpl does. This might not always be so, perhaps someone rewrites the
fake one to only call the first consumer, etc. Compared to that, the current
code makes sure that the fetcher tries to access its data members after calling
the consumers, and this stays true unless it is changed in this very file.

So accepting that this will always need an ASAN bot to be reliable, no matter
what we do, I am weighing the advantage against the drawback in the previous
paragraph. In my opinion, the additional code is worth being sure that the
use-after-free is exercised, but I'm happy to talk.

+
+  base::RunLoop().RunUntilIdle();
+
+  // Ultimately, |form_fetcher| should have been deleted. It just should happen
+  // after it finishes executing.
+  EXPECT_FALSE(form_fetcher);
+}
+
+}  // namespace password_manager