From a72826c79f4c691ad7d1591cf182ce49a5b5056e Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 1 Jul 2021 12:03:01 +0200
Subject: [PATCH] dnsdist: Warn if we could not re-open the LogResponseAction
 file

---
 pdns/dnsdist-lua-actions.cc | 61 ++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/pdns/dnsdist-lua-actions.cc b/pdns/dnsdist-lua-actions.cc
index 9d57f5507a..5399edd838 100644
--- a/pdns/dnsdist-lua-actions.cc
+++ b/pdns/dnsdist-lua-actions.cc
@@ -737,8 +737,7 @@ public:
       return;
     }
 
-    reopenLogFile();
-    if (!d_fp) {
+    if (!reopenLogFile())  {
       throw std::runtime_error("Unable to open file '" + str + "' for logging: " + stringerror());
     }
   }
@@ -800,32 +799,32 @@ public:
 
   void reload() override
   {
-    reopenLogFile();
+    if (!reopenLogFile()) {
+      warnlog("Unable to open file '%s' for logging: %s", d_fname, stringerror());
+    }
   }
 
 private:
-  void reopenLogFile()
+  bool reopenLogFile()
   {
-    std::shared_ptr<FILE> fp = nullptr;
-
-    if (d_append) {
-      fp = std::shared_ptr<FILE>(fopen(d_fname.c_str(), "a+"), fclose);
-    }
-    else {
-      fp = std::shared_ptr<FILE>(fopen(d_fname.c_str(), "w"), fclose);
-    }
-
-    if (!fp) {
+    // we are using a naked pointer here because we don't want fclose to be called
+    // with a nullptr, which would happen if we constructor a shared_ptr with fclose
+    // as a custom deleter and nullptr as a FILE*
+    auto nfp = fopen(d_fname.c_str(), d_append ? "a+" : "w");
+    if (!nfp) {
       /* don't fall on our sword when reopening */
-      warnlog("Unable to open file '%s' for logging: %s", d_fname, stringerror());
-      return;
+      return false;
     }
 
+    auto fp = std::shared_ptr<FILE>(nfp, fclose);
+    nfp = nullptr;
+
     if (!d_buffered) {
       setbuf(fp.get(), 0);
     }
 
     std::atomic_store_explicit(&d_fp, fp, std::memory_order_release);
+    return true;
   }
 
   std::string d_fname;
@@ -850,8 +849,7 @@ public:
       return;
     }
 
-    reopenLogFile();
-    if (!d_fp) {
+    if (!reopenLogFile()) {
       throw std::runtime_error("Unable to open file '" + str + "' for logging: " + stringerror());
     }
   }
@@ -890,31 +888,32 @@ public:
 
   void reload() override
   {
-    reopenLogFile();
+    if (!reopenLogFile()) {
+      warnlog("Unable to open file '%s' for logging: %s", d_fname, stringerror());
+    }
   }
 
 private:
-  void reopenLogFile()
+  bool reopenLogFile()
   {
-    std::shared_ptr<FILE> fp = nullptr;
-
-    if (d_append) {
-      fp = std::shared_ptr<FILE>(fopen(d_fname.c_str(), "a+"), fclose);
-    }
-    else {
-      fp = std::shared_ptr<FILE>(fopen(d_fname.c_str(), "w"), fclose);
-    }
-
-    if (!fp) {
+    // we are using a naked pointer here because we don't want fclose to be called
+    // with a nullptr, which would happen if we constructor a shared_ptr with fclose
+    // as a custom deleter and nullptr as a FILE*
+    auto nfp = fopen(d_fname.c_str(), d_append ? "a+" : "w");
+    if (!nfp) {
       /* don't fall on our sword when reopening */
-      return;
+      return false;
     }
 
+    auto fp = std::shared_ptr<FILE>(nfp, fclose);
+    nfp = nullptr;
+
     if (!d_buffered) {
       setbuf(fp.get(), 0);
     }
 
     std::atomic_store_explicit(&d_fp, fp, std::memory_order_release);
+    return true;
   }
 
   std::string d_fname;
-- 
2.47.2