Avoid use-after-free in FormFetcherImpl

Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in https://crbug.com/675178#c8.

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG=675178, 621355
Committed: https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2
Cr-Commit-Position: refs/heads/master@{#440107}

[vabr (Chromium), 2016-12-20 12:01:08]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 12:01:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 12:06:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 12:06:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vabr (Chromium), 2016-12-20 13:34:19]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 13:34:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 13:44:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 13:44:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[vabr (Chromium), 2016-12-20 13:46:31]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 13:46:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-20 13:56:04]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2016-12-20 13:56:05]

Hi Vasilii,

Could I bother you with one more CL for review?

Cheers,
Vaclav

[vasilii, 2016-12-20 15:36:37]

Couple of general questions.
- An object A owns (in all the senses: memory management and delegating some
task) an object B. B makes a callback to A. Is it OK to delete B?
For me the answer is yes. CredentialManagerPasswordFormManager is an example. If
it does something after OnProvisionalSaveComplete() then it's a bug.

- Is the same applicable to PasswordFormManager and FormFetcherImpl?
No. We are already in the hybrid state where PasswordFormManager may or may not
own FormFetcherImpl. If FormFetcherImpl had had only a weak pointer instead of
std::set<Consumer*> then the bug would not have happened. We are moving in the
opposite direction and soon FormFetcherImpl will belong to someone else.

Therefore, in the current state the fix should be on
CredentialManagerPasswordFormManager level (I hope PasswordFormManager can't be
deleted the same way). I don't want to move the problem solution to
CredentialManagerPasswordFormManagerDelegate. Can you post a task to
CredentialManagerPasswordFormManager instead and from there call the delegate
synchronously?

[commit-bot: I haz the power, 2016-12-20 15:38:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 15:38:11]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-12-20 15:46:08]

On 2016/12/20 15:36:37, vasilii wrote:
> Couple of general questions.
> - An object A owns (in all the senses: memory management and delegating some
> task) an object B. B makes a callback to A. Is it OK to delete B?
> For me the answer is yes. CredentialManagerPasswordFormManager is an example.
If
> it does something after OnProvisionalSaveComplete() then it's a bug.
> 
> - Is the same applicable to PasswordFormManager and FormFetcherImpl?
> No. We are already in the hybrid state where PasswordFormManager may or may
not
> own FormFetcherImpl. If FormFetcherImpl had had only a weak pointer instead of
> std::set<Consumer*> then the bug would not have happened. We are moving in the
> opposite direction and soon FormFetcherImpl will belong to someone else.
> 
> Therefore, in the current state the fix should be on
> CredentialManagerPasswordFormManager level (I hope PasswordFormManager can't
be
> deleted the same way). I don't want to move the problem solution to
> CredentialManagerPasswordFormManagerDelegate. Can you post a task to
> CredentialManagerPasswordFormManager instead and from there call the delegate
> synchronously?

Hi Vasilii,

Sure, that would work for me. I'll do it tomorrow and ping this CL once ready.

Thanks!
Vaclav

[vabr (Chromium), 2016-12-21 08:23:42]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 08:23:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-21 09:21:38]

Hi Vasilii,

Please take a look again.

I used WeakFactory instead of deriving from SupportsWeakPtr in this case,
because the former is intended for cases where the weak pointer is only used in
the class internally.

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-12-21 09:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 09:22:04]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:20:
#include "components/password_manager/core/browser/form_saver.h"
I think both headers are unused.

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:73:
form_fetcher->Fetch();
Fetch is called from the constructor below. What's the point?

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:79:
auto deleter = [&form_fetcher]() { form_fetcher = nullptr; };
form_fetcher.reset();

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:85:
form_fetcher->SetEmptyNonFederatedAndCheck();
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.

https://codereview.chromium.org/2592653003/diff/80001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/2592653003/diff/80001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/credential_manager.mm:15: #include
"components/password_manager/core/browser/form_saver.h"
unused?

[vabr (Chromium), 2016-12-21 11:34:15]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 11:34:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-21 11:35:26]

Thanks, Vasilii!

I'm afraid I pushed back on most of the comments this time. I'm happy to talk in
person as well, if you find that helpful.

Thanks for your time reviewing this!
Vaclav

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:20:
#include "components/password_manager/core/browser/form_saver.h"
On 2016/12/21 11:00:49, vasilii wrote:
> I think both headers are unused.

They are not. On line 82, there are two nullptr passed to a function call in
place of pointers to FormFetcher and FormSaver. In particular, the FormSaver is
a unique_ptr, so there the header is needed to know how to create it based on
nullptr. The other one is raw, so I can drop that header. (Done.)

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:73:
form_fetcher->Fetch();
On 2016/12/21 11:00:49, vasilii wrote:
> Fetch is called from the constructor below. What's the point?

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.

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:79:
auto deleter = [&form_fetcher]() { form_fetcher = nullptr; };
On 2016/12/21 11:00:49, vasilii wrote:
> form_fetcher.reset();

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

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:85:
form_fetcher->SetEmptyNonFederatedAndCheck();
On 2016/12/21 11:00:49, vasilii wrote:
> 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.

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.

https://codereview.chromium.org/2592653003/diff/80001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/2592653003/diff/80001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/credential_manager.mm:15: #include
"components/password_manager/core/browser/form_saver.h"
On 2016/12/21 11:00:49, vasilii wrote:
> unused?

One of them. Done.

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

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:73:
form_fetcher->Fetch();
On 2016/12/21 11:35:25, vabr (Chromium) wrote:
> On 2016/12/21 11:00:49, vasilii wrote:
> > Fetch is called from the constructor below. What's the point?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:79:
auto deleter = [&form_fetcher]() { form_fetcher = nullptr; };
On 2016/12/21 11:35:25, vabr (Chromium) wrote:
> On 2016/12/21 11:00:49, vasilii wrote:
> > form_fetcher.reset();
> 
> 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

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

[commit-bot: I haz the power, 2016-12-21 12:28:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 12:28:16]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-12-21 13:40:22]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 13:40:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks, Vasilii!

Please have another look.

I addressed also your comment about FakeFormFetcher, which you shared offline.

Cheers,
Vaclav

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:79:
auto deleter = [&form_fetcher]() { form_fetcher = nullptr; };
On 2016/12/21 12:10:29, vasilii wrote:
> On 2016/12/21 11:35:25, vabr (Chromium) wrote:
> > On 2016/12/21 11:00:49, vasilii wrote:
> > > form_fetcher.reset();
> > 
> > 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
> 
> So that I understand that it's a smart pointer without looking at #72. It's
> optional.

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

[vasilii, 2016-12-21 13:55:30]

lgtm

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:64:
// in turn should delete |form_manager|.
It should delete |form_fetcher|.

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:68:
// delete after free during SetNonFederated.
typo?

[vabr (Chromium), 2016-12-21 13:59:20]

Thanks!

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:64:
// in turn should delete |form_manager|.
On 2016/12/21 13:55:30, vasilii wrote:
> It should delete |form_fetcher|.

Done.

https://codereview.chromium.org/2592653003/diff/120001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:68:
// delete after free during SetNonFederated.
On 2016/12/21 13:55:30, vasilii wrote:
> typo?

Done.

[vabr (Chromium), 2016-12-21 13:59:25]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-12-21 13:59:25]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2592653003/#ps140001
(title: "Typos")

[commit-bot: I haz the power, 2016-12-21 13:59:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 15:24:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482328765483870,
"parent_rev": "6462cacb2bb55c34ca78626b290e8d737e0c4cf2", "commit_rev":
"f3ee0ea0f42aa55499071a6eaeacea1d68aa64b1"}

[commit-bot: I haz the power, 2016-12-21 15:25:23]

Description was changed from

==========
Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in https://crbug.com/675178#c8.

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG=675178,621355
==========

to

==========
Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in https://crbug.com/675178#c8.

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG=675178,621355

Review-Url: https://codereview.chromium.org/2592653003
==========

[commit-bot: I haz the power, 2016-12-21 15:25:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-21 15:28:27]

Description was changed from

==========
Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in https://crbug.com/675178#c8.

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG=675178,621355

Review-Url: https://codereview.chromium.org/2592653003
==========

to

==========
Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in https://crbug.com/675178#c8.

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG=675178,621355

Committed: https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2
Cr-Commit-Position: refs/heads/master@{#440107}
==========

[commit-bot: I haz the power, 2016-12-21 15:28:29]

Patchset 8 (id:??) landed as
https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2
Cr-Commit-Position: refs/heads/master@{#440107}