Fix a possible use-after-free in EDS transaction rollback

[MochalovAlexey]

EDS::Transaction::rollback() used to call m_connection.deleteTransaction() before reporting rollback errors. This cleanup may release or delete the owning EDS::Connection.

If rollback returned an error, the code then called conn.raise(), which uses getDataSourceName() and reads m_dbName from the potentially deleted connection. This could lead to a crash while building the error message.

The fix raises the rollback error while the connection is still alive and uses a catch/rethrow block to guarantee external transaction cleanup before propagating the original exception.

[aafemt]

I would suggest to use Cleanup class instead:

Cleanup cleanup = [&]()
{
	if (!retain)
	{
		detachFromJrdTran();
		m_connection.deleteTransaction(tdbb, this);
	}
};

[hvlad]

Why not use class Cleanup here and avoid re-throw and explicit cleanup ?

[MochalovAlexey]

Answered below #8991 (comment)

[MochalovAlexey]

I would suggest to use Cleanup class instead:

Cleanup cleanup = [&]()
{
	if (!retain)
	{
		detachFromJrdTran();
		m_connection.deleteTransaction(tdbb, this);
	}
};

That would be risky here because the cleanup destructor may throw from deleteTransaction().
If that happens while another exception is already being unwound, C++ will call std::terminate().

[aafemt]

That would be risky here because the cleanup destructor may throw from deleteTransaction().

Because it is a cleanup routine, I would make sure that it cannot throw. Throwing from rollback() can create an unrecoverable state of code.

[dyemanov]

Strictly speaking Alexey is correct. I'd say all usages of class Cleanup should be revisited, because CCH_release, TRA_rollback, whatever -- all they can throw (even if unlikely). Moreover, our code in /common/classes often throws from RAII destructors (usually this is system_call_failed). The question is whether we're going to solve this problem globally before v6 is out.

[aafemt]

I'd say all usages of class Cleanup should be revisited,

They say clang-tidy can track cases when noexcept functions call non-noexcept functions. May be it can handle destructors and lambdas. In this case it could be enough to mark Cleanup parameter as noexcept.

[AlexPeshkoff]

On 4/16/26 20:38, Dmitry Yemanov wrote: *dyemanov* left a comment (FirebirdSQL/firebird#8991) <#8991 (comment)> Strictly speaking Alexey is correct. I'd say all usages of |class Cleanup| should be revisited, because |CCH_release|, |TRA_rollback|, whatever -- all they can throw (even if unlikely). Moreover, our code in /common/classes often throws from RAII destructors (usually this is |system_call_failed|). The question is whether we're going to solve this problem globally before v6 is out.

Yes, Cleanup is very small part of bigger problem. We should solve the problem in general. May be ideal way is to merge exceptions according to ERR_punt() rules but right now I see no easy way to do it, std::uncaught_exceptions() returns only unwinding state and anyway we can't modify active exception. In absence of better solutions we may silently ignore errors in dtor, in most cases they are caused by problems related with initial exception.