Skip to content

This should fix bug #8296 : Crash in TipCache::findStates #8550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

This should fix bug #8296 : Crash in TipCache::findStates #8550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a2a3c7
This should fix bug #8296 : Crash in TipCache::findStates
hvlad May 4, 2025
9a69d24
Correction
hvlad May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 51 additions & 43 deletions src/jrd/tpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,14 +205,11 @@ CommitNumber TipCache::cacheState(TraNumber number)
if (number < oldest)
return CN_PREHISTORIC;

// It is possible to amortize barrier in getTransactionStatusBlock
// over large number of operations, if our callers are made aware of
// TransactionStatusBlock granularity and iterate over transactions
// directly. But since this function is not really called too frequently,
// it should not matter and we leave interface "as is" for now.
TpcBlockNumber blockNumber = number / m_transactionsPerBlock;
ULONG offset = number % m_transactionsPerBlock;
TransactionStatusBlock* block = getTransactionStatusBlock(header, blockNumber);

Sync sync(&m_sync_status, FB_FUNCTION);
TransactionStatusBlock* block = getTransactionStatusBlock(header, blockNumber, sync);

// This should not really happen ever
fb_assert(block);
Expand Down Expand Up @@ -334,7 +331,9 @@ void TipCache::loadInventoryPages(thread_db* tdbb, GlobalTpcHeader* header)

TpcBlockNumber blockNumber = hdr_oldest_transaction / m_transactionsPerBlock;
ULONG transOffset = hdr_oldest_transaction % m_transactionsPerBlock;
TransactionStatusBlock* statusBlock = getTransactionStatusBlock(header, blockNumber);

SyncLockGuard sync(&m_sync_status, SYNC_EXCLUSIVE, FB_FUNCTION);
TransactionStatusBlock* statusBlock = createTransactionStatusBlock(header->tpc_block_size, blockNumber);

for (TraNumber t = hdr_oldest_transaction; ; )
{
Expand All @@ -353,7 +352,7 @@ void TipCache::loadInventoryPages(thread_db* tdbb, GlobalTpcHeader* header)
{
blockNumber++;
transOffset = 0;
statusBlock = getTransactionStatusBlock(header, blockNumber);
statusBlock = createTransactionStatusBlock(header->tpc_block_size, blockNumber);
}
}
}
Expand All @@ -364,7 +363,10 @@ void TipCache::mapInventoryPages(GlobalTpcHeader* header)
const TpcBlockNumber lastNumber = header->latest_transaction_id / m_transactionsPerBlock;

for (; blockNumber <= lastNumber; blockNumber++)
getTransactionStatusBlock(header, blockNumber);
{
Sync sync(&m_sync_status, FB_FUNCTION);
getTransactionStatusBlock(header, blockNumber, sync);
}
}

TipCache::StatusBlockData::StatusBlockData(thread_db* tdbb, TipCache* tipCache, ULONG blockSize, TpcBlockNumber blkNumber)
Expand Down Expand Up @@ -498,20 +500,24 @@ TipCache::TransactionStatusBlock* TipCache::createTransactionStatusBlock(ULONG b
return blockData->memory->getHeader();
}

TipCache::TransactionStatusBlock* TipCache::getTransactionStatusBlock(GlobalTpcHeader* header, TpcBlockNumber blockNumber)
TipCache::TransactionStatusBlock* TipCache::getTransactionStatusBlock(GlobalTpcHeader* header, TpcBlockNumber blockNumber, Sync& sync)
{
fb_assert(sync.getState() == SYNC_NONE);

// This is a double-checked locking pattern. SyncLockGuard uses atomic ops internally and should be cheap
TransactionStatusBlock* block = NULL;
{
SyncLockGuard sync(&m_sync_status, SYNC_SHARED, "TipCache::getTransactionStatusBlock");
sync.lock(SYNC_SHARED);
BlocksMemoryMap::ConstAccessor acc(&m_blocks_memory);
if (acc.locate(blockNumber))
block = acc.current()->memory->getHeader();
else
sync.unlock();
}

if (!block)
{
SyncLockGuard sync(&m_sync_status, SYNC_EXCLUSIVE, "TipCache::getTransactionStatusBlock");
sync.lock(SYNC_EXCLUSIVE);
BlocksMemoryMap::ConstAccessor acc(&m_blocks_memory);
if (acc.locate(blockNumber))
block = acc.current()->memory->getHeader();
Expand All @@ -521,6 +527,8 @@ TipCache::TransactionStatusBlock* TipCache::getTransactionStatusBlock(GlobalTpcH
TraNumber oldest = header->oldest_transaction.load(std::memory_order_relaxed);
if (blockNumber >= oldest / m_transactionsPerBlock)
block = createTransactionStatusBlock(header->tpc_block_size, blockNumber);
else
sync.unlock();
}
}
return block;
Expand All @@ -532,24 +540,36 @@ TraNumber TipCache::findStates(TraNumber minNumber, TraNumber maxNumber, ULONG m
fb_assert(m_tpcHeader);
GlobalTpcHeader* header = m_tpcHeader->getHeader();

TransactionStatusBlock* statusBlock;
TpcBlockNumber blockNumber;
ULONG transOffset;
TransactionStatusBlock* statusBlock = nullptr;
ULONG transOffset = 0;

do
Sync sync(&m_sync_status, FB_FUNCTION);
for (TraNumber tran = minNumber; ; tran++, transOffset++)
{
TraNumber oldest = header->oldest_transaction.load(std::memory_order_relaxed);
if (transOffset == m_transactionsPerBlock)
{
sync.unlock();
statusBlock = nullptr;
}

while (!statusBlock)
{
const TraNumber oldest = header->oldest_transaction.load(std::memory_order_relaxed);

if (minNumber < oldest)
minNumber = oldest;
if (tran < oldest)
tran = oldest;

blockNumber = minNumber / m_transactionsPerBlock;
transOffset = minNumber % m_transactionsPerBlock;
statusBlock = getTransactionStatusBlock(header, blockNumber);
} while (!statusBlock);
const TpcBlockNumber blockNumber = tran / m_transactionsPerBlock;
transOffset = tran % m_transactionsPerBlock;
statusBlock = getTransactionStatusBlock(header, blockNumber, sync);

if (sync.getState() == SYNC_EXCLUSIVE)
sync.downgrade(SYNC_SHARED);
}

if (tran >= maxNumber)
break;

for (TraNumber t = minNumber; ; )
{
// Barrier is not needed here. Slightly out-dated information shall be ok here.
// Such transaction shall already be considered active by our caller.
// TODO: check if this assumption is indeed correct.
Expand Down Expand Up @@ -577,18 +597,8 @@ TraNumber TipCache::findStates(TraNumber minNumber, TraNumber maxNumber, ULONG m
break;
}

if (((1 << state) & mask) != 0)
return t;

if (++t >= maxNumber)
break;

if (++transOffset == m_transactionsPerBlock)
{
blockNumber++;
transOffset = 0;
statusBlock = getTransactionStatusBlock(header, blockNumber);
}
if (((1UL << state) & mask) != 0)
return tran;
}

return 0;
Expand All @@ -600,13 +610,11 @@ CommitNumber TipCache::setState(TraNumber number, int state)
fb_assert(m_tpcHeader);
GlobalTpcHeader* header = m_tpcHeader->getHeader();

// over large number of operations, if our callers are made aware of
// TransactionStatusBlock granularity and iterate over transactions
// directly. But since this function is not really called too frequently,
// it should not matter and we leave interface "as is" for now.
TpcBlockNumber blockNumber = number / m_transactionsPerBlock;
ULONG offset = number % m_transactionsPerBlock;
TransactionStatusBlock* block = getTransactionStatusBlock(header, blockNumber);

Sync sync(&m_sync_status, FB_FUNCTION);
TransactionStatusBlock* block = getTransactionStatusBlock(header, blockNumber, sync);

// This should not really happen
if (!block)
Expand Down Expand Up @@ -799,7 +807,7 @@ void TipCache::releaseSharedMemory(thread_db* tdbb, TraNumber oldest_old, TraNum
if (blocksToCleanup.isEmpty())
return;

SyncLockGuard sync(&m_sync_status, SYNC_EXCLUSIVE, "TipCache::releaseSharedMemory");
SyncLockGuard sync(&m_sync_status, SYNC_EXCLUSIVE, FB_FUNCTION);
while (blocksToCleanup.hasData())
{
TpcBlockNumber blockNumber = blocksToCleanup.pop();
Expand Down
7 changes: 5 additions & 2 deletions src/jrd/tpc_proto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,9 +311,12 @@ class TipCache

// Returns block holding transaction state.
// Returns NULL if requested block is too old and is no longer cached.
TransactionStatusBlock* getTransactionStatusBlock(GlobalTpcHeader* header, TpcBlockNumber blockNumber);
// Sync should be bound to m_sync_status. On enter, sync must be unlocked.
// If returns not NULL then sync remains locked.
TransactionStatusBlock* getTransactionStatusBlock(GlobalTpcHeader* header, TpcBlockNumber blockNumber, Firebird::Sync& sync);

// Map shared memory for a block
// Map shared memory for a block.
// Assume exclusive lock of m_sync_status.
TransactionStatusBlock* createTransactionStatusBlock(ULONG blockSize, TpcBlockNumber blockNumber);

// Release shared memory blocks, if possible.
Expand Down
Loading