From 7d3df55bf5fd93b3eaca36cc43cb22ea7879bf7e Mon Sep 17 00:00:00 2001
From: Tobias Markmann <tm@ayena.de>
Date: Sun, 8 Feb 2015 00:48:10 +0100
Subject: Fix data race in DummyEventLoop and BoostConnection(Server)Test
 reported by TSAN

The data race is on the events_ member in DummyEventLoop.

A BoostIOServerThread can post events to the DummyEventLoop and thereby
access its events_ data member while the test's main code processes
events of the loop.

To prevent access to the DummyEventLoop by the BoostIOServiceThread
after the DummyEventLoop is deleted, the BoostIOServiceThread is
deleted before the DummyEventLoop.

Process remaining events in BoostConnectionTest::tearDown like we do in
BoostConnectionServerTest::tearDown.

Test-Information:

Run multiple times on NetworkTest as TSAN enabled build on Linux.
Without the patch it reports a data race for the events_ deque member.

Change-Id: I3c85535338fc0ce0263dbfc3534aceb1dd09c137

diff --git a/Swiften/EventLoop/DummyEventLoop.cpp b/Swiften/EventLoop/DummyEventLoop.cpp
index 4ea425e..b8e631e 100644
--- a/Swiften/EventLoop/DummyEventLoop.cpp
+++ b/Swiften/EventLoop/DummyEventLoop.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 Isode Limited.
+ * Copyright (c) 2010-2015 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -14,6 +14,7 @@ DummyEventLoop::DummyEventLoop() {
 }
 
 DummyEventLoop::~DummyEventLoop() {
+	boost::lock_guard<boost::mutex> lock(eventsMutex_);
 	if (!events_.empty()) {
 		std::cerr << "DummyEventLoop: Unhandled events at destruction time" << std::endl;
 	}
diff --git a/Swiften/EventLoop/DummyEventLoop.h b/Swiften/EventLoop/DummyEventLoop.h
index 6ee1b77..297549e 100644
--- a/Swiften/EventLoop/DummyEventLoop.h
+++ b/Swiften/EventLoop/DummyEventLoop.h
@@ -8,6 +8,9 @@
 
 #include <deque>
 
+#include <boost/thread/mutex.hpp>
+#include <boost/thread/locks.hpp>
+
 #include <Swiften/Base/API.h>
 #include <Swiften/EventLoop/EventLoop.h>
 
@@ -18,21 +21,39 @@ namespace Swift {
 			virtual ~DummyEventLoop();
 
 			void processEvents() {
-				while (!events_.empty()) {
-					handleEvent(events_[0]);
-					events_.pop_front();
+				while (hasEvents()) {
+					/* 
+					  Creating a copy of the to-be-handled Event object because handling
+					  it can result in a DummyEventLoop::post() call.
+					  This call would also try to lock the eventsMutex_, resulting in a 
+					  deadlock. 
+					*/
+
+					eventsMutex_.lock();
+					Event eventCopy = events_[0];
+					eventsMutex_.unlock();
+
+					handleEvent(eventCopy);
+
+					{
+						boost::lock_guard<boost::mutex> lock(eventsMutex_);
+						events_.pop_front();
+					}
 				}
 			}
 
 			bool hasEvents() {
+				boost::lock_guard<boost::mutex> lock(eventsMutex_);
 				return !events_.empty();
 			}
 
 			virtual void post(const Event& event) {
+				boost::lock_guard<boost::mutex> lock(eventsMutex_);
 				events_.push_back(event);
 			}
 
 		private:
+			boost::mutex eventsMutex_;
 			std::deque<Event> events_;
 	};
 }
diff --git a/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp b/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp
index 1b21042..7488d50 100644
--- a/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp
+++ b/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 Isode Limited.
+ * Copyright (c) 2010-2015 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -24,18 +24,18 @@ class BoostConnectionServerTest : public CppUnit::TestFixture {
 
 	public:
 		void setUp() {
-			boostIOServiceThread_ = new BoostIOServiceThread();
 			eventLoop_ = new DummyEventLoop();
+			boostIOServiceThread_ = new BoostIOServiceThread();
 			stopped = false;
 			stoppedError.reset();
 		}
 
 		void tearDown() {
+			delete boostIOServiceThread_;
 			while (eventLoop_->hasEvents()) {
 				eventLoop_->processEvents();
 			}
 			delete eventLoop_;
-			delete boostIOServiceThread_;
 		}
 
 		void testConstructor_TwoServersOnSamePort() {
diff --git a/Swiften/QA/NetworkTest/BoostConnectionTest.cpp b/Swiften/QA/NetworkTest/BoostConnectionTest.cpp
index ff9ab0a..0cb38c3 100644
--- a/Swiften/QA/NetworkTest/BoostConnectionTest.cpp
+++ b/Swiften/QA/NetworkTest/BoostConnectionTest.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2013 Isode Limited.
+ * Copyright (c) 2010-2015 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -35,16 +35,19 @@ class BoostConnectionTest : public CppUnit::TestFixture {
 
 	public:
 		void setUp() {
+			eventLoop_ = new DummyEventLoop();
 			boostIOServiceThread_ = new BoostIOServiceThread();
 			boostIOService = boost::make_shared<boost::asio::io_service>();
-			eventLoop_ = new DummyEventLoop();
 			disconnected = false;
 			connectFinished = false;
 		}
 
 		void tearDown() {
-			delete eventLoop_;
 			delete boostIOServiceThread_;
+			while (eventLoop_->hasEvents()) {
+				eventLoop_->processEvents();
+			}
+			delete eventLoop_;
 		}
 
 		void testDestructor() {
-- 
cgit v0.10.2-6-g49f6