Issue 5118 - Lock the platform interfaces before use

src/Platform.cpp

 /*
 * This file is part of Adblock Plus <https://adblockplus.org/>,
 * Copyright (C) 2006-present eyeo GmbH
 *
 * Adblock Plus is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
 * published by the Free Software Foundation.
 *
 * Adblock Plus is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 34 matching lines @@
 Platform::Platform(CreationParameters&& creationParameters)
 {
   ASSIGN_PLATFORM_PARAM(logSystem);
   ASSIGN_PLATFORM_PARAM(timer);
   ASSIGN_PLATFORM_PARAM(fileSystem);
   ASSIGN_PLATFORM_PARAM(webRequest);
 }
 
 Platform::~Platform()
 {
-  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
-  logSystem.reset();
-  timer.reset();

[sergei, 2017/09/13 19:20:53]

I'm afraid here is a dead lock:
Let's say `interfacesMutex` has been acquired here and the current thread is on
the line 56, meanwhile the timer callback is being fired and gets locked on the
same mutex, now our thread reaches the 57th line and destroys the timer which
will wait for the finishing of its thread what will never happen because the
timer's thread is waiting for the mutex.
I think it should be rather
{
  LogSystemPtr tmpLogSystem;
  ...
  {
    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
    tmpLogSystem = move(logSystem);
    ...
  }
  // here we can safely destroy the interfaces because the mutex is unlocked and
they can wait for their threads but any new scheduled tasks (functions passed
into WithSomeInterface) will be ignored.
}

[hub, 2017/09/13 19:50:49]

Good catch.

-  fileSystem.reset();
-  webRequest.reset();
 }
 
 void Platform::SetUpJsEngine(const AppInfo& appInfo, std::unique_ptr<IV8IsolateProvider> isolate)
 {
   std::lock_guard<std::mutex> lock(modulesMutex);
   if (jsEngine)
     return;
   jsEngine = JsEngine::New(appInfo, *this, std::move(isolate));
 }
 
@@ skipping 25 matching lines @@
 }
 
 FilterEngine& Platform::GetFilterEngine()
 {
   CreateFilterEngineAsync();
   return *std::shared_future<FilterEnginePtr>(filterEngine).get();
 }
 
 void Platform::WithTimer(const WithTimerCallback& callback)
 {
-  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
   if (timer && callback)
     callback(*timer);
 }
 
 void Platform::WithFileSystem(const WithFileSystemCallback& callback)
 {
-  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
   if (fileSystem && callback)
     callback(*fileSystem);
 }
 
 void Platform::WithWebRequest(const WithWebRequestCallback& callback)
 {
-  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
   if (webRequest && callback)
     callback(*webRequest);
 }
 
 void Platform::WithLogSystem(const WithLogSystemCallback& callback)
 {
-  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
   if (logSystem && callback)
     callback(*logSystem);
 }
 
 namespace
 {
   class DefaultPlatform : public Platform
   {
   public:
     typedef std::shared_ptr<Scheduler> AsyncExecutorPtr;
     explicit DefaultPlatform(const AsyncExecutorPtr& asyncExecutor, CreationParameters&& creationParams)
       : Platform(std::move(creationParams)), asyncExecutor(asyncExecutor)
     {
     }
-    ~DefaultPlatform()
-    {
-      asyncExecutor.reset();
-    }
+    ~DefaultPlatform();
+
+    void WithTimer(const WithTimerCallback&) override;
+    void WithFileSystem(const WithFileSystemCallback&) override;
+    void WithWebRequest(const WithWebRequestCallback&) override;
+    void WithLogSystem(const WithLogSystemCallback&) override;
+
   private:
     AsyncExecutorPtr asyncExecutor;
+    std::recursive_mutex interfacesMutex;
   };
+
+  DefaultPlatform::~DefaultPlatform()
+  {
+    asyncExecutor.reset();
+    LogSystemPtr tmpLogSystem;
+    TimerPtr tmpTimer;
+    FileSystemPtr tmpFileSystem;
+    WebRequestPtr tmpWebRequest;
+    {
+      std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+      tmpLogSystem = std::move(logSystem);
+      tmpTimer = std::move(timer);
+      tmpFileSystem = std::move(fileSystem);
+      tmpWebRequest = std::move(webRequest);
+    }
+  }
+
+  void DefaultPlatform::WithTimer(const WithTimerCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithTimer(callback);
+  }
+
+  void DefaultPlatform::WithFileSystem(const WithFileSystemCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithFileSystem(callback);
+  }
+
+  void DefaultPlatform::WithWebRequest(const WithWebRequestCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithWebRequest(callback);
+  }
+
+  void DefaultPlatform::WithLogSystem(const WithLogSystemCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithLogSystem(callback);
+  }
 }
 
 Scheduler DefaultPlatformBuilder::GetDefaultAsyncExecutor()
 {
   if (!defaultScheduler)
   {
     asyncExecutor = std::make_shared<Scheduler>(::DummyScheduler);
     std::weak_ptr<Scheduler> weakExecutor = asyncExecutor;
     defaultScheduler = [weakExecutor](const SchedulerTask& task)
     {
@@ skipping 36 matching lines @@
     CreateDefaultTimer();
   if (!fileSystem)
     CreateDefaultFileSystem();
   if (!webRequest)
     CreateDefaultWebRequest();
 
   std::unique_ptr<Platform> platform(new DefaultPlatform(asyncExecutor, std::move(*this)));
   asyncExecutor.reset();
   return platform;
 }