Skip to content

Commit

Permalink
Fix potential deadlock when starting the encryption thread (#8403) B3…
Browse files Browse the repository at this point in the history
…_0_Release backport (#8412)

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

Co-authored-by: aleksey.mochalov <[email protected]>
  • Loading branch information
MochalovAlexey and AlexeyMochalov authored Jan 28, 2025
1 parent f20aba8 commit e7801a5
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 11 deletions.
28 changes: 26 additions & 2 deletions src/common/ThreadStart.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,19 @@ ThreadId Thread::getId()
#endif
}

ThreadId Thread::getIdFromHandle(Handle threadHandle)
{
return threadHandle;
}

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)
Expand Down Expand Up @@ -363,9 +373,19 @@ ThreadId Thread::getId()
return GetCurrentThreadId();
}

ThreadId Thread::getIdFromHandle(Handle threadHandle)
{
return GetThreadId(threadHandle);
}

bool Thread::isCurrent(InternalId iid)
{
return GetCurrentThreadId() == iid;
}

bool Thread::isCurrent()
{
return GetCurrentThreadId() == internalId;
return isCurrent(internalId);
}

void Thread::sleep(unsigned milliseconds)
Expand Down Expand Up @@ -410,6 +430,10 @@ Thread::Handle Thread::getId()
{
}

Thread::Handle Thread::getIdFromHandle(Handle)
{
}

void Thread::sleep(unsigned milliseconds)
{
}
Expand Down
2 changes: 2 additions & 0 deletions src/common/ThreadStart.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,12 @@ class Thread
static void kill(Handle& handle);

static ThreadId getId();
static ThreadId getIdFromHandle(Handle threadHandle);

static void sleep(unsigned milliseconds);
static void yield();

static bool isCurrent(InternalId iid);
bool isCurrent();

Thread()
Expand Down
14 changes: 7 additions & 7 deletions src/jrd/CryptoManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ namespace Jrd {
keyConsumers(getPool()),
hash(getPool()),
dbInfo(FB_NEW DbInfo(this)),
cryptThreadId(0),
cryptThreadHandle(0),
cryptPlugin(NULL),
checkFactory(NULL),
dbb(*tdbb->getDatabase()),
Expand All @@ -346,8 +346,8 @@ namespace Jrd {

CryptoManager::~CryptoManager()
{
if (cryptThreadId)
Thread::waitForCompletion(cryptThreadId);
if (cryptThreadHandle)
Thread::waitForCompletion(cryptThreadHandle);

delete stateLock;
delete threadLock;
Expand Down Expand Up @@ -933,10 +933,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;
}
}

Expand Down Expand Up @@ -995,7 +995,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&)
{
Expand Down
6 changes: 5 additions & 1 deletion src/jrd/CryptoManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,10 @@ class CryptoManager FB_FINAL : public Firebird::PermanentStorage, public BarSync
ULONG getCurrentPage(thread_db* tdbb) const;
UCHAR getCurrentState(thread_db* tdbb) const;
const char* getKeyName() const;
Thread::Handle getCryptThreadHandle() const
{
return cryptThreadHandle;
}

private:
enum IoResult {SUCCESS_ALL, FAILED_CRYPT, FAILED_IO};
Expand Down Expand Up @@ -388,7 +392,7 @@ class CryptoManager FB_FINAL : public Firebird::PermanentStorage, public BarSync
AttachmentsRefHolder keyProviders, keyConsumers;
Firebird::string hash;
Firebird::RefPtr<DbInfo> dbInfo;
Thread::Handle cryptThreadId;
Thread::Handle cryptThreadHandle;
Firebird::IDbCryptPlugin* cryptPlugin;
Factory* checkFactory;
Database& dbb;
Expand Down
12 changes: 11 additions & 1 deletion src/jrd/jrd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6433,9 +6433,19 @@ static void release_attachment(thread_db* tdbb, Jrd::Attachment* attachment)

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
XThreadEnsureUnlock threadGuard(dbb->dbb_thread_mutex, FB_FUNCTION);
threadGuard.enter();
XThreadEnsureUnlock* activeThreadGuard = &threadGuard;
if (dbb->dbb_crypto_manager
&& Thread::isCurrent(Thread::getIdFromHandle(dbb->dbb_crypto_manager->getCryptThreadHandle())))
{
activeThreadGuard = &dummyGuard;
}
activeThreadGuard->enter();

sync.lock(SYNC_EXCLUSIVE);

Expand Down

0 comments on commit e7801a5

Please sign in to comment.