From f4fc0ff6f98695eaee527762c27824938d57c442 Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 26 May 2016 22:40:09 +0200 Subject: [PATCH] Reland of Adding a some checks and switching out a few assert for RTC_[D]CHECK. (patchset #1 id:1 of https://codereview.webrtc.org/2018553002/ ) Adding a some checks and switching out a few assert for RTC_[D]CHECK. These changes are around use of AudioFrame.data_ to help us catch issues earlier since assert() is left out in release builds, including builds with DCHECK enabled. I've also added a few full-on CHECKs to avoid reading past buffer boundaries or continuing on in a failed state. TBR=kjellander@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Committed: https://crrev.com/60c4e0ae8f124f08372645a95042f4a1246d7aa3 Cr-Commit-Position: refs/heads/master@{#12925} Committed: https://crrev.com/5771beb265129082d31736259b7dc6ca037cff4d Cr-Commit-Position: refs/heads/master@{#12926} Committed: https://crrev.com/54e1c6a500e390e543bce7b78fae65eb9bb14ab6 Cr-Commit-Position: refs/heads/master@{#12927} Committed: https://crrev.com/f9d2fe983fe196373850c55acd3dc3824add480e Cr-Commit-Position: refs/heads/master@{#12928} Review URL: https://codereview.webrtc.org/2014973002 . Cr-Commit-Position: refs/heads/master@{#12929} --- .../common_audio/resampler/push_resampler.cc | 38 +++++++++++++++---- .../resampler/push_resampler_unittest.cc | 5 ++- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/webrtc/common_audio/resampler/push_resampler.cc b/webrtc/common_audio/resampler/push_resampler.cc index f654e9a397..03d481cd06 100644 --- a/webrtc/common_audio/resampler/push_resampler.cc +++ b/webrtc/common_audio/resampler/push_resampler.cc @@ -12,11 +12,33 @@ #include +#include "webrtc/base/checks.h" #include "webrtc/common_audio/include/audio_util.h" #include "webrtc/common_audio/resampler/include/resampler.h" #include "webrtc/common_audio/resampler/push_sinc_resampler.h" namespace webrtc { +namespace { +// These checks were factored out into a non-templatized function +// due to problems with clang on Windows in debug builds. +// For some reason having the DCHECKs inline in the template code +// caused the compiler to generate code that threw off the linker. +void CheckValidInitParams(int src_sample_rate_hz, int dst_sample_rate_hz, + size_t num_channels) { + RTC_DCHECK_GT(src_sample_rate_hz, 0); + RTC_DCHECK_GT(dst_sample_rate_hz, 0); + RTC_DCHECK_GT(num_channels, 0u); + RTC_DCHECK_LE(num_channels, 2u); +} + +void CheckExpectedBufferSizes(size_t num_channels, int src_sample_rate, + int dst_sample_rate) { + const size_t src_size_10ms = src_sample_rate * num_channels / 100; + const size_t dst_size_10ms = dst_sample_rate * num_channels / 100; + RTC_CHECK_EQ(src_length, src_size_10ms); + RTC_CHECK_GE(dst_capacity, dst_size_10ms); +} +} template PushResampler::PushResampler() @@ -33,15 +55,19 @@ template int PushResampler::InitializeIfNeeded(int src_sample_rate_hz, int dst_sample_rate_hz, size_t num_channels) { + CheckValidInitParams(src_sample_rate_hz, dst_sample_rate_hz, num_channels); + if (src_sample_rate_hz == src_sample_rate_hz_ && dst_sample_rate_hz == dst_sample_rate_hz_ && - num_channels == num_channels_) + num_channels == num_channels_) { // No-op if settings haven't changed. return 0; + } - if (src_sample_rate_hz <= 0 || dst_sample_rate_hz <= 0 || - num_channels <= 0 || num_channels > 2) + if (src_sample_rate_hz <= 0 || dst_sample_rate_hz <= 0 || num_channels <= 0 || + num_channels > 2) { return -1; + } src_sample_rate_hz_ = src_sample_rate_hz; dst_sample_rate_hz_ = dst_sample_rate_hz; @@ -68,10 +94,8 @@ int PushResampler::InitializeIfNeeded(int src_sample_rate_hz, template int PushResampler::Resample(const T* src, size_t src_length, T* dst, size_t dst_capacity) { - const size_t src_size_10ms = src_sample_rate_hz_ * num_channels_ / 100; - const size_t dst_size_10ms = dst_sample_rate_hz_ * num_channels_ / 100; - if (src_length != src_size_10ms || dst_capacity < dst_size_10ms) - return -1; + CheckExpectedBufferSizes(num_channels_, src_sample_rate_hz_, + dst_sample_rate_hz_) if (src_sample_rate_hz_ == dst_sample_rate_hz_) { // The old resampler provides this memcpy facility in the case of matching diff --git a/webrtc/common_audio/resampler/push_resampler_unittest.cc b/webrtc/common_audio/resampler/push_resampler_unittest.cc index 58880cc1b7..589ab3e574 100644 --- a/webrtc/common_audio/resampler/push_resampler_unittest.cc +++ b/webrtc/common_audio/resampler/push_resampler_unittest.cc @@ -22,7 +22,10 @@ TEST(PushResamplerTest, VerifiesInputParameters) { EXPECT_EQ(0, resampler.InitializeIfNeeded(16000, 16000, 2)); } -#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +// The below tests are temporarily disabled on WEBRTC_WIN due to problems +// with clang debug builds. +#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) && \ + !defined(WEBRTC_WIN) TEST(PushResamplerTest, VerifiesBadInputParameters1) { PushResampler resampler; EXPECT_DEATH(resampler.InitializeIfNeeded(-1, 16000, 1),