0

Fix dangling pointers in DBus during Shutdown.

@alimariam reported the dangling pointer detector found a new dangling
pointer when running tests on linux Workstation.

The error is:
```
  The memory was freed at:
    allocator_shim::internal::PartitionFree()
    bluez::BluezDBusThreadManager::~BluezDBusThreadManager()
    bluez::BluezDBusThreadManager::Shutdown()
    ChromeBrowserMainPartsLinux::PostDestroyThreads()
    content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
    content::BrowserMainRunnerImpl::Shutdown()
    content::BrowserMain()
   content::RunBrowserProcessMain()
   content::ContentMainRunnerImpl::RunBrowser()
   content::ContentMainRunnerImpl::Run()
   content::RunContentProcess()
   content::ContentMain()
   ChromeMain

  The dangling raw_ptr was released at:
    base::internal::RawPtrBackupRefImpl<>::ReleaseInternal()
    dbus::ObjectManager::~ObjectManager()
    std::__Cr::__tuple_impl<>::~__tuple_impl()
    base::internal::BindState<>::Destroy()
    base::[...]::LazilyDeallocatedDeque<>::Ring::~Ring()
    base::[...]::TaskQueueImpl::UnregisterTaskQueue()
    base::[...]::SequenceManagerImpl::UnregisterTaskQueueImpl()
   base::sequence_manager::TaskQueue::ShutdownTaskQueue()
   content::BrowserTaskQueues::~BrowserTaskQueues()
   content::BrowserUIThreadScheduler::~BrowserUIThreadScheduler()
   content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
   content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
   content::BrowserTaskExecutor::Shutdown()
   content::ContentMainRunnerImpl::Shutdown()
   content::RunContentProcess()
   content::ContentMain()
   ChromeMain
```

Diagnostic:
- `bluez::BluezDBusThreadManager` owns a `dbus::Bus` as `system_bus`.
- `dbus::Bus` owns:
  - The set of `dbus::ObjectManager` as `object_manager_table_`.
  - The DBus task runner as `dbus_task_runner_`.
- The `dbus::ObjectManager` references `dbus::Bus` via `bus_`.

So far so good, the ownership is clear. The problem happens when calling
`dbus::Bus::RemoveObjectManager`. Indeed this moves the ObjectManager
out of `dbus::Bus` toward a callback to a new thread. This still works
transitively, because the dbus::Bus owns the thread. The problem happens
after a second transfer back to the original thread.

Indeed, there is a race condition possible:

Behavior without problems: -----------------------------------

┌─────────────┐                    ┌───────────┐
│Origin thread│                    │DBus thread│
└──────┬──────┘                    └─────┬─────┘
RemoveObjectManager()                    │
       │────────────────────────────────>│
       │                      RemoveObjectManagerInternal()
       │<────────────────────────────────│
RemoveObjectManagerInternalHelper()      │
~ObjectManager()                         │
       │                           ┌─────┴─────┐
Shutdown DBus Thread ─────────────>│DBus thread│
Shutdown DBus Thread <─────────────│DBus thread│
       │                           └───────────┘
      ~Bus
┌──────┴──────┐
│Origin thread│
└─────────────┘

Behavior with problems: ----------------------------------------

┌─────────────┐                    ┌───────────┐
│Origin thread│                    │DBus thread│
└──────┬──────┘                    └─────┬─────┘
RemoveObjectManager()                    │
       │────────────────────────────────>│
       │                      RemoveObjectManagerInternal()
       │                    ┌────────────│
       │                    │      ┌─────┴─────┐
Shutdown DBus Thread ─────────────>│DBus thread│
Shutdown DBus Thread <─────────────│DBus thread│
       │                    │      └───────────┘
     ~Bus()                 │
       │                    │
       │<───────────────────┘
RemoveObjectManagerInternalHelper()
~ObjectManager()
┌──────┴──────┐
│Origin thread│
└─────────────┘
-----------------------------------------------------------------

In the second case: ~Bus() is called before ~ObjectManager().

The fix is a use `ObjectManager::Cleanup()` to cleanup the raw_ptr while
the object is still transitively owned by the object it referenced.

Bug: chromium:1478759
Fixed: chromium:1478759
Change-Id: I4ac04d449ab8a7b860256c490f8ac878c1c5c7c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4839496
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1192343}
This commit is contained in:
Arthur Sonzogni
2023-09-05 09:26:45 +00:00
committed by Chromium LUCI CQ
parent faed507331
commit bcb156213b
2 changed files with 18 additions and 14 deletions

@ -50,11 +50,7 @@ scoped_refptr<ObjectManager> ObjectManager::Create(
ObjectManager::ObjectManager(Bus* bus,
const std::string& service_name,
const ObjectPath& object_path)
: bus_(bus),
service_name_(service_name),
object_path_(object_path),
setup_success_(false),
cleanup_called_(false) {
: bus_(bus), service_name_(service_name), object_path_(object_path) {
LOG_IF(FATAL, !object_path_.IsValid()) << object_path_.value();
DVLOG(1) << "Creating ObjectManager for " << service_name_
<< " " << object_path_.value();
@ -175,15 +171,20 @@ void ObjectManager::CleanUp() {
}
match_rule_.clear();
// After Cleanup(), the Bus doesn't own `this` anymore and might be deleted
// before `this`.
bus_ = nullptr;
}
bool ObjectManager::SetupMatchRuleAndFilter() {
DCHECK(bus_);
DCHECK(!setup_success_);
bus_->AssertOnDBusThread();
if (cleanup_called_)
if (cleanup_called_) {
return false;
}
DCHECK(bus_);
bus_->AssertOnDBusThread();
if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
return false;
@ -225,16 +226,17 @@ void ObjectManager::OnSetupMatchRuleAndFilterComplete(bool success) {
<< ": Failed to set up match rule.";
return;
}
DCHECK(bus_);
DCHECK(object_proxy_);
DCHECK(setup_success_);
bus_->AssertOnOriginThread();
// |object_proxy_| is no longer valid if the Bus was shut down before this
// call. Don't initiate any other action from the origin thread.
if (cleanup_called_)
if (cleanup_called_) {
return;
}
DCHECK(bus_);
bus_->AssertOnOriginThread();
object_proxy_->ConnectToSignal(
kObjectManagerInterface, kObjectManagerInterfacesAdded,

@ -328,14 +328,16 @@ class CHROME_DBUS_EXPORT ObjectManager final
// |service_name_owner_|.
void UpdateServiceNameOwner(const std::string& new_owner);
// Valid in between the constructor and `CleanUp()`.
// After Cleanup(), `this` lifetime might exceed Bus's one.
raw_ptr<Bus> bus_;
std::string service_name_;
std::string service_name_owner_;
std::string match_rule_;
ObjectPath object_path_;
raw_ptr<ObjectProxy, AcrossTasksDanglingUntriaged> object_proxy_;
bool setup_success_;
bool cleanup_called_;
bool setup_success_ = false;
bool cleanup_called_ = false;
// Maps the name of an interface to the implementation class used for
// instantiating PropertySet structures for that interface's properties.