New issue
Advanced search Search tips

Issue 681752 link

Starred by 2 users
Status: Archived
Owner: ----
Closed: Jan 10
Cc:
alemate@chromium.org
hidehiko@chromium.org
nya@chromium.org
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

FakeChromeUserManager leaves dangling pointers in ProfileHelper if added users are not explicitly removed

Project Member Reported by nya@chromium.org, Jan 17 2017 
FakeChromeUserManager::AddUser() calls ProfileHelper::SetProfileToUserMappingForTesting() to inject a new user to ProfileHelper. However, FakeChromeUserManager does not remove the added users from ProfileHelper unless we explicitly call FakeChromeUserManager::RemoveUserFromList(), which ends up leaving dangling pointers in ProfileHelper.user_list_for_testing_.

Actually this behavior caused use-after-free in crrev.com/2638713002 because ProfileHelper::GetUserByProfile() is called in VolumeManager::Shutdown() after a FakeChromeUserManager is destructed.

Existing tests often call AddUser() without cleaning up with RemoveUserFromList(), so dangling pointers are possibly there without being realized.
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acfaa8216d01efc4da558881290577e79b4c950b

commit acfaa8216d01efc4da558881290577e79b4c950b
Author: nya <nya@chromium.org>
Date: Tue Jan 17 06:50:57 2017

Avoid leaving dangling pointers to User in ProfileHelper.

FakeChromeUserManager::AddUser() calls
ProfileHelper::SetProfileToUserMappingForTesting() to inject a new user
to ProfileHelper. However, FakeChromeUserManager does not remove
the added users from ProfileHelper unless we explicitly call
FakeChromeUserManager::RemoveUserFromList(), which ends up leaving
dangling pointers in ProfileHelper.user_list_for_testing_.

This caused use-after-free in crrev.com/2638713002 because
ProfileHelper::GetUserByProfile() is called in VolumeManager::Shutdown()
after a FakeChromeUserManager is destructed.

BUG= chromium:681752 
TEST=browser_tests --gtest_filter='ArcSessionManagerTest.*'  # with ASAN

Review-Url: https://codereview.chromium.org/2639483003
Cr-Commit-Position: refs/heads/master@{#443993}

[modify] https://crrev.com/acfaa8216d01efc4da558881290577e79b4c950b/chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc

Comment 2 by nya@chromium.org, Jan 17 2017 

#1 fixed use-after-free in ArcSessionManagerTest by calling RemoveUserFromList() explicitly, but I guess we want to resolve possible bugs by either:

1. automatically unregister users from ProfileHelper in the destructor of FakeChromeUserManager, or
2. insert DCHECK in the destructor of FakeChromeUserManager to catch the case where RemoveUserFromList() is not called explicitly.

Comment 3 by benhenry@google.com, Jan 10

Status: Archived (was: Untriaged)
Archiving P3s older than 1 year with no owner or component.
►

Sign in to add a comment