From a2ab1b148473366afcfc4d962bfb57c7ba49eca3 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 6 Aug 2021 17:02:10 -0700 Subject: [PATCH 1/3] Disable call to sqlite3_enable_shared_cache under ThreadSanitizer See https://sqlite.org/forum/forumpost/54424d80ee for details. --- src/input/readers/sqlite/SQLite.cc | 2 ++ src/logging/writers/sqlite/SQLite.cc | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/input/readers/sqlite/SQLite.cc b/src/input/readers/sqlite/SQLite.cc index 0785ae226b..f37001d3c3 100644 --- a/src/input/readers/sqlite/SQLite.cc +++ b/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 ) { diff --git a/src/logging/writers/sqlite/SQLite.cc b/src/logging/writers/sqlite/SQLite.cc index faf80547c2..885eb20b78 100644 --- a/src/logging/writers/sqlite/SQLite.cc +++ b/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; From 4dc20826fd8cebab06a7af26b5ccb4fddb506507 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Mon, 9 Aug 2021 10:39:56 -0700 Subject: [PATCH 2/3] Mark MsgThread::cnt_sent_{in,out} as atomic to avoid a data race --- src/threading/MsgThread.cc | 8 +++++--- src/threading/MsgThread.h | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/threading/MsgThread.cc b/src/threading/MsgThread.cc index 0bc3fa3901..87b4cf5677 100644 --- a/src/threading/MsgThread.cc +++ b/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_out = cnt_sent_out; + stats->sent_in = cnt_sent_in.load(); + 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); diff --git a/src/threading/MsgThread.h b/src/threading/MsgThread.h index 155fbb6ae4..5ab5b60ddd 100644 --- a/src/threading/MsgThread.h +++ b/src/threading/MsgThread.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "zeek/DebugLogger.h" #include "zeek/threading/BasicThread.h" #include "zeek/threading/Queue.h" @@ -335,8 +337,8 @@ private: Queue queue_in; Queue queue_out; - uint64_t cnt_sent_in; // Counts message sent to child. - uint64_t cnt_sent_out; // Counts message sent by child. + std::atomic cnt_sent_in; // Counts message sent to child. + std::atomic 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. From 0369ca01bcb5b0cacc69da0bca1f2cd7261bdf65 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Mon, 9 Aug 2021 15:11:18 -0700 Subject: [PATCH 3/3] Disable the scripts.base.frameworks.logging.sqlite.simultaneous-writes test under TSan 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. --- CMakeLists.txt | 4 ++++ .../frameworks/logging/sqlite/simultaneous-writes.zeek | 5 ++++- zeek-config.h.in | 8 ++++++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 33418cf05b..908c15a057 100644 --- a/CMakeLists.txt +++ b/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() diff --git a/testing/btest/scripts/base/frameworks/logging/sqlite/simultaneous-writes.zeek b/testing/btest/scripts/base/frameworks/logging/sqlite/simultaneous-writes.zeek index fcdbd928ee..67de58471f 100644 --- a/testing/btest/scripts/base/frameworks/logging/sqlite/simultaneous-writes.zeek +++ b/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); } - diff --git a/zeek-config.h.in b/zeek-config.h.in index 90570a81d8..3c90bd5ceb 100644 --- a/zeek-config.h.in +++ b/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