SERVER-30283 Fix integer overflow in PingStats::hit()

Closes #1196

Signed-off-by: William Schultz <william.schultz@mongodb.com>

2 files changed, 26 insertions, 20 deletions

diff --git a/src/mongo/db/repl/topology_coordinator.cpp b/src/mongo/db/repl/topology_coordinator.cpp
index 29cce45feca..9676d1b2747 100644
--- a/src/mongo/db/repl/topology_coordinator.cpp
+++ b/src/mongo/db/repl/topology_coordinator.cpp
@@ -109,9 +109,6 @@ int indexOfIterator(const std::vector<T>& vec, typename std::vector<T>::const_it
     return static_cast<int>(it - vec.begin());
 }
 
-// Maximum number of retries for a failed heartbeat.
-const int kMaxHeartbeatRetries = 2;
-
 /**
  * Returns true if the only up heartbeats are auth errors.
  */
@@ -149,10 +146,11 @@ void appendOpTime(BSONObjBuilder* bob,
 void TopologyCoordinator::PingStats::start(Date_t now) {
     _lastHeartbeatStartDate = now;
     _numFailuresSinceLastStart = 0;
+    _goodHeartbeatReceived = false;
 }
 
 void TopologyCoordinator::PingStats::hit(Milliseconds millis) {
-    _numFailuresSinceLastStart = std::numeric_limits<int>::max();
+    _goodHeartbeatReceived = true;
     ++count;
 
     value = value == UninitializedPing ? millis : Milliseconds((value * 4 + millis) / 5);
@@ -907,8 +905,7 @@ std::pair<ReplSetHeartbeatArgs, Milliseconds> TopologyCoordinator::prepareHeartb
     Date_t now, const std::string& ourSetName, const HostAndPort& target) {
     PingStats& hbStats = _pings[target];
     Milliseconds alreadyElapsed = now - hbStats.getLastHeartbeatStartDate();
-    if (!_rsConfig.isInitialized() ||
-        (hbStats.getNumFailuresSinceLastStart() > kMaxHeartbeatRetries) ||
+    if (!_rsConfig.isInitialized() || (hbStats.hasFailed()) ||
         (alreadyElapsed >= _rsConfig.getHeartbeatTimeoutPeriodMillis())) {
         // This is either the first request ever for "target", or the heartbeat timeout has
         // passed, so we're starting a "new" heartbeat.
@@ -946,8 +943,7 @@ std::pair<ReplSetHeartbeatArgsV1, Milliseconds> TopologyCoordinator::prepareHear
     Date_t now, const std::string& ourSetName, const HostAndPort& target) {
     PingStats& hbStats = _pings[target];
     Milliseconds alreadyElapsed(now.asInt64() - hbStats.getLastHeartbeatStartDate().asInt64());
-    if (!_rsConfig.isInitialized() ||
-        (hbStats.getNumFailuresSinceLastStart() > kMaxHeartbeatRetries) ||
+    if ((!_rsConfig.isInitialized()) || (hbStats.hasFailed()) ||
         (alreadyElapsed >= _rsConfig.getHeartbeatTimeoutPeriodMillis())) {
         // This is either the first request ever for "target", or the heartbeat timeout has
         // passed, so we're starting a "new" heartbeat.
@@ -1025,8 +1021,7 @@ HeartbeatResponseAction TopologyCoordinator::processHeartbeatResponse(
     const Milliseconds alreadyElapsed = now - hbStats.getLastHeartbeatStartDate();
     Date_t nextHeartbeatStartDate;
     // Determine next heartbeat start time.
-    if (hbStats.getNumFailuresSinceLastStart() <= kMaxHeartbeatRetries &&
-        alreadyElapsed < _rsConfig.getHeartbeatTimeoutPeriod()) {
+    if ((!hbStats.hasFailed()) && (alreadyElapsed < _rsConfig.getHeartbeatTimeoutPeriod())) {
         // There are still retries left, let's use one.
         nextHeartbeatStartDate = now;
     } else {
@@ -1093,12 +1088,12 @@ HeartbeatResponseAction TopologyCoordinator::processHeartbeatResponse(
     if (!hbResponse.isOK()) {
         if (isUnauthorized) {
             hbData.setAuthIssue(now);
-        } else if (hbStats.getNumFailuresSinceLastStart() > kMaxHeartbeatRetries ||
-                   alreadyElapsed >= _rsConfig.getHeartbeatTimeoutPeriod()) {
+        } else if ((hbStats.hasFailed()) ||
+                   (alreadyElapsed >= _rsConfig.getHeartbeatTimeoutPeriod())) {
             hbData.setDownValues(now, hbResponse.getStatus().reason());
         } else {
-            LOG(3) << "Bad heartbeat response from " << target << "; trying again; Retries left: "
-                   << (kMaxHeartbeatRetries - hbStats.getNumFailuresSinceLastStart()) << "; "
+            LOG(3) << "Bad heartbeat response from " << target
+                   << "; trying again; Retries left: " << (hbStats.retriesLeft()) << "; "
                    << alreadyElapsed << " have already elapsed";
         }
     } else {
diff --git a/src/mongo/db/repl/topology_coordinator.h b/src/mongo/db/repl/topology_coordinator.h
index 44f5abcead0..09e5a7fd08a 100644
--- a/src/mongo/db/repl/topology_coordinator.h
+++ b/src/mongo/db/repl/topology_coordinator.h
@@ -52,6 +52,9 @@ class ReplSetConfig;
 class TagSubgroup;
 struct MemberState;
 
+// Maximum number of retries for a failed heartbeat.
+const int kMaxHeartbeatRetries = 2;
+
 /**
  * Replication Topology Coordinator
  *
@@ -1041,13 +1044,20 @@ public:
     }
 
     /**
-     * Gets the number of failures since start() was last called.
-     *
-     * This value is incremented by calls to miss(), cleared by calls to start() and
-     * set to the maximum possible value by calls to hit().
+     * Returns true if the number of failed heartbeats has exceeded the max number
+     * of heartbeat retries or if a good heartbeat has been received since the
+     * last call to start(). Otherwise, returns false.
+     */
+    bool hasFailed() const {
+        return (_numFailuresSinceLastStart > kMaxHeartbeatRetries) ||
+            (_goodHeartbeatReceived == true);
+    }
+
+    /**
+     * Gets the number of retries left for a failed heartbeat.
      */
-    int getNumFailuresSinceLastStart() const {
-        return _numFailuresSinceLastStart;
+    int retriesLeft() const {
+        return kMaxHeartbeatRetries - _numFailuresSinceLastStart;
     }
 
 private:
@@ -1057,6 +1067,7 @@ private:
     Milliseconds value = UninitializedPing;
     Date_t _lastHeartbeatStartDate;
     int _numFailuresSinceLastStart = std::numeric_limits<int>::max();
+    bool _goodHeartbeatReceived = false;
 };
 
 //