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.