From 94c589f69692140313994990e25cf6140ed6936f Mon Sep 17 00:00:00 2001
From: Benjamin Sergeant <bsergean@gmail.com>
Date: Tue, 24 Sep 2019 11:46:54 -0700
Subject: [PATCH] Fix 2 race conditions detected with TSan, one in
 CobraMetricsPublisher::push and another one in WebSocketTransport::sendData
 (that one was bad).

---
 DOCKER_VERSION                        |  2 +-
 docs/CHANGELOG.md                     |  4 ++++
 ixcobra/ixcobra/IXCobraConnection.cpp |  2 ++
 ixcobra/ixcobra/IXCobraConnection.h   |  1 +
 ixwebsocket/IXWebSocketTransport.cpp  |  5 ++++-
 ixwebsocket/IXWebSocketVersion.h      |  2 +-
 makefile                              |  3 +++
 test/IXCobraMetricsPublisherTest.cpp  | 29 +++++++++++++++++++++++++++
 8 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/DOCKER_VERSION b/DOCKER_VERSION
index a6534bb3..09d22fa5 100644
--- a/DOCKER_VERSION
+++ b/DOCKER_VERSION
@@ -1 +1 @@
-6.2.5
+6.2.6
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index e280726c..3c45b6e0 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -1,6 +1,10 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [6.2.5] - 2019-09-24
+
+- Fix 2 race conditions detected with TSan, one in CobraMetricsPublisher::push and another one in WebSocketTransport::sendData (that one was bad).
+
 ## [6.2.5] - 2019-09-23
 
 - Add simple Redis Server which is only capable of doing publish / subscribe. New ws redis_server sub-command to use it. The server is used in the unittest, so that we can run on CI in environment where redis isn not available like github actions env.
diff --git a/ixcobra/ixcobra/IXCobraConnection.cpp b/ixcobra/ixcobra/IXCobraConnection.cpp
index 285b3845..9382050a 100644
--- a/ixcobra/ixcobra/IXCobraConnection.cpp
+++ b/ixcobra/ixcobra/IXCobraConnection.cpp
@@ -462,6 +462,8 @@ namespace ix
         const Json::Value& msg,
         bool addToQueue)
     {
+        std::lock_guard<std::mutex> lock(_prePublishMutex);
+
         invokePublishTrackerCallback(true, false);
 
         CobraConnection::MsgId msgId = _id;
diff --git a/ixcobra/ixcobra/IXCobraConnection.h b/ixcobra/ixcobra/IXCobraConnection.h
index 656a45e8..baa01038 100644
--- a/ixcobra/ixcobra/IXCobraConnection.h
+++ b/ixcobra/ixcobra/IXCobraConnection.h
@@ -181,6 +181,7 @@ namespace ix
         Json::Value _pdu;
         Json::FastWriter _jsonWriter;
         mutable std::mutex _jsonWriterMutex;
+        std::mutex _prePublishMutex;
 
         /// Traffic tracker callback
         static TrafficTrackerCallback _trafficTrackerCallback;
diff --git a/ixwebsocket/IXWebSocketTransport.cpp b/ixwebsocket/IXWebSocketTransport.cpp
index 3ff7948c..6d901dad 100644
--- a/ixwebsocket/IXWebSocketTransport.cpp
+++ b/ixwebsocket/IXWebSocketTransport.cpp
@@ -867,7 +867,10 @@ namespace ix
             message_end = compressedMessage.end();
         }
 
-        _txbuf.reserve(wireSize);
+        {
+            std::lock_guard<std::mutex> lock(_socketMutex);
+            _txbuf.reserve(wireSize);
+        }
 
         // Common case for most message. No fragmentation required.
         if (wireSize < kChunkSize)
diff --git a/ixwebsocket/IXWebSocketVersion.h b/ixwebsocket/IXWebSocketVersion.h
index fe318a15..a7613f74 100644
--- a/ixwebsocket/IXWebSocketVersion.h
+++ b/ixwebsocket/IXWebSocketVersion.h
@@ -6,4 +6,4 @@
 
 #pragma once
 
-#define IX_WEBSOCKET_VERSION "6.2.5"
+#define IX_WEBSOCKET_VERSION "6.2.6"
diff --git a/makefile b/makefile
index fbdeb3f9..34b77cd6 100644
--- a/makefile
+++ b/makefile
@@ -20,6 +20,9 @@ uninstall:
 tag:
 	git tag v"`cat DOCKER_VERSION`"
 
+xcode:
+	cmake -DCMAKE_BUILD_TYPE=Debug -DUSE_TLS=1 -DUSE_WS=1 -DUSE_TEST=1 -GXcode && open ixwebsocket.xcodeproj
+
 .PHONY: docker
 
 NAME   := bsergean/ws
diff --git a/test/IXCobraMetricsPublisherTest.cpp b/test/IXCobraMetricsPublisherTest.cpp
index d4b64d2d..daf2f6ab 100644
--- a/test/IXCobraMetricsPublisherTest.cpp
+++ b/test/IXCobraMetricsPublisherTest.cpp
@@ -139,6 +139,21 @@ namespace
 
         gUniqueMessageIdsCount = gIds.size();
     }
+
+    // publish 100 messages, during roughly 100ms
+    // this is used to test thread safety of CobraMetricsPublisher::push
+    void runAdditionalPublisher(ix::CobraMetricsPublisher* cobraMetricsPublisher)
+    {
+        Json::Value data;
+        data["foo"] = "bar";
+
+        for (int i = 0; i < 100; ++i)
+        {
+            cobraMetricsPublisher->push("sms_metric_F_id", data);
+            ix::msleep(1);
+        }
+    }
+
 } // namespace
 
 TEST_CASE("Cobra_Metrics_Publisher", "[cobra]")
@@ -252,6 +267,19 @@ TEST_CASE("Cobra_Metrics_Publisher", "[cobra]")
 
     ix::msleep(500);
 
+    // Test multi-threaded publish
+    std::thread bgPublisher1(&runAdditionalPublisher, &cobraMetricsPublisher);
+    std::thread bgPublisher2(&runAdditionalPublisher, &cobraMetricsPublisher);
+    std::thread bgPublisher3(&runAdditionalPublisher, &cobraMetricsPublisher);
+    std::thread bgPublisher4(&runAdditionalPublisher, &cobraMetricsPublisher);
+    std::thread bgPublisher5(&runAdditionalPublisher, &cobraMetricsPublisher);
+
+    bgPublisher1.join();
+    bgPublisher2.join();
+    bgPublisher3.join();
+    bgPublisher4.join();
+    bgPublisher5.join();
+
     // Now stop the thread
     gStop = true;
     bgThread.join();
@@ -264,6 +292,7 @@ TEST_CASE("Cobra_Metrics_Publisher", "[cobra]")
     CHECK(gIds.count("sms_metric_C_id") == 1);
     CHECK(gIds.count("sms_metric_D_id") == 1);
     CHECK(gIds.count("sms_metric_E_id") == 1);
+    CHECK(gIds.count("sms_metric_F_id") == 1);
     CHECK(gIds.count("sms_set_rate_control_id") == 1);
     CHECK(gIds.count("sms_set_blacklist_id") == 1);