From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 27 Jun 2025 14:40:38 +0000 (+0200)
Subject: lmdb-safe: Fix a small race in `getMDBEnv`
X-Git-Tag: rec-5.3.0-alpha2~37^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=57224e94d9189f4916bbd7b2aaf61fa0821b1a2c;p=thirdparty%2Fpdns.git

lmdb-safe: Fix a small race in `getMDBEnv`

I believe there is a small race in the `getMDBEnv`: if the database
file does not exist when we first try to get the file metadata, we
acquire the lock then create a new `MDBEnv` and store it in the map.
But what happens if a different thread created the database between
our first check and the call to `MDBEnv`? I believe we would create
a new environment and override the existing entry in the map, bypassing
the check.
This commit introduces a second check right after acquiring the lock
to prevent that.

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>
---

diff --git a/ext/lmdb-safe/lmdb-safe.cc b/ext/lmdb-safe/lmdb-safe.cc
index 84866cfa22..cb8f1885fa 100644
--- a/ext/lmdb-safe/lmdb-safe.cc
+++ b/ext/lmdb-safe/lmdb-safe.cc
@@ -180,39 +180,45 @@ std::shared_ptr<MDBEnv> getMDBEnv(const char* fname, int flags, int mode, uint64
   static std::map<tuple<dev_t, ino_t>, Value> s_envs;
   static std::mutex mut;
 
-  struct stat statbuf;
-  if(stat(fname, &statbuf)) {
-    if(errno != ENOENT)
+  struct stat statbuf{};
+  if (stat(fname, &statbuf) != 0) {
+    if (errno != ENOENT) {
       throw std::runtime_error("Unable to stat prospective mdb database: "+string(strerror(errno)));
-    else {
-      std::lock_guard<std::mutex> l(mut);
+    }
+    std::lock_guard<std::mutex> lock(mut);
+    /* we need to check again _after_ taking the lock, otherwise a different thread might have created
+       it in the meantime */
+    if (stat(fname, &statbuf) != 0) {
+      if (errno != ENOENT) {
+        throw std::runtime_error("Unable to stat prospective mdb database: "+string(strerror(errno)));
+      }
       auto fresh = std::make_shared<MDBEnv>(fname, flags, mode, mapsizeMB);
-      if(stat(fname, &statbuf))
+      if (stat(fname, &statbuf) != 0) {
         throw std::runtime_error("Unable to stat prospective mdb database: "+string(strerror(errno)));
+      }
       auto key = std::tie(statbuf.st_dev, statbuf.st_ino);
-      s_envs[key] = {fresh, flags};
+      s_envs.emplace(key, Value{fresh, flags});
       return fresh;
     }
   }
 
-  std::lock_guard<std::mutex> l(mut);
+  std::lock_guard<std::mutex> lock(mut);
   auto key = std::tie(statbuf.st_dev, statbuf.st_ino);
   auto iter = s_envs.find(key);
-  if(iter != s_envs.end()) {
+  if (iter != s_envs.end()) {
     auto sp = iter->second.wp.lock();
-    if(sp) {
-      if(iter->second.flags != flags)
+    if (sp) {
+      if (iter->second.flags != flags) {
         throw std::runtime_error("Can't open mdb with differing flags");
+      }
 
       return sp;
     }
-    else {
-      s_envs.erase(iter); // useful if make_shared fails
-    }
+    s_envs.erase(iter); // useful if make_shared fails
   }
 
   auto fresh = std::make_shared<MDBEnv>(fname, flags, mode, mapsizeMB);
-  s_envs[key] = {fresh, flags};
+  s_envs.emplace(key, Value{fresh, flags});
 
   return fresh;
 }