New issue
Advanced search Search tips

Issue 875376 link

Starred by 1 user
Status: Fixed
Owner:
lazyboy@chromium.org
Closed: Sep 8
Cc:
falken@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

SWContext raw pointer usage in extensions is potentially race-y

Project Member Reported by lazyboy@chromium.org, Aug 17 
In extensions code, we retrieve a raw ServiceWorkerContext pointer
through //content and pass it to IO thread to work with the context.
This seems troublesome as the underlying ref-counted SWContextWrapper
doesn't add ref in this scenario.

Example:
void ServiceWorkerTaskQueue::RunTaskAfterStartWorker(
    LazyContextId* context_id,
    LazyContextTaskQueue::PendingTask task) {
...
  content::StoragePartition* partition =
      BrowserContext::GetStoragePartitionForSite(
          context_id->browser_context(), context_id->service_worker_scope());
  content::ServiceWorkerContext* service_worker_context =
      partition->GetServiceWorkerContext();

  content::BrowserThread::PostTask(
      content::BrowserThread::IO, FROM_HERE,
      base::BindOnce(
          &GetServiceWorkerInfoOnIO, context_id->service_worker_scope(),
          context_id->extension_id(), service_worker_context, std::move(task)));
}

void GetServiceWorkerInfoOnIO(.., content::ServiceWorkerContext* context, ..) {
  // No guarantee that |context| is valid at this point.
}

The issue here is that the ref-counted class ServiceWorkerContextWrapper
is internal to //content and isn't expose.

Current plan is to expose a //content method to run a task while keeping
a ServiceWorkerContext* alive.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 22 

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

commit 415f86f67e9e7480512c79a86d1ac136b6048832
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Wed Aug 22 20:19:13 2018

Expose ability to run tasks keeping ServiceWorkerContext alive.

//content doesn't expose RefCounted-ness of ServiceWorkerContexts.
This means there's no safe way to run IO thread tasks on a
ServiceWorkerContext that was obtained on UI thread, typically
through StoragePartition::GetServiceWorkerContext.

This CL exposes a static method that takes in a ServiceWorkerContext*
and a OnceCallback and then it add ref-count to the underlying
ServiceWorkerContextWrapper until the OnceCallback is called.

Bug:  875376 
Change-Id: I2d5c3bfa605679623e7558f507e31cf425c2804d
Reviewed-on: https://chromium-review.googlesource.com/1176734
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585223}
[modify] https://crrev.com/415f86f67e9e7480512c79a86d1ac136b6048832/content/browser/service_worker/service_worker_context_wrapper.cc
[modify] https://crrev.com/415f86f67e9e7480512c79a86d1ac136b6048832/content/public/browser/service_worker_context.h
[modify] https://crrev.com/415f86f67e9e7480512c79a86d1ac136b6048832/extensions/browser/service_worker_task_queue.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 7 

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

commit 2c6f64dd7e4e27783d48f09129f20fcd12374994
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Fri Sep 07 22:18:09 2018

[Extensions] Fix potential UAF access of ServiceWorkerContext.

Use ServiceWorkerContext::RunTask() to make sure that
ServiceWorkerContext* is kept alive while transitioning from
UI thread to IO thread.

Bug:  875376 
Change-Id: Ib961931260b453e8a99828b8598d77c7821cff06
Reviewed-on: https://chromium-review.googlesource.com/1211971
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589686}
[modify] https://crrev.com/2c6f64dd7e4e27783d48f09129f20fcd12374994/extensions/browser/events/event_ack_data.cc

Comment 3 by lazyboy@chromium.org, Sep 8

Status: Fixed (was: Started)
►

Sign in to add a comment