Fix use-after-free bug if beginBackgroundTaskWithExpirationHandler re…

…turns UIBackgroundTaskInvalid

In this case UIApplication doesn't retain block, and at the end
of constructor ref_count of |this| will be decremented to zero ->
|this| will be deleted. Later deleted object will be accessed in
|core_(new ScopedCriticalAction::Core())|

The solution is to separate object construction and
beginBackgroundTaskWithExpirationHandler method call.

Change-Id: I0aa21cba2396231ea9ccd3ee34617f5d0417dbdd
Reviewed-on: https://chromium-review.googlesource.com/558245
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485582}

base/ios/scoped_critical_action.h

@@ -39,8 +39,12 @@ class ScopedCriticalAction {
    public:
     Core();
 
-    // Informs the OS that the background task has completed.
-    void EndBackgroundTask();
+    // Informs the OS that the background task has started. This is a
+    // static method to ensure that the instance has a non-zero refcount.
+    static void StartBackgroundTask(scoped_refptr<Core> core);
+    // Informs the OS that the background task has completed. This is a
+    // static method to ensure that the instance has a non-zero refcount.
+    static void EndBackgroundTask(scoped_refptr<Core> core);
 
    private:
     friend base::RefCountedThreadSafe<Core>;

base/ios/scoped_critical_action.mm

@@ -14,46 +14,59 @@
 namespace ios {
 
 ScopedCriticalAction::ScopedCriticalAction()
-    : core_(new ScopedCriticalAction::Core()) {
+    : core_(base::MakeRefCounted<ScopedCriticalAction::Core>()) {
+  ScopedCriticalAction::Core::StartBackgroundTask(core_);
 }
 
 ScopedCriticalAction::~ScopedCriticalAction() {
-  core_->EndBackgroundTask();
+  ScopedCriticalAction::Core::EndBackgroundTask(core_);
+}
+
+ScopedCriticalAction::Core::Core()
+    : background_task_id_(UIBackgroundTaskInvalid) {}
+
+ScopedCriticalAction::Core::~Core() {
+  DCHECK_EQ(background_task_id_, UIBackgroundTaskInvalid);
 }
 
 // This implementation calls |beginBackgroundTaskWithExpirationHandler:| when
 // instantiated and |endBackgroundTask:| when destroyed, creating a scope whose
 // execution will continue (temporarily) even after the app is backgrounded.
-ScopedCriticalAction::Core::Core() {
-  scoped_refptr<ScopedCriticalAction::Core> core = this;
-  background_task_id_ = [[UIApplication sharedApplication]
-      beginBackgroundTaskWithExpirationHandler:^{
-        DLOG(WARNING) << "Background task with id " << background_task_id_
+// static
+void ScopedCriticalAction::Core::StartBackgroundTask(scoped_refptr<Core> core) {
+  UIApplication* application = [UIApplication sharedApplication];
+  if (!application) {
+    return;
+  }
+
+  core->background_task_id_ =
+      [application beginBackgroundTaskWithExpirationHandler:^{
+        DLOG(WARNING) << "Background task with id " << core->background_task_id_
                       << " expired.";
         // Note if |endBackgroundTask:| is not called for each task before time
         // expires, the system kills the application.
-        core->EndBackgroundTask();
+        EndBackgroundTask(core);
       }];
-  if (background_task_id_ == UIBackgroundTaskInvalid) {
-    DLOG(WARNING) <<
-        "beginBackgroundTaskWithExpirationHandler: returned an invalid ID";
+
+  if (core->background_task_id_ == UIBackgroundTaskInvalid) {
+    DLOG(WARNING)
+        << "beginBackgroundTaskWithExpirationHandler: returned an invalid ID";
   } else {
-    VLOG(3) << "Beginning background task with id " << background_task_id_;
+    VLOG(3) << "Beginning background task with id "
+            << core->background_task_id_;
   }
 }
 
-ScopedCriticalAction::Core::~Core() {
-  DCHECK_EQ(background_task_id_, UIBackgroundTaskInvalid);
-}
-
-void ScopedCriticalAction::Core::EndBackgroundTask() {
+// static
+void ScopedCriticalAction::Core::EndBackgroundTask(scoped_refptr<Core> core) {
   UIBackgroundTaskIdentifier task_id;
   {
-    AutoLock lock_scope(background_task_id_lock_);
-    if (background_task_id_ == UIBackgroundTaskInvalid)
+    AutoLock lock_scope(core->background_task_id_lock_);
+    if (core->background_task_id_ == UIBackgroundTaskInvalid) {
       return;
-    task_id = background_task_id_;
-    background_task_id_ = UIBackgroundTaskInvalid;
+    }
+    task_id = core->background_task_id_;
+    core->background_task_id_ = UIBackgroundTaskInvalid;
   }
 
   VLOG(3) << "Ending background task with id " << task_id;