From 8fdc60d882eadab43d51bcd4a20d5dadecf6f01c Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 24 Jun 2024 13:19:01 +0200
Subject: [PATCH] dnsdist: Fix a race when accessing a backend health status

While there should not be much risk in a data race involving a boolean
apart from getting an outdated value, it's still undefined behaviour
and it rightfully makes TSAN unhappy.
This commit makes the status atomic: hopefully using relaxed memory
ordering when reading the status will make it as cheap as a regular
non-atomic read on most platforms.
---
 pdns/dnsdistdist/dnsdist-backend.cc      |  4 ++--
 pdns/dnsdistdist/dnsdist-lua-bindings.cc |  5 ++++-
 pdns/dnsdistdist/dnsdist-web.cc          |  2 +-
 pdns/dnsdistdist/dnsdist.hh              | 10 +++++-----
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc
index 7f5603482f..2f400cf55a 100644
--- a/pdns/dnsdistdist/dnsdist-backend.cc
+++ b/pdns/dnsdistdist/dnsdist-backend.cc
@@ -834,7 +834,7 @@ void DownstreamState::submitHealthCheckResult(bool initial, bool newResult)
 
     currentCheckFailures++;
 
-    if (upStatus) {
+    if (upStatus.load()) {
       /* we were previously marked as "up" and failed a health-check,
          let's see if this is enough to move to the "down" state or if
          need more failed checks for that */
@@ -853,7 +853,7 @@ void DownstreamState::submitHealthCheckResult(bool initial, bool newResult)
     }
   }
 
-  if (newState != upStatus) {
+  if (newState != upStatus.load()) {
     /* we are actually moving to a new state */
     if (!IsAnyAddress(d_config.remote)) {
       infolog("Marking downstream %s as '%s'", getNameWithAddr(), newState ? "up" : "down");
diff --git a/pdns/dnsdistdist/dnsdist-lua-bindings.cc b/pdns/dnsdistdist/dnsdist-lua-bindings.cc
index e2bf8aef86..de53de97db 100644
--- a/pdns/dnsdistdist/dnsdist-lua-bindings.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-bindings.cc
@@ -144,7 +144,10 @@ void setupLuaBindings(LuaContext& luaCtx, bool client, bool configCheck)
   });
   luaCtx.registerFunction<std::string (DownstreamState::*)() const>("getName", [](const DownstreamState& state) -> const std::string& { return state.getName(); });
   luaCtx.registerFunction<std::string (DownstreamState::*)() const>("getNameWithAddr", [](const DownstreamState& state) -> const std::string& { return state.getNameWithAddr(); });
-  luaCtx.registerMember("upStatus", &DownstreamState::upStatus);
+  luaCtx.registerMember<bool(DownstreamState::*)>(
+    "upStatus",
+    [](const DownstreamState& state) -> bool { return state.upStatus.load(std::memory_order_relaxed); },
+    [](DownstreamState& state, bool newStatus) { state.upStatus.store(newStatus); });
   luaCtx.registerMember<int(DownstreamState::*)>(
     "weight",
     [](const DownstreamState& state) -> int { return state.d_config.d_weight; },
diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc
index 45c9b431e6..705646b68e 100644
--- a/pdns/dnsdistdist/dnsdist-web.cc
+++ b/pdns/dnsdistdist/dnsdist-web.cc
@@ -1078,7 +1078,7 @@ static void addServerToJSON(Json::array& servers, int identifier, const std::sha
     status = "DOWN";
   }
   else {
-    status = (backend->upStatus ? "up" : "down");
+    status = (backend->upStatus.load(std::memory_order_relaxed) ? "up" : "down");
   }
 
   Json::array pools;
diff --git a/pdns/dnsdistdist/dnsdist.hh b/pdns/dnsdistdist/dnsdist.hh
index cf4da8f6f5..b643c827f3 100644
--- a/pdns/dnsdistdist/dnsdist.hh
+++ b/pdns/dnsdistdist/dnsdist.hh
@@ -903,7 +903,7 @@ public:
   uint16_t currentCheckFailures{0};
   std::atomic<bool> hashesComputed{false};
   std::atomic<bool> connected{false};
-  bool upStatus{false};
+  std::atomic<bool> upStatus{false};
 
 private:
   void handleUDPTimeout(IDState& ids);
@@ -942,7 +942,7 @@ public:
     else if (d_config.availability == Availability::Up) {
       return true;
     }
-    return upStatus;
+    return upStatus.load(std::memory_order_relaxed);
   }
 
   void setUp()
@@ -952,8 +952,8 @@ public:
 
   void setUpStatus(bool newStatus)
   {
-    upStatus = newStatus;
-    if (!upStatus) {
+    upStatus.store(newStatus);
+    if (!newStatus) {
       latencyUsec = 0.0;
       latencyUsecTCP = 0.0;
     }
@@ -999,7 +999,7 @@ public:
       status = "DOWN";
     }
     else {
-      status = (upStatus ? "up" : "down");
+      status = (upStatus.load(std::memory_order_relaxed) ? "up" : "down");
     }
     return status;
   }
-- 
2.47.2