4307/8/9/10 data races (#4312)

* fix(NumericString): properly mark uIntToString deprecated #4304

* dev(runLibtests): allow to specify test to run

* fix(NotificationCenter): data race #4307

* fix(DirectoryWatcher): data race #4308

* fix(ArchiveStrategy): data race #4309

* fix(ActiveThread): data race #4310

* fix(Task): Cancelled Task shouldn't start running #4311 (WIP)

* fix(String): ignore clang loop unrolling warnings

* fix(TaskManager): task ownership #4311

* chore(FIFOEventTest): fix unused var warning; disable benchmark in test

* fix(Task): remove unnecessary mutex (and prevent cyclic locking reported by TSAN)

* fix(CryptoTest): disable testEncryptDecryptGCM

* fix(ci): typo

* fix(NotificationCenter): disable and clear observers in dtor (#4307)

---------

Co-authored-by: Matej Kenda <matejken@gmail.com>

.github/workflows/ci.yml

@@ -2,6 +2,9 @@
 # To retry only on timeout set retry_on: timeout
 # To retry only on error set retry_on: error
 # For more information on the retry action see https://github.com/nick-fields/retry
+
+name: Compile and Testrun
+
 on:
   pull_request:
     types: [opened]
@@ -231,7 +234,8 @@ jobs:
             CppUnit::TestCaller<ExpireLRUCacheTest>.testAccessExpireN,
             CppUnit::TestCaller<UniqueExpireLRUCacheTest>.testExpireN,
             CppUnit::TestCaller<ExpireLRUCacheTest>.testAccessExpireN,
-            CppUnit::TestCaller<PollSetTest>.testPollClosedServer"
+            CppUnit::TestCaller<PollSetTest>.testPollClosedServer,
+            CppUnit::TestCaller<CryptoTest>.testEncryptDecryptGCM"
             PWD=`pwd`
             ctest --output-on-failure -E "(DataMySQL)|(DataODBC)|(PostgreSQL)|(MongoDB)|(Redis)"
 

Crypto/testsuite/Makefile

@@ -16,10 +16,14 @@ else
 ifeq ($(findstring AIX, $(POCO_CONFIG)), AIX)
 SYSLIBS += -lssl_a -lcrypto_a -lz -ldl
 else
+ifeq ($(POCO_CONFIG),Darwin)
+SYSLIBS += -lssl -lcrypto -lz
+else
 SYSLIBS += -lssl -lcrypto -lz -ldl
-endif
-endif
-endif
+endif # Darwin
+endif # AIX
+endif # QNX
+endif # FreeBSD
 
 objects = CryptoTestSuite Driver \
 	CryptoTest DigestEngineTest ECTest \

Foundation/include/Poco/ArchiveStrategy.h

@@ -23,6 +23,7 @@
 #include "Poco/File.h"
 #include "Poco/DateTimeFormatter.h"
 #include "Poco/NumberFormatter.h"
+#include <atomic>
 
 
 namespace Poco {
@@ -44,7 +45,7 @@ class Foundation_API ArchiveStrategy
 
 	virtual LogFile* open(LogFile* pFile) = 0;
 		/// Open a new log file and return it.
-	
+
 	virtual LogFile* archive(LogFile* pFile) = 0;
 		/// Renames the given log file for archiving
 		/// and creates and returns a new log file.
@@ -61,8 +62,8 @@ class Foundation_API ArchiveStrategy
 	ArchiveStrategy(const ArchiveStrategy&);
 	ArchiveStrategy& operator = (const ArchiveStrategy&);
 
-	bool _compress;
-	ArchiveCompressor* _pCompressor;
+	std::atomic<bool> _compress;
+	std::atomic<ArchiveCompressor*> _pCompressor;
 };
 
 

Foundation/include/Poco/NumericString.h

@@ -387,7 +387,8 @@ bool intToStr(T value,
 	char fill = ' ',
 	char thSep = 0,
 	bool lowercase = false)
-	/// Converts integer to string. Standard numeric bases from binary to hexadecimal are supported.
+	/// Converts signed integer to string. Standard numeric bases from binary to hexadecimal
+	/// are supported.
 	/// If width is non-zero, it pads the return value with fill character to the specified width.
 	/// When padding is zero character ('0'), it is prepended to the number itself; all other
 	/// paddings are prepended to the formatted result with minus sign or base prefix included
@@ -534,8 +535,8 @@ bool intToStr(T value,
 }
 
 
-//@ deprecated
 template <typename T>
+[[deprecated("use intToStr instead")]]
 bool uIntToStr(T value,
 	unsigned short base,
 	char* result,
@@ -579,8 +580,8 @@ bool intToStr (T number,
 }
 
 
-//@ deprecated
 template <typename T>
+[[deprecated("use intToStr instead")]]
 bool uIntToStr (T number,
 	unsigned short base,
 	std::string& result,

Foundation/include/Poco/String.h

@@ -23,6 +23,11 @@
 #include <cstring>
 #include <algorithm>
 
+// ignore loop unrolling warnings in this file
+#if defined(__clang__) && ((__clang_major__ > 3) || (__clang_major__ == 3 && __clang_minor__ >= 6))
+#	pragma clang diagnostic push
+#	pragma clang diagnostic ignored "-Wpass-failed"
+#endif
 
 namespace Poco {
 
@@ -760,5 +765,8 @@ struct CILess
 
 } // namespace Poco
 
+#if defined(__clang__) && ((__clang_major__ > 3) || (__clang_major__ == 3 && __clang_minor__ >= 6))
+#	pragma clang diagnostic pop
+#endif
 
 #endif // Foundation_String_INCLUDED

Foundation/include/Poco/Task.h

@@ -77,6 +77,8 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 		/// A Task's runTask() method should periodically
 		/// call this method and stop whatever it is doing in an
 		/// orderly way when this method returns true.
+		/// If task is cancelled before it had a chance to run,
+		/// runTask() will never be called.
 
 	TaskState state() const;
 		/// Returns the task's current state.
@@ -90,9 +92,14 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 		/// be overridden by subclasses.
 
 	void run();
-		/// Calls the task's runTask() method and notifies the owner
-		/// of the task's start and completion.
-
+		/// If task has not been cancelled prior to this call, it
+		/// calls the task's runTask() method and notifies the owner of
+		/// the task's start and completion.
+		/// If task has been cancelled prior to this call, it only sets
+		/// the state to TASK_FINISHED and notifies the owner.
+
+	bool hasOwner() const;
+		/// Returns true iff the task has an owner.
 protected:
 	bool sleep(long milliseconds);
 		/// Suspends the current thread for the specified
@@ -134,7 +141,7 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 	TaskManager* getOwner() const;
 		/// Returns the owner of the task, which may be NULL.
 
-	void setState(TaskState state);
+	TaskState setState(TaskState state);
 		/// Sets the task's state.
 
 	virtual ~Task();
@@ -145,12 +152,12 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 	Task(const Task&);
 	Task& operator = (const Task&);
 
-	std::string            _name;
-	TaskManager*           _pOwner;
-	std::atomic<float>     _progress;
-	std::atomic<TaskState> _state;
-	Event                  _cancelEvent;
-	mutable FastMutex      _mutex;
+	std::string               _name;
+	std::atomic<TaskManager*> _pOwner;
+	std::atomic<float>        _progress;
+	std::atomic<TaskState>    _state;
+	Event                     _cancelEvent;
+	mutable FastMutex         _mutex;
 
 	friend class TaskManager;
 };
@@ -185,12 +192,16 @@ inline Task::TaskState Task::state() const
 
 inline TaskManager* Task::getOwner() const
 {
-	FastMutex::ScopedLock lock(_mutex);
-
 	return _pOwner;
 }
 
 
+inline bool Task::hasOwner() const
+{
+	return _pOwner != nullptr;
+}
+
+
 } // namespace Poco
 
 

Foundation/include/Poco/TaskManager.h

@@ -65,9 +65,16 @@ class Foundation_API TaskManager
 	~TaskManager();
 		/// Destroys the TaskManager.
 
-	void start(Task* pTask);
+	bool start(Task* pTask);
 		/// Starts the given task in a thread obtained
-		/// from the thread pool.
+		/// from the thread pool; returns true if successful.
+		///
+		/// If this method returns false, the task was cancelled
+		/// before it could be started, or it was already running;
+		/// in any case, a false return means refusal of ownership
+		/// and indicates that the task pointer may not be valid
+		/// anymore (it will only be valid if it was duplicated
+		/// prior to this call).
 		///
 		/// The TaskManager takes ownership of the Task object
 		/// and deletes it when it is finished.

Foundation/src/ActiveThreadPool.cpp

@@ -28,11 +28,14 @@ namespace Poco {
 class NewActionNotification: public Notification
 {
 public:
-	NewActionNotification(Thread::Priority priority, Runnable &runnable, std::string name) :
-	_priority(priority),
-	_runnable(runnable),
-	_name(std::move(name))
-	{ }
+	using Ptr = AutoPtr<NewActionNotification>;
+
+	NewActionNotification(Thread::Priority priority, Runnable& runnable, const std::string& name) :
+		_priority(priority),
+		_runnable(runnable),
+		_name(name)
+	{
+	}
 
 	~NewActionNotification() override = default;
 
@@ -41,16 +44,16 @@ class NewActionNotification: public Notification
 		return _runnable;
 	}
 
-	Thread::Priority priotity() const
+	Thread::Priority priority() const
 	{
 		return _priority;
 	}
-	
+
 	const std::string &threadName() const
 	{
 		return _name;
 	}
-	
+
 	std::string threadFullName() const
 	{
 		std::string fullName(_name);
@@ -68,8 +71,8 @@ class NewActionNotification: public Notification
 	}
 
 private:
-	Thread::Priority _priority;
-	Runnable &_runnable;
+	std::atomic<Thread::Priority> _priority;
+	Runnable& _runnable;
 	std::string _name;
 };
 
@@ -87,13 +90,13 @@ class ActiveThread: public Runnable
 	void run() override;
 
 private:
-	NotificationQueue    _pTargetQueue;
-	std::string          _name;
-	Thread               _thread;
-	Event                _targetCompleted;
-	FastMutex            _mutex;
-	const long           JOIN_TIMEOUT = 10000;
-	std::atomic<bool>    _needToStop{false};
+	NotificationQueue _pTargetQueue;
+	std::string       _name;
+	Thread            _thread;
+	Event             _targetCompleted;
+	FastMutex         _mutex;
+	const long        JOIN_TIMEOUT = 10000;
+	std::atomic<bool> _needToStop{false};
 };
 
 
@@ -157,16 +160,18 @@ void ActiveThread::release()
 
 void ActiveThread::run()
 {
-	do {
-		auto *_pTarget = dynamic_cast<NewActionNotification*>(_pTargetQueue.waitDequeueNotification());
-		while (_pTarget)
+	do
+	{
+		AutoPtr<Notification> pN = _pTargetQueue.waitDequeueNotification();
+		while (pN)
 		{
-			Runnable* pTarget = &_pTarget->runnable();
-			_thread.setPriority(_pTarget->priotity());
-			_thread.setName(_pTarget->name());
+			NewActionNotification::Ptr pNAN = pN.cast<NewActionNotification>();
+			Runnable& target = pNAN->runnable();
+			_thread.setPriority(pNAN->priority());
+			_thread.setName(pNAN->name());
 			try
 			{
-				pTarget->run();
+				target.run();
 			}
 			catch (Exception& exc)
 			{
@@ -180,11 +185,10 @@ void ActiveThread::run()
 			{
 				ErrorHandler::handle();
 			}
-			_pTarget->release();
 			_thread.setName(_name);
 			_thread.setPriority(Thread::PRIO_NORMAL);
 			ThreadLocalStorage::clear();
-			_pTarget = dynamic_cast<NewActionNotification*>(_pTargetQueue.waitDequeueNotification(1000));
+			pN = _pTargetQueue.waitDequeueNotification(1000);
 		}
 		_targetCompleted.set();
 	}

Foundation/src/ArchiveStrategy.cpp

@@ -123,7 +123,7 @@ void ArchiveStrategy::moveFile(const std::string& oldPath, const std::string& ne
 	{
 		f.renameTo(newPath);
 		if (!_pCompressor) _pCompressor = new ArchiveCompressor;
-		_pCompressor->compress(newPath);
+		_pCompressor.load()->compress(newPath);
 	}
 }
 

Foundation/src/DirectoryWatcher.cpp

@@ -40,6 +40,7 @@
 #endif
 #include <algorithm>
 #include <map>
+#include <atomic>
 
 
 namespace Poco {
@@ -244,7 +245,7 @@ class WindowsDirectoryWatcherStrategy: public DirectoryWatcherStrategy
 	}
 
 private:
-	HANDLE _hStopped;
+	std::atomic<HANDLE> _hStopped;
 };
 
 
@@ -455,7 +456,7 @@ class BSDDirectoryWatcherStrategy: public DirectoryWatcherStrategy
 private:
 	int _queueFD;
 	int _dirFD;
-	bool _stopped;
+	std::atomic<bool> _stopped;
 };
 
 

Foundation/src/NotificationCenter.cpp

@@ -29,6 +29,18 @@ NotificationCenter::NotificationCenter()
 
 NotificationCenter::~NotificationCenter()
 {
+	try
+	{
+		Mutex::ScopedLock lock(_mutex);
+		for (auto& o: _observers)
+			o->disable();
+
+		_observers.clear();
+	}
+	catch(...)
+	{
+		poco_unexpected();
+	}
 }