Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574533002:
Fix integer overflow in ProbeController. (Closed)
|
Created:
4 years ago by Sergey Ulanov Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix integer overflow in ProbeController.
Previously ProbeController would overflow int when calculating
min_bitrate_to_probe_further_bps and when probing bitrate is
above 17 Mbps. The problem was introduced in
https://codereview.webrtc.org/2504023002. Fixed ProbeController to use
int64_t internally for bitrate calculations.
BUG=6332
Committed: https://crrev.com/5bc3945f8f2c5922780f6e2c4840b34bb0739632
Cr-Commit-Position: refs/heads/master@{#15642}
Patch Set 1 #
Total comments: 4
Patch Set 2 : . #
Total comments: 6
Patch Set 3 : . #
Messages
Total messages: 21 (10 generated)
sergeyu@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_controller.cc:98: /*probe_furter=*/false); 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|. https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_controller.cc:227: min_bitrate_to_probe_further_bps_ = rtc::checked_cast<int>( 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.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_controller.cc:98: /*probe_furter=*/false); On 2016/12/13 14:26:27, philipel wrote: > 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|. Done. https://codereview.webrtc.org/2574533002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_controller.cc:227: min_bitrate_to_probe_further_bps_ = rtc::checked_cast<int>( On 2016/12/13 14:26:27, philipel wrote: > 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. Changed all bps variables in this class to int64_t. FWIW style guide says the following: > If your variable represents a value that could ever be greater than or equal > to 2^31 (2GiB), use a 64-bit type such as int64_t. Keep in mind that even if > your value won't ever be too large for an int, it may be used in intermediate > calculations which may require a larger type. When in doubt, choose a larger > type. So yes, int64_t is a better choice for bps values.
lgtm with comments. https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:80: int old_max_bitrate_bps = max_bitrate_bps_; int64_t https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller_unittest.cc (right): https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller_unittest.cc:164: int kMbps = 1000000; const int/int64_t kBitsPerSecond = 1000000; kMbps = 1000000 sound like you set the rate to one million Mbps or 1000 Gbps.
Oops, missed an int -> int64_t in my last set of comments. https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:205: int max_probe_bitrate_bps = int64_t
https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:80: int old_max_bitrate_bps = max_bitrate_bps_; On 2016/12/14 09:33:44, philipel wrote: > int64_t Done. https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller.cc:205: int max_probe_bitrate_bps = On 2016/12/14 09:43:10, philipel wrote: > int64_t Done. https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_controller_unittest.cc (right): https://codereview.webrtc.org/2574533002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_controller_unittest.cc:164: int kMbps = 1000000; On 2016/12/14 09:33:44, philipel wrote: > const int/int64_t kBitsPerSecond = 1000000; > > kMbps = 1000000 sound like you set the rate to one million Mbps or 1000 Gbps. Renamed to kMbpsMultiplier.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2574533002/#ps60001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11483)
Description was changed from ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. ========== to ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. BUG=6332 ==========
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481827049511770,
"parent_rev": "987e25ca12cbf4ea836d9cc2b55a3ccf0821f2c3", "commit_rev":
"8fa23efdb5a8e7cacabf3cded134cb6a1887042d"}
Message was sent while issue was closed.
Description was changed from ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. BUG=6332 ========== to ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. BUG=6332 Review-Url: https://codereview.webrtc.org/2574533002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. BUG=6332 Review-Url: https://codereview.webrtc.org/2574533002 ========== to ========== Fix integer overflow in ProbeController. Previously ProbeController would overflow int when calculating min_bitrate_to_probe_further_bps and when probing bitrate is above 17 Mbps. The problem was introduced in https://codereview.webrtc.org/2504023002. Fixed ProbeController to use int64_t internally for bitrate calculations. BUG=6332 Committed: https://crrev.com/5bc3945f8f2c5922780f6e2c4840b34bb0739632 Cr-Commit-Position: refs/heads/master@{#15642} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5bc3945f8f2c5922780f6e2c4840b34bb0739632 Cr-Commit-Position: refs/heads/master@{#15642} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
