From 30500023c31ac02eca87def905cebd26e9ec5ffe Mon Sep 17 00:00:00 2001 From: "aleksey.mochalov" Date: Mon, 20 Jan 2025 11:13:57 +0300 Subject: [PATCH 1/3] add a check to verify thread matching between the encryption thread and the thread where we release the attachment. If they match, use a dummy mutex instead of the actual dbb_thread_mutex to avoid a deadlock --- src/common/ThreadStart.cpp | 14 ++++++++++++-- src/common/ThreadStart.h | 1 + src/jrd/CryptoManager.h | 4 ++++ src/jrd/jrd.cpp | 11 +++++++++-- 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/common/ThreadStart.cpp b/src/common/ThreadStart.cpp index 195e3fbffc0..8a4c9c2e42e 100644 --- a/src/common/ThreadStart.cpp +++ b/src/common/ThreadStart.cpp @@ -219,9 +219,14 @@ ThreadId Thread::getId() #endif } +bool Thread::isCurrent(InternalId iid) +{ + return pthread_equal(iid, pthread_self()); +} + bool Thread::isCurrent() { - return pthread_equal(internalId, pthread_self()); + return isCurrent(internalId); } void Thread::sleep(unsigned milliseconds) @@ -363,9 +368,14 @@ ThreadId Thread::getId() return GetCurrentThreadId(); } +bool Thread::isCurrent(InternalId iid) +{ + return GetCurrentThreadId() == iid; +} + bool Thread::isCurrent() { - return GetCurrentThreadId() == internalId; + return isCurrent(internalId); } void Thread::sleep(unsigned milliseconds) diff --git a/src/common/ThreadStart.h b/src/common/ThreadStart.h index c962313fd91..1aa821c7d3c 100644 --- a/src/common/ThreadStart.h +++ b/src/common/ThreadStart.h @@ -87,6 +87,7 @@ class Thread static void sleep(unsigned milliseconds); static void yield(); + static bool isCurrent(InternalId iid); bool isCurrent(); Thread() diff --git a/src/jrd/CryptoManager.h b/src/jrd/CryptoManager.h index 8037e086d9b..0c153915fe1 100644 --- a/src/jrd/CryptoManager.h +++ b/src/jrd/CryptoManager.h @@ -301,6 +301,10 @@ class CryptoManager final : public Firebird::PermanentStorage, public BarSync::I UCHAR getCurrentState(thread_db* tdbb) const; const char* getKeyName() const; const char* getPluginName() const; + Thread::Handle getCryptThreadHandle() const + { + return cryptThreadId; + } private: enum IoResult {SUCCESS_ALL, FAILED_CRYPT, FAILED_IO}; diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index 2d0fea95639..ce1e562ab36 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -7755,14 +7755,21 @@ void release_attachment(thread_db* tdbb, Jrd::Attachment* attachment, XThreadEns Sync sync(&dbb->dbb_sync, "jrd.cpp: release_attachment"); + // dummy mutex is used to avoid races with crypto thread + XThreadMutex dummy_mutex; + XThreadEnsureUnlock dummyGuard(dummy_mutex, FB_FUNCTION); + // avoid races with special threads // take into an account lock earlier taken in DROP DATABASE XThreadEnsureUnlock threadGuard(dbb->dbb_thread_mutex, FB_FUNCTION); XThreadEnsureUnlock* activeThreadGuard = dropGuard; if (!activeThreadGuard) { - threadGuard.enter(); - activeThreadGuard = &threadGuard; + if (dbb->dbb_crypto_manager && Thread::isCurrent(dbb->dbb_crypto_manager->getCryptThreadHandle())) + activeThreadGuard = &dummyGuard; + else + activeThreadGuard = &threadGuard; + activeThreadGuard->enter(); } sync.lock(SYNC_EXCLUSIVE); From 178b64305bc5052d099fcd559452a22c69407d36 Mon Sep 17 00:00:00 2001 From: "aleksey.mochalov" Date: Wed, 22 Jan 2025 14:56:54 +0300 Subject: [PATCH 2/3] fix Windows build by introducing a method to retrieve thread id from crypto thread handle --- src/common/ThreadStart.cpp | 14 ++++++++++++++ src/common/ThreadStart.h | 1 + src/jrd/jrd.cpp | 7 ++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/common/ThreadStart.cpp b/src/common/ThreadStart.cpp index 8a4c9c2e42e..e90a62a24d3 100644 --- a/src/common/ThreadStart.cpp +++ b/src/common/ThreadStart.cpp @@ -219,6 +219,11 @@ ThreadId Thread::getId() #endif } +ThreadId Thread::getIdFromHandle(Handle threadHandle) +{ + return threadHandle; +} + bool Thread::isCurrent(InternalId iid) { return pthread_equal(iid, pthread_self()); @@ -368,6 +373,11 @@ ThreadId Thread::getId() return GetCurrentThreadId(); } +ThreadId Thread::getIdFromHandle(Handle threadHandle) +{ + return GetThreadId(threadHandle); +} + bool Thread::isCurrent(InternalId iid) { return GetCurrentThreadId() == iid; @@ -420,6 +430,10 @@ Thread::Handle Thread::getId() { } +Thread::Handle Thread::getIdFromHandle(Handle) +{ +} + void Thread::sleep(unsigned milliseconds) { } diff --git a/src/common/ThreadStart.h b/src/common/ThreadStart.h index 1aa821c7d3c..25d59638a1d 100644 --- a/src/common/ThreadStart.h +++ b/src/common/ThreadStart.h @@ -83,6 +83,7 @@ class Thread static void kill(Handle& handle); static ThreadId getId(); + static ThreadId getIdFromHandle(Handle threadHandle); static void sleep(unsigned milliseconds); static void yield(); diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index ce1e562ab36..0a7238b5a12 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -7765,10 +7765,15 @@ void release_attachment(thread_db* tdbb, Jrd::Attachment* attachment, XThreadEns XThreadEnsureUnlock* activeThreadGuard = dropGuard; if (!activeThreadGuard) { - if (dbb->dbb_crypto_manager && Thread::isCurrent(dbb->dbb_crypto_manager->getCryptThreadHandle())) + if (dbb->dbb_crypto_manager + && Thread::isCurrent(Thread::getIdFromHandle(dbb->dbb_crypto_manager->getCryptThreadHandle()))) + { activeThreadGuard = &dummyGuard; + } else + { activeThreadGuard = &threadGuard; + } activeThreadGuard->enter(); } From b92b819f5a2161b7aa2439516f9df5914e26f2e7 Mon Sep 17 00:00:00 2001 From: "aleksey.mochalov" Date: Thu, 23 Jan 2025 09:48:18 +0300 Subject: [PATCH 3/3] rename the crypt process thread id variable to its actual purpose as handle --- src/jrd/CryptoManager.cpp | 14 +++++++------- src/jrd/CryptoManager.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/jrd/CryptoManager.cpp b/src/jrd/CryptoManager.cpp index b3e17273304..118d0b54158 100644 --- a/src/jrd/CryptoManager.cpp +++ b/src/jrd/CryptoManager.cpp @@ -319,7 +319,7 @@ namespace Jrd { keyConsumers(getPool()), hash(getPool()), dbInfo(FB_NEW DbInfo(this)), - cryptThreadId(0), + cryptThreadHandle(0), cryptPlugin(NULL), checkFactory(NULL), dbb(*tdbb->getDatabase()), @@ -337,8 +337,8 @@ namespace Jrd { CryptoManager::~CryptoManager() { - if (cryptThreadId) - Thread::waitForCompletion(cryptThreadId); + if (cryptThreadHandle) + Thread::waitForCompletion(cryptThreadHandle); delete stateLock; delete threadLock; @@ -925,10 +925,10 @@ namespace Jrd { void CryptoManager::terminateCryptThread(thread_db*, bool wait) { flDown = true; - if (wait && cryptThreadId) + if (wait && cryptThreadHandle) { - Thread::waitForCompletion(cryptThreadId); - cryptThreadId = 0; + Thread::waitForCompletion(cryptThreadHandle); + cryptThreadHandle = 0; } } @@ -987,7 +987,7 @@ namespace Jrd { // ready to go guard.leave(); // release in advance to avoid races with cryptThread() - Thread::start(cryptThreadStatic, (THREAD_ENTRY_PARAM) this, THREAD_medium, &cryptThreadId); + Thread::start(cryptThreadStatic, (THREAD_ENTRY_PARAM) this, THREAD_medium, &cryptThreadHandle); } catch (const Firebird::Exception&) { diff --git a/src/jrd/CryptoManager.h b/src/jrd/CryptoManager.h index 0c153915fe1..ebc0dc8ee38 100644 --- a/src/jrd/CryptoManager.h +++ b/src/jrd/CryptoManager.h @@ -303,7 +303,7 @@ class CryptoManager final : public Firebird::PermanentStorage, public BarSync::I const char* getPluginName() const; Thread::Handle getCryptThreadHandle() const { - return cryptThreadId; + return cryptThreadHandle; } private: @@ -382,7 +382,7 @@ class CryptoManager final : public Firebird::PermanentStorage, public BarSync::I AttachmentsRefHolder keyProviders, keyConsumers; Firebird::string hash; Firebird::RefPtr dbInfo; - Thread::Handle cryptThreadId; + Thread::Handle cryptThreadHandle; Firebird::IDbCryptPlugin* cryptPlugin; Factory* checkFactory; Database& dbb;