From 9f02fa6b2ee1bfac85b20907693c0afdb1d274b5 Mon Sep 17 00:00:00 2001
From: Konstantin Varlamov <var-const@users.noreply.github.com>
Date: Wed, 11 Apr 2018 15:17:49 -0400
Subject: iOS: fix data race in FSTDispatchQueue. (#1070)

operationInProgress is accessed from both the main thread and from
libdispatch on some other thread. Make it atomic to avoid a data race.
Also reorder assertion checks to only access operationInProgress
after making sure the function is running on the queue.

Tested: ran unit tests using old and new versions under Thread
Sanitizer, verified that TSan reports a data race for the old version,
but finds no issues with the new version.
---
 Firestore/Source/Util/FSTDispatchQueue.mm | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

(limited to 'Firestore/Source/Util')

diff --git a/Firestore/Source/Util/FSTDispatchQueue.mm b/Firestore/Source/Util/FSTDispatchQueue.mm
index 15d6e7b..0974359 100644
--- a/Firestore/Source/Util/FSTDispatchQueue.mm
+++ b/Firestore/Source/Util/FSTDispatchQueue.mm
@@ -16,6 +16,8 @@
 
 #import <Foundation/Foundation.h>
 
+#include <atomic>
+
 #import "Firestore/Source/Util/FSTAssert.h"
 #import "Firestore/Source/Util/FSTDispatchQueue.h"
 
@@ -146,24 +148,23 @@ NS_ASSUME_NONNULL_BEGIN
 #pragma mark - FSTDispatchQueue
 
 @interface FSTDispatchQueue ()
-
 /**
  * Callbacks scheduled to be queued in the future. Callbacks are automatically removed after they
  * are run or canceled.
  */
 @property(nonatomic, strong, readonly) NSMutableArray<FSTDelayedCallback *> *delayedCallbacks;
 
-/**
- * Flag set while an FSTDispatchQueue operation is currently executing. Used for assertion
- * sanity-checks.
- */
-@property(nonatomic, assign) BOOL operationInProgress;
-
 - (instancetype)initWithQueue:(dispatch_queue_t)queue NS_DESIGNATED_INITIALIZER;
 
 @end
 
-@implementation FSTDispatchQueue
+@implementation FSTDispatchQueue {
+  /**
+   * Flag set while an FSTDispatchQueue operation is currently executing. Used for assertion
+   * sanity-checks.
+   */
+  std::atomic<bool> _operationInProgress;
+}
 
 + (instancetype)queueWith:(dispatch_queue_t)dispatchQueue {
   return [[FSTDispatchQueue alloc] initWithQueue:dispatchQueue];
@@ -171,9 +172,9 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (instancetype)initWithQueue:(dispatch_queue_t)queue {
   if (self = [super init]) {
+    _operationInProgress = false;
     _queue = queue;
     _delayedCallbacks = [NSMutableArray array];
-    _operationInProgress = NO;
   }
   return self;
 }
@@ -191,16 +192,16 @@ NS_ASSUME_NONNULL_BEGIN
   FSTAssert(!_operationInProgress,
             @"enterCheckedOperation may not be called when an operation is in progress");
   @try {
-    _operationInProgress = YES;
+    _operationInProgress = true;
     [self verifyIsCurrentQueue];
     block();
   } @finally {
-    _operationInProgress = NO;
+    _operationInProgress = false;
   }
 }
 
 - (void)dispatchAsync:(void (^)(void))block {
-  FSTAssert(!_operationInProgress || ![self onTargetQueue],
+  FSTAssert(![self onTargetQueue] || !_operationInProgress,
             @"dispatchAsync called when we are already running on target dispatch queue '%@'",
             [self targetQueueLabel]);
 
@@ -216,7 +217,7 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (void)dispatchSync:(void (^)(void))block {
-  FSTAssert(!_operationInProgress || ![self onTargetQueue],
+  FSTAssert(![self onTargetQueue] || !_operationInProgress,
             @"dispatchSync called when we are already running on target dispatch queue '%@'",
             [self targetQueueLabel]);
 
-- 
cgit v1.2.3