PasswordManager::MoveOwnedSubmittedManager invalidates PasswordManager::form_managers_ |
|||||||||
Issue description
This code in PasswordManager::MoveOwnedSubmittedManager introduces null members in PasswordManager::form_managers_ while also relying on all of them being non-null:
for (std::unique_ptr<NewPasswordFormManager>& manager : form_managers_) {
if (manager->is_submitted())
return std::move(manager);
}
As a result, MoveOwnedSubmittedManager must not be called twice.
There seems to be no obvious guarantee that MoveOwnedSubmittedManager is only called once in the lifetime of a PasswordManager. Similarly, it is not clear whether any other part of PasswordManager which relies on all elements of form_managers_being non-null can be called after MoveOwnedSubmittedManager, now or in the future.
Therefore, MoveOwnedSubmittedManager should either delete the moved entry, or perhaps clear all form_managers_ (if the idea is indeed that this is something happening once towards the end of PasswordManager).
,
Sep 4
Thanks, Vadym, SGTM!
,
Sep 4
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c4b5d63d1dde6da23424531f1bb8f1a5ed6ecda5 commit c4b5d63d1dde6da23424531f1bb8f1a5ed6ecda5 Author: Vadym Doroshenko <dvadym@chromium.org> Date: Thu Sep 06 09:01:20 2018 Fix in PasswordManager. When new parsing for saving enabled PasswordManager::MoveOwnedSubmittedManager tries to find submitted NewPasswordFormManager and if it's found it's moved. As result there is nullptr in |form_managers_|. That would crash this function if it's called twice. This CL makes this function behaves gracefully - to remove nullptr. Bug: 880229 Change-Id: I7f91a113dc33a6a2d0b483485a66657bec9e6d28 Reviewed-on: https://chromium-review.googlesource.com/1206591 Reviewed-by: Vaclav Brozek <vabr@chromium.org> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#589116} [modify] https://crrev.com/c4b5d63d1dde6da23424531f1bb8f1a5ed6ecda5/components/password_manager/core/browser/password_manager.cc [modify] https://crrev.com/c4b5d63d1dde6da23424531f1bb8f1a5ed6ecda5/components/password_manager/core/browser/password_manager_unittest.cc
,
Sep 11
,
Sep 11
Without this CL Chrome would crash in almost the same cases as on Bug 880247. This is a very simply fix.
,
Sep 11
branch:3538
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e31ee3f99dad50fd17cfd9758dc9a266bbe9705 commit 9e31ee3f99dad50fd17cfd9758dc9a266bbe9705 Author: Vadym Doroshenko <dvadym@chromium.org> Date: Wed Sep 12 10:56:56 2018 Fix in PasswordManager. When new parsing for saving enabled PasswordManager::MoveOwnedSubmittedManager tries to find submitted NewPasswordFormManager and if it's found it's moved. As result there is nullptr in |form_managers_|. That would crash this function if it's called twice. This CL makes this function behaves gracefully - to remove nullptr. TBR=dvadym@chromium.org (cherry picked from commit c4b5d63d1dde6da23424531f1bb8f1a5ed6ecda5) Bug: 880229 Change-Id: I7f91a113dc33a6a2d0b483485a66657bec9e6d28 Reviewed-on: https://chromium-review.googlesource.com/1206591 Reviewed-by: Vaclav Brozek <vabr@chromium.org> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589116} Reviewed-on: https://chromium-review.googlesource.com/1221208 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#317} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/9e31ee3f99dad50fd17cfd9758dc9a266bbe9705/components/password_manager/core/browser/password_manager.cc [modify] https://crrev.com/9e31ee3f99dad50fd17cfd9758dc9a266bbe9705/components/password_manager/core/browser/password_manager_unittest.cc
,
Oct 1
,
Nov 29
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by dvadym@chromium.org
, Sep 4