diff --git a/CHANGES b/CHANGES index 6c1f81ecf0..1c7f0e1a3a 100644 --- a/CHANGES +++ b/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) 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/VERSION b/VERSION index 5c76bcd962..dc4b6a035e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.2.0-dev.99 +4.2.0-dev.103 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; 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. 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