Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879863002:
Fix for fuzzer bug. (Closed)
DescriptionFixed NetEq overflow bug.
Negating an int can result in a value that cannot be represented as an int. This is fixed here by using a 64 bit variable.
BUG=chromium:663611
Review-Url: https://codereview.webrtc.org/2879863002
Cr-Commit-Position: refs/heads/master@{#18167}
Committed: https://chromium.googlesource.com/external/webrtc/+/e3fc11464e5b295c53c897d3dcb4da0cdf10fc21
Patch Set 1 #
Total comments: 11
Patch Set 2 : Review comments. #Messages
Total messages: 16 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Fix for fuzzer bug. BUG= ========== to ========== Fixed NetEq overflow bug. Negating an int can result in a value that cannot be represented as an int. This is fixed here by using a 64 bit variable. BUG=chromium:663611 ==========
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Hi guys, PTAL at this bugfix for NetEq. I verified that the issue is resolved using this CL. https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:107: noise_fast_forward_ = rtc::saturated_cast<size_t>(noise_fast_forward_ + Not sure if saturated_cast is the right thing to do here, wdyt? https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:110: rtc::saturated_cast<int32_t>(timestamp_diff + excess_waiting_time_samp); Here I do think saturated_cast makes sense, since the only thing timestamp_diff is used for below is to check if it's < 0, which should be preserved by saturated_cast.
LGTM with one comment. https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:107: noise_fast_forward_ = rtc::saturated_cast<size_t>(noise_fast_forward_ + On 2017/05/12 15:00:26, ivoc wrote: > Not sure if saturated_cast is the right thing to do here, wdyt? I think you should use a dchecked_cast instead. This variable should represent times on the order of seconds. Even on a 32-bit platform, maxing out a size_t (32-bit) at 48000 samples per second is more than 24 hours. https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:110: rtc::saturated_cast<int32_t>(timestamp_diff + excess_waiting_time_samp); On 2017/05/12 15:00:26, ivoc wrote: > Here I do think saturated_cast makes sense, since the only thing timestamp_diff > is used for below is to check if it's < 0, which should be preserved by > saturated_cast. Acknowledged.
https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:100: int64_t excess_waiting_time_samp = Add const while you're at it? https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:107: noise_fast_forward_ = rtc::saturated_cast<size_t>(noise_fast_forward_ + On 2017/05/15 08:13:45, hlundin-webrtc wrote: > On 2017/05/12 15:00:26, ivoc wrote: > > Not sure if saturated_cast is the right thing to do here, wdyt? > > I think you should use a dchecked_cast instead. This variable should represent > times on the order of seconds. Even on a 32-bit platform, maxing out a size_t > (32-bit) at 48000 samples per second is more than 24 hours. +1 for dchecked_cast https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:110: rtc::saturated_cast<int32_t>(timestamp_diff + excess_waiting_time_samp); On 2017/05/15 08:13:45, hlundin-webrtc wrote: > On 2017/05/12 15:00:26, ivoc wrote: > > Here I do think saturated_cast makes sense, since the only thing > timestamp_diff > > is used for below is to check if it's < 0, which should be preserved by > > saturated_cast. > > Acknowledged. Umm... excess_waiting_time_samp == -static_cast<int64_t>(timestamp_diff) - optimal_level_samp. So why not avoid updating timestamp_diff, and replace timestamp_diff < 0 below with optimal_level_samp > 0 ?
https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:110: rtc::saturated_cast<int32_t>(timestamp_diff + excess_waiting_time_samp); On 2017/05/15 09:02:45, kwiberg-webrtc wrote: > On 2017/05/15 08:13:45, hlundin-webrtc wrote: > > On 2017/05/12 15:00:26, ivoc wrote: > > > Here I do think saturated_cast makes sense, since the only thing > > timestamp_diff > > > is used for below is to check if it's < 0, which should be preserved by > > > saturated_cast. > > > > Acknowledged. > > Umm... excess_waiting_time_samp == -static_cast<int64_t>(timestamp_diff) - > optimal_level_samp. So why not avoid updating timestamp_diff, and replace > > timestamp_diff < 0 > > below with > > optimal_level_samp > 0 > > ? But the modification of timestamp_diff is only done conditioned on excess_waiting_time_samp > optimal_level_samp / 2.
lgtm https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:110: rtc::saturated_cast<int32_t>(timestamp_diff + excess_waiting_time_samp); On 2017/05/15 09:48:45, hlundin-webrtc wrote: > On 2017/05/15 09:02:45, kwiberg-webrtc wrote: > > On 2017/05/15 08:13:45, hlundin-webrtc wrote: > > > On 2017/05/12 15:00:26, ivoc wrote: > > > > Here I do think saturated_cast makes sense, since the only thing > > > timestamp_diff > > > > is used for below is to check if it's < 0, which should be preserved by > > > > saturated_cast. > > > > > > Acknowledged. > > > > Umm... excess_waiting_time_samp == -static_cast<int64_t>(timestamp_diff) - > > optimal_level_samp. So why not avoid updating timestamp_diff, and replace > > > > timestamp_diff < 0 > > > > below with > > > > optimal_level_samp > 0 > > > > ? > > But the modification of timestamp_diff is only done conditioned on > excess_waiting_time_samp > optimal_level_samp / 2. Right. And you can't assign -timestamp_diff or dchecked_cast<int32_t>(timestamp_diff + excess_waiting_time_samp), because those two (equivalent) expressions may evaluate to -INT32_MIN... Hmpf. You could make a bool that's set to optimal_level_samp > 0 in this branch and timestamp_diff < 0 in the other, but I guess just using saturating_cast here is just as good.
Thanks for the comments! https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decision_logic_normal.cc (right): https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:100: int64_t excess_waiting_time_samp = On 2017/05/15 09:02:45, kwiberg-webrtc wrote: > Add const while you're at it? Done. https://codereview.webrtc.org/2879863002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decision_logic_normal.cc:107: noise_fast_forward_ = rtc::saturated_cast<size_t>(noise_fast_forward_ + On 2017/05/15 09:02:45, kwiberg-webrtc wrote: > On 2017/05/15 08:13:45, hlundin-webrtc wrote: > > On 2017/05/12 15:00:26, ivoc wrote: > > > Not sure if saturated_cast is the right thing to do here, wdyt? > > > > I think you should use a dchecked_cast instead. This variable should represent > > times on the order of seconds. Even on a 32-bit platform, maxing out a size_t > > (32-bit) at 48000 samples per second is more than 24 hours. > > +1 for dchecked_cast Done.
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2879863002/#ps60001 (title: "Review comments.")
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": 1494942119386250,
"parent_rev": "52c83fe7102f566cf35a7533092873d58b38f426", "commit_rev":
"e3fc11464e5b295c53c897d3dcb4da0cdf10fc21"}
Message was sent while issue was closed.
Description was changed from ========== Fixed NetEq overflow bug. Negating an int can result in a value that cannot be represented as an int. This is fixed here by using a 64 bit variable. BUG=chromium:663611 ========== to ========== Fixed NetEq overflow bug. Negating an int can result in a value that cannot be represented as an int. This is fixed here by using a 64 bit variable. BUG=chromium:663611 Review-Url: https://codereview.webrtc.org/2879863002 Cr-Commit-Position: refs/heads/master@{#18167} Committed: https://chromium.googlesource.com/external/webrtc/+/e3fc11464e5b295c53c897d3d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/e3fc11464e5b295c53c897d3d... |
