From 7b5e66a823785569c0c5b4bd6defaa02a34a2b1d Mon Sep 17 00:00:00 2001
From: Miod Vallat <miod.vallat@powerdns.com>
Date: Wed, 19 Feb 2025 09:54:43 +0100
Subject: [PATCH] lmdb: be sure to abort pending transactions in the correct
 order.

If the LMDBBackend destructor is invoked while there are still pending
transactions, these need to be aborted, but in the reverse order of
their creation (i.e. abort the innermost transaction first).

The default destructor would abort them in a class field
declaration-dependent order, which may not match the actual cinematic.

We now remember which transaction is the innermost one, so that we can
abort them in the expected order.

This gets rid of "double free or corruption (top)" aborts with glibc,
and Address Sanitizer errors.
---
 modules/lmdbbackend/lmdbbackend.cc | 20 ++++++++++++++++++++
 modules/lmdbbackend/lmdbbackend.hh |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc
index bb701e628f9a..b73fcc04a748 100644
--- a/modules/lmdbbackend/lmdbbackend.cc
+++ b/modules/lmdbbackend/lmdbbackend.cc
@@ -809,6 +809,23 @@ LMDBBackend::LMDBBackend(const std::string& suffix)
   d_dolog = ::arg().mustDo("query-logging");
 }
 
+LMDBBackend::~LMDBBackend()
+{
+  // LMDB internals require that, if we have multiple transactions active,
+  // we destroy them in the reverse order of their creation, thus we can't
+  // let the default destructor take care of d_rotxn and d_rwtxn.
+  if (d_txnorder) {
+    // RO transaction more recent than RW transaction
+    d_rotxn.reset();
+    d_rwtxn.reset();
+  }
+  else {
+    // RW transaction more recent than RO transaction
+    d_rwtxn.reset();
+    d_rotxn.reset();
+  }
+}
+
 namespace boost
 {
 namespace serialization
@@ -1073,6 +1090,7 @@ bool LMDBBackend::startTransaction(const DNSName& domain, int domain_id)
     throw DBException("Attempt to start a transaction while one was open already");
   }
   d_rwtxn = getRecordsRWTransaction(real_id);
+  d_txnorder = false;
 
   d_transactiondomain = domain;
   d_transactiondomainid = real_id;
@@ -1443,6 +1461,7 @@ bool LMDBBackend::list(const DNSName& target, int /* id */, bool include_disable
   }
 
   d_rotxn = getRecordsROTransaction(di.id, d_rwtxn);
+  d_txnorder = true;
   d_getcursor = std::make_shared<MDBROCursor>(d_rotxn->txn->getCursor(d_rotxn->db->dbi));
 
   compoundOrdername co;
@@ -1503,6 +1522,7 @@ void LMDBBackend::lookup(const QType& type, const DNSName& qdomain, int zoneId,
   }
   // cout<<"get will look for "<<relqname<< " in zone "<<hunt<<" with id "<<zoneId<<" and type "<<type.toString()<<endl;
   d_rotxn = getRecordsROTransaction(zoneId, d_rwtxn);
+  d_txnorder = true;
 
   compoundOrdername co;
   d_getcursor = std::make_shared<MDBROCursor>(d_rotxn->txn->getCursor(d_rotxn->db->dbi));
diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh
index 0b40b5284be5..523f24871186 100644
--- a/modules/lmdbbackend/lmdbbackend.hh
+++ b/modules/lmdbbackend/lmdbbackend.hh
@@ -60,6 +60,7 @@ class LMDBBackend : public DNSBackend
 {
 public:
   explicit LMDBBackend(const string& suffix = "");
+  ~LMDBBackend();
 
   bool list(const DNSName& target, int id, bool include_disabled) override;
 
@@ -305,6 +306,7 @@ private:
 
   shared_ptr<RecordsROTransaction> d_rotxn; // for lookup and list
   shared_ptr<RecordsRWTransaction> d_rwtxn; // for feedrecord within begin/aborttransaction
+  bool d_txnorder{false}; // whether d_rotxn is more recent than d_rwtxn
   std::shared_ptr<RecordsRWTransaction> getRecordsRWTransaction(uint32_t id);
   std::shared_ptr<RecordsROTransaction> getRecordsROTransaction(uint32_t id, const std::shared_ptr<LMDBBackend::RecordsRWTransaction>& rwtxn = nullptr);
   int genChangeDomain(const DNSName& domain, const std::function<void(DomainInfo&)>& func);