Merge remote-tracking branch 'origin/topic/timw/1500-disable-tsan-sqlite'

* origin/topic/timw/1500-disable-tsan-sqlite:
  Disable the scripts.base.frameworks.logging.sqlite.simultaneous-writes test under TSan
  Mark MsgThread::cnt_sent_{in,out} as atomic to avoid a data race
  Disable call to sqlite3_enable_shared_cache under ThreadSanitizer

CHANGES

@@ -1,3 +1,19 @@
+4.2.0-dev.103 | 2021-09-03 18:08:57 +0000
+
+  * Disable the scripts.base.frameworks.logging.sqlite.simultaneous-writes test under TSan (Tim Wojtulewicz, Corelight)
+
+    Due to a bug (or intentional code) in SQLite, we disabled enabling the shared cache
+    in sqlite3 if running under ThreadSanitizer (see cf1fefbe0b0a6163b389cc92b5a6878c7fc95f1f).
+    Unfortunately, this has the side-effect of breaking the simultaneous-writes test because
+    the shared cache is disabled. This is hopefully a temporary fix until SQLite fixes the
+    issue on their side.
+
+  * Mark MsgThread::cnt_sent_{in,out} as atomic to avoid a data race (Tim Wojtulewicz, Corelight)
+
+  * Disable call to sqlite3_enable_shared_cache under ThreadSanitizer (Tim Wojtulewicz, Corelight)
+
+    See https://sqlite.org/forum/forumpost/54424d80ee for details.
+
 4.2.0-dev.99 | 2021-09-03 17:36:09 +0000
 
   * GH-1589: Avoid extracting IP-like strings from SMTP headers (Tim Wojtulewicz, Corelight)

CMakeLists.txt

@@ -147,6 +147,10 @@ if ( ZEEK_SANITIZERS )
             set(ZEEK_SANITIZERS "${ZEEK_SANITIZERS},")
         endif ()
 
+        if ( _sanitizer STREQUAL "thread" )
+            set(ZEEK_TSAN true)
+        endif ()
+
         if ( NOT _sanitizer STREQUAL "undefined" )
             set(ZEEK_SANITIZERS "${ZEEK_SANITIZERS}${_sanitizer}")
             continue()

VERSION

@@ -1 +1 @@
-4.2.0-dev.99
+4.2.0-dev.103

src/input/readers/sqlite/SQLite.cc

@@ -82,7 +82,9 @@ bool SQLite::DoInit(const ReaderInfo& info, int arg_num_fields, const threading:
 
 	// Allow connections to same DB to use single data/schema cache. Also
 	// allows simultaneous writes to one file.
+#ifndef ZEEK_TSAN
 	sqlite3_enable_shared_cache(1);
+#endif
 
 	if ( Info().mode != MODE_MANUAL )
 		{

src/logging/writers/sqlite/SQLite.cc

@@ -122,7 +122,9 @@ bool SQLite::DoInit(const WriterInfo& info, int arg_num_fields,
 
 	// Allow connections to same DB to use single data/schema cache. Also
 	// allows simultaneous writes to one file.
+#ifndef ZEEK_TSAN
 	sqlite3_enable_shared_cache(1);
+#endif
 
 	num_fields = arg_num_fields;
 	fields = arg_fields;

src/threading/MsgThread.cc

@@ -202,7 +202,9 @@ Message::~Message()
 
 MsgThread::MsgThread() : BasicThread(), queue_in(this, nullptr), queue_out(nullptr, this)
 	{
-	cnt_sent_in = cnt_sent_out = 0;
+	cnt_sent_in.store(0);
+	cnt_sent_out.store(0);
+
 	main_finished = false;
 	child_finished = false;
 	child_sent_finish = false;
@@ -457,8 +459,8 @@ void MsgThread::Run()
 
 void MsgThread::GetStats(Stats* stats)
 	{
-	stats->sent_in = cnt_sent_in;
+	stats->sent_in = cnt_sent_in.load();
-	stats->sent_out = cnt_sent_out;
+	stats->sent_out = cnt_sent_out.load();
 	stats->pending_in = queue_in.Size();
 	stats->pending_out = queue_out.Size();
 	queue_in.GetStats(&stats->queue_in_stats);

src/threading/MsgThread.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <atomic>
+
 #include "zeek/DebugLogger.h"
 #include "zeek/threading/BasicThread.h"
 #include "zeek/threading/Queue.h"
@@ -335,8 +337,8 @@ private:
 	Queue<BasicInputMessage *> queue_in;
 	Queue<BasicOutputMessage *> queue_out;
 
-	uint64_t cnt_sent_in;	// Counts message sent to child.
+	std::atomic<uint64_t> cnt_sent_in;	// Counts message sent to child.
-	uint64_t cnt_sent_out;	// Counts message sent by child.
+	std::atomic<uint64_t> cnt_sent_out;	// Counts message sent by child.
 
 	bool main_finished;	// Main thread is finished, meaning child_finished propagated back through message queue.
 	bool child_finished;	// Child thread is finished.

testing/btest/scripts/base/frameworks/logging/sqlite/simultaneous-writes.zeek

@@ -2,6 +2,10 @@
 #
 # @TEST-REQUIRES: which sqlite3
 # @TEST-REQUIRES: has-writer Zeek::SQLiteWriter
+
+# Don't run this test if we build with '--sanitizers=thread' because we
+# disable the shared cache in that case due to a SQLite bug.
+# @TEST-REQUIRES: grep -q "#define ZEEK_TSAN" zeek-config.h || test $? == 0
 # @TEST-GROUP: sqlite
 #
 # @TEST-EXEC: zeek -b %INPUT
@@ -87,4 +91,3 @@ event zeek_init()
 	Log::write(SSH::LOG, out);
 	Log::write(SSH::LOG2, out);
 }
-

zeek-config.h.in

@@ -276,13 +276,17 @@ extern const char* BRO_VERSION_FUNCTION();
 	#define ZEEK_LSAN_DISABLE_SCOPE(x)
 #endif
 
+// This part is dependent on calling configure with '--sanitizers=thread'
+// and not manually setting CFLAGS/CXXFLAGS to include --fsanitize=thread.
+// This is because some of the unit tests only work when built without
+// TSan, at least until SQLite opts to fix their problems with atomics.
 #if defined(__SANITIZE_THREAD__)
-	#define ZEEK_TSAN
+	#cmakedefine ZEEK_TSAN
 #endif
 
 #if defined(__has_feature)
 	#if __has_feature(thread_sanitizer)
-		#define ZEEK_TSAN
+		#cmakedefine ZEEK_TSAN
 	#endif
 #endif