diff --git a/content/browser/service_worker/service_worker_version_unittest.cc b/content/browser/service_worker/service_worker_version_unittest.cc index 30fbf766ae697..6109c9cf3c77b 100644 --- a/content/browser/service_worker/service_worker_version_unittest.cc +++ b/content/browser/service_worker/service_worker_version_unittest.cc @@ -26,7 +26,10 @@ #include "content/browser/service_worker/fake_embedded_worker_instance_client.h" #include "content/browser/service_worker/fake_service_worker.h" #include "content/browser/service_worker/service_worker_client.h" +#include "content/browser/service_worker/service_worker_container_host.h" #include "content/browser/service_worker/service_worker_context_core.h" +#include "content/browser/service_worker/service_worker_context_wrapper.h" +#include "content/browser/service_worker/service_worker_host.h" #include "content/browser/service_worker/service_worker_info.h" #include "content/browser/service_worker/service_worker_ping_controller.h" #include "content/browser/service_worker/service_worker_registration.h" @@ -2365,5 +2368,117 @@ TEST_P(ServiceWorkerVersionTest, EXPECT_FALSE(is_update_scheduled()); } +// A ServiceWorkerContextObserverSynchronous that, on its first OnStoppedSync +// notification, calls ServiceWorkerContextWrapper::FinishedExternalRequest() +// for the dying version. This models exactly what the //extensions sync +// observer chain does: +// ServiceWorkerState::OnStoppedSync +// -> ServiceWorkerTaskQueue::OnWorkerStop +// -> ProcessManager::StopTrackingServiceWorkerRunningInstance +// -> EventRouter::OnStoppedTrackingServiceWorkerInstance +// -> EventAckData::ClearUnackedEventsForWorker +// -> ServiceWorkerContextWrapper::FinishedExternalRequest() +// The one-shot guard models ProcessManager early-returning on the second +// re-entrant call (the worker was already removed from +// all_running_extension_workers_ on the first pass). +class ReentrantFinishExternalRequestObserver + : public ServiceWorkerContextObserverSynchronous { + public: + explicit ReentrantFinishExternalRequestObserver( + ServiceWorkerContextWrapper* wrapper) + : wrapper_(wrapper), + request_uuid_(base::Uuid::GenerateRandomV4()) {} + + void OnStoppedSync( + int64_t version_id, + const GURL& scope, + const blink::ServiceWorkerToken& service_worker_token) override { + if (fired_) { + return; + } + fired_ = true; + // The inner ~ServiceWorkerVersion -> RemoveLiveVersion will re-iterate the + // same sync observer list, which trips ObserverList's DUMP_WILL_BE_CHECK + // re-entrancy guard (fatal in non-official builds, DumpWithoutCrashing in + // official builds). Removing `this` first makes the list empty so the + // inner begin() short-circuits without constructing an Iter, matching the + // official-build behaviour where execution continues past the guard. + wrapper_->RemoveSyncObserver(this); + // FinishedExternalRequest() wraps GetLiveVersion() in a scoped_refptr. + // When called from inside ~ServiceWorkerVersion (refcount==0), this + // revives a dying object: AddRef 0->1, then on scope exit Release 1->0 + // -> second `delete this`. + wrapper_->FinishedExternalRequest(version_id, request_uuid_); + } + + bool fired() const { return fired_; } + + private: + raw_ptr wrapper_; + base::Uuid request_uuid_; + bool fired_ = false; +}; + +// Regression test: ServiceWorkerContextCore::RemoveLiveVersion() is called +// from inside ~ServiceWorkerVersion() (refcount already 0) and dispatches the +// synchronous OnStoppedSync observer chain *before* erasing the dying object +// from live_versions_. If a sync observer re-enters +// ServiceWorkerContextWrapper::FinishedExternalRequest() (as the //extensions +// EventAckData chain does), GetLiveVersion() returns the dying object and a +// scoped_refptr is taken on it -> AddRef on refcount==0 -> double-free. +// +// The kStarting/kRunning destructor path that this models is documented in +// ~ServiceWorkerVersion() itself ("the user closed the tab before the SW could +// start up"). +TEST_P(ServiceWorkerVersionTest, RemoveLiveVersionReentrantDoubleFree) { + // Start the worker so running_status() != kStopped (RemoveLiveVersion() only + // dispatches OnStoppedSync in that case). + ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk, + StartServiceWorker(version_.get())); + ASSERT_EQ(blink::EmbeddedWorkerStatus::kRunning, version_->running_status()); + + // InitializeGlobalScope() created a self-referencing ServiceWorkerObjectHost + // (so that `self.serviceWorker` works in the renderer). Drop it so that the + // test fixture's `version_` becomes the last ref and we can enter the + // kRunning destructor path. In production this self-ref is broken by + // worker_host_.reset() in OnStoppedInternal(), which never runs on the + // kStarting/kRunning destructor path either. + version_->worker_host() + ->container_host() + ->version_object_manager() + .RemoveHost(version_->version_id()); + ASSERT_TRUE(version_->HasOneRef()) + << "test precondition: fixture must hold the last ref"; + + // Register the sync observer that re-enters FinishedExternalRequest(), like + // the //extensions chain does for unacked events. + ReentrantFinishExternalRequestObserver observer(helper_->context_wrapper()); + helper_->context_wrapper()->AddSyncObserver(&observer); + + // Drop the last ref while running_status() == kRunning. + // + // ~ServiceWorkerVersion() + // -> RemoveLiveVersion() [entry still in live_versions_] + // -> OnStoppedSync() + // -> FinishedExternalRequest() + // -> scoped_refptr(GetLiveVersion(id)) AddRef 0->1 + // -> ~scoped_refptr Release 1->0 + // -> ~ServiceWorkerVersion() second delete + // -> RemoveLiveVersion() + // -> live_versions_.erase() frees rb-tree node + // -> member dtors / operator delete frees the version slot + // -> live_versions_.erase(stale_it) UAF on freed map node + // -> embedded_worker_->RemoveObserver(this) UAF on freed members + // + // Under ASAN this manifests as heap-use-after-free; in release builds the + // RefCountedBase::AddRef() guard against revival is DCHECK-only, so the + // double-free is reachable. + version_ = nullptr; + + // If we got here, no double-free occurred (e.g. the entry was erased before + // the sync observer loop). + EXPECT_TRUE(observer.fired()); +} + } // namespace service_worker_version_unittest } // namespace content