Fix integer overflow in ProbeController.

webrtc/modules/congestion_controller/probe_controller.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/congestion_controller/probe_controller.h"
 
 #include <algorithm>
 #include <initializer_list>
 
 #include "webrtc/base/logging.h"
+#include "webrtc/base/safe_conversions.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
 
 namespace {
 
 // Number of deltas between probes per cluster. On the very first cluster,
 // we will need kProbeDeltasPerCluster + 1 probes, but on a cluster following
 // another, we need kProbeDeltasPerCluster probes.
 constexpr int kProbeDeltasPerCluster = 5;
@@ skipping 10 matching lines @@
 // specify max bitrate.
 constexpr int kDefaultMaxProbingBitrateBps = 5000000;
 
 // This is a limit on how often probing can be done when there is a BW
 // drop detected in ALR.
 constexpr int64_t kAlrProbingIntervalMinMs = 5000;
 
 // Interval between probes when ALR periodic probing is enabled.
 constexpr int64_t kAlrPeriodicProbingIntervalMs = 5000;
 
-// Minimum probe bitrate percentage to probe further for repeated probes.
-constexpr int kRepeatedProbeMinPercentage = 125;
+// Minimum probe bitrate percentage to probe further for repeated probes,
+// relative to the previous probe. For example, if 1Mbps probe results in
+// 80kbps, then we'll probe again at 1.6Mbps. In that case second probe won't be
+// sent if we get 600kbps from the first one.
+constexpr int kRepeatedProbeMinPercentage = 70;
 
 }  // namespace
 
 ProbeController::ProbeController(PacedSender* pacer, Clock* clock)
     : pacer_(pacer),
       clock_(clock),
       network_state_(kNetworkUp),
       state_(State::kInit),
       min_bitrate_to_probe_further_bps_(kExponentialProbingDisabled),
       time_last_probing_initiated_ms_(0),
@@ skipping 21 matching lines @@
     case State::kInit:
       if (network_state_ == kNetworkUp)
         InitiateExponentialProbing();
       break;
 
     case State::kWaitingForProbingResult:
       break;
 
     case State::kProbingComplete:
       // Initiate probing when |max_bitrate_| was increased mid-call.
-      if (estimated_bitrate_bps_ != 0 &&
+      if (estimated_bitrate_bps_ != kExponentialProbingDisabled &&
           estimated_bitrate_bps_ < old_max_bitrate_bps &&
           max_bitrate_bps_ > old_max_bitrate_bps) {
         InitiateProbing(clock_->TimeInMilliseconds(), {max_bitrate_bps},
-                        kExponentialProbingDisabled);
+                        /*probe_furter=*/false);

[philipel, 2016/12/13 14:26:27]

In general, if you want to describe the parameter just:
const bool probe_further = false;
InitiateProbing(..., probe_further);

I don't think you need to describe it though, just use |true| or |false|.

[Sergey Ulanov, 2016/12/13 19:38:09]

Done.

       }
       break;
   }
 }
 
 void ProbeController::OnNetworkStateChanged(NetworkState network_state) {
   rtc::CritScope cs(&critsect_);
   network_state_ = network_state;
   if (network_state_ == kNetworkUp && state_ == State::kInit)
     InitiateExponentialProbing();
 }
 
 void ProbeController::InitiateExponentialProbing() {
   RTC_DCHECK(network_state_ == kNetworkUp);
   RTC_DCHECK(state_ == State::kInit);
   RTC_DCHECK_GT(start_bitrate_bps_, 0);
 
   // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of
   // 1.2 Mbps to continue probing.
   InitiateProbing(clock_->TimeInMilliseconds(),
-                  {3 * start_bitrate_bps_, 6 * start_bitrate_bps_},
-                  4 * start_bitrate_bps_);
+                  {3 * static_cast<int64_t>(start_bitrate_bps_),
+                   6 * static_cast<int64_t>(start_bitrate_bps_)},
+                  /*probe_furter=*/true);
 }
 
 void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
   rtc::CritScope cs(&critsect_);
   int64_t now_ms = clock_->TimeInMilliseconds();
 
   if (state_ == State::kWaitingForProbingResult) {
     // Continue probing if probing results indicate channel has greater
     // capacity.
     LOG(LS_INFO) << "Measured bitrate: " << bitrate_bps
                  << " Minimum to probe further: "
                  << min_bitrate_to_probe_further_bps_;
     if (min_bitrate_to_probe_further_bps_ != kExponentialProbingDisabled &&
         bitrate_bps > min_bitrate_to_probe_further_bps_) {
-      // Double the probing bitrate and expect a minimum of 25% gain to
-      // continue probing.
-      InitiateProbing(now_ms, {2 * bitrate_bps},
-                      bitrate_bps * kRepeatedProbeMinPercentage / 100);
+      // Double the probing bitrate.
+      InitiateProbing(now_ms, {2 * static_cast<int64_t>(bitrate_bps)},
+                      /*probe_furter=*/true);
     }
   }
 
   // Detect a drop in estimated BW when operating in ALR and not already
   // probing. The current response is to initiate a single probe session at the
   // previous bitrate and immediately use the reported bitrate as the new
   // bitrate.
   //
   // If the probe session fails, the assumption is that this drop was a
   // real one from a competing flow or something else on the network and
   // it ramps up from bitrate_bps.
   if (state_ == State::kProbingComplete &&
       pacer_->GetApplicationLimitedRegionStartTime() &&
       bitrate_bps < estimated_bitrate_bps_ / 2 &&
       (now_ms - last_alr_probing_time_) > kAlrProbingIntervalMinMs) {
     LOG(LS_INFO) << "Detected big BW drop in ALR, start probe.";
     // Track how often we probe in response to BW drop in ALR.
     RTC_HISTOGRAM_COUNTS_10000("WebRTC.BWE.AlrProbingIntervalInS",
                                (now_ms - last_alr_probing_time_) / 1000);
-    InitiateProbing(now_ms, {estimated_bitrate_bps_},
-                    kExponentialProbingDisabled);
+    InitiateProbing(now_ms, {estimated_bitrate_bps_}, /*probe_furter=*/false);
     last_alr_probing_time_ = now_ms;
 
     // TODO(isheriff): May want to track when we did ALR probing in order
     // to reset |last_alr_probing_time_| if we validate that it was a
     // drop due to exogenous event.
   }
 
   estimated_bitrate_bps_ = bitrate_bps;
 }
 
@@ skipping 19 matching lines @@
     return;
 
   // Probe bandwidth periodically when in ALR state.
   rtc::Optional<int64_t> alr_start_time =
       pacer_->GetApplicationLimitedRegionStartTime();
   if (alr_start_time) {
     int64_t next_probe_time_ms =
         std::max(*alr_start_time, time_last_probing_initiated_ms_) +
         kAlrPeriodicProbingIntervalMs;
     if (now_ms >= next_probe_time_ms) {
-      InitiateProbing(
-          now_ms, {estimated_bitrate_bps_ * 2},
-          estimated_bitrate_bps_ * kRepeatedProbeMinPercentage / 100);
+      InitiateProbing(now_ms,
+                      {static_cast<int64_t>(estimated_bitrate_bps_) * 2},
+                      /*probe_furter=*/true);
     }
   }
 }
 
 void ProbeController::InitiateProbing(
     int64_t now_ms,
-    std::initializer_list<int> bitrates_to_probe,
-    int min_bitrate_to_probe_further_bps) {
+    std::initializer_list<int64_t> bitrates_to_probe,
+    bool probe_further) {
   bool first_cluster = true;
-  for (int bitrate : bitrates_to_probe) {
+  for (int64_t bitrate : bitrates_to_probe) {
     int max_probe_bitrate_bps =
         max_bitrate_bps_ > 0 ? max_bitrate_bps_ : kDefaultMaxProbingBitrateBps;
-    bitrate = std::min(bitrate, max_probe_bitrate_bps);
+    if (bitrate > max_probe_bitrate_bps) {
+      bitrate = max_probe_bitrate_bps;
+      probe_further = false;
+    }
     if (first_cluster) {
       pacer_->CreateProbeCluster(bitrate, kProbeDeltasPerCluster + 1);
       first_cluster = false;
     } else {
       pacer_->CreateProbeCluster(bitrate, kProbeDeltasPerCluster);
     }
   }
-  min_bitrate_to_probe_further_bps_ = min_bitrate_to_probe_further_bps;
   time_last_probing_initiated_ms_ = now_ms;
-  if (min_bitrate_to_probe_further_bps == kExponentialProbingDisabled)
+  if (probe_further) {
+    state_ = State::kWaitingForProbingResult;
+    min_bitrate_to_probe_further_bps_ = rtc::checked_cast<int>(

[philipel, 2016/12/13 14:26:27]

As I see it, we are ok to store bitrates in 32 bit integers (maxing out at 2.1
Gbps).

Don't you think it is easier to just do the calculations in 64 bits and then
continue to store the bitrates in ints? If not, I think we should use int64_t
for all bitrates.

[Sergey Ulanov, 2016/12/13 19:38:09]

Changed all bps variables in this class to int64_t. FWIW style guide says the
following:
So yes, int64_t is a better choice for bps values.

+        (*(bitrates_to_probe.end() - 1)) * kRepeatedProbeMinPercentage / 100);
+  } else {
     state_ = State::kProbingComplete;
-  else
-    state_ = State::kWaitingForProbingResult;
+    min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
+  }
 }
 
 }  // namespace webrtc