Merge pull request PowerDNS#14376 from rgacogne/ddist-upstatus-race

dnsdist: Fix a race when accessing a backend health status
GlobalCyberAlliance · Jun 28, 2024 · addf744 · addf744
2 parents 7b059eb + f4cf0d7
commit addf744
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 10 deletions.
diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc
@@ -803,7 +803,7 @@ void DownstreamState::submitHealthCheckResult(bool initial, bool newResult)
     currentCheckFailures = 0;
     consecutiveSuccessfulChecks++;
 
-    if (!upStatus) {
+    if (!upStatus.load(std::memory_order_relaxed)) {
       /* we were previously marked as "down" and had a successful health-check,
          let's see if this is enough to move to the "up" state or if we need
          more successful health-checks for that */
@@ -834,7 +834,7 @@ void DownstreamState::submitHealthCheckResult(bool initial, bool newResult)
 
     currentCheckFailures++;
 
-    if (upStatus) {
+    if (upStatus.load(std::memory_order_relaxed)) {
       /* 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(std::memory_order_relaxed)) {
     /* 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
@@ -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
@@ -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
@@ -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;
   }