Don't read past the end of the impulseResponse array

Don't read past the end of the impulseResponse array

BUG=281480
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157007

[Raymond Toy (Google), 2013-08-29 21:36:18]

PTAL.

[Ken Russell (switch to Gerrit), 2013-08-29 21:49:23]

https://codereview.chromium.org/23689004/diff/1/Source/core/platform/audio/Re...
File Source/core/platform/audio/ReverbConvolverStage.cpp (right):

https://codereview.chromium.org/23689004/diff/1/Source/core/platform/audio/Re...
Source/core/platform/audio/ReverbConvolverStage.cpp:62:
m_directKernel->copyToRange(impulseResponse + stageOffset, 0,
std::min(stageLength, fftSize / 2));
Why does this guarantee that we won't read beyond the end of the impulseResponse
array?

The header says that the second argument to this constructor is called
"responseLength". However, this argument is *ignored* here in the
implementation. Why is the correct value then not (something like):

  std::min(std::max(0, responseLength - stageOffset), fftSize / 2)

Though taking into account that responseLength and stageOffset are both
unsigned?

[Raymond Toy (Google), 2013-08-29 22:27:43]

On 2013/08/29 21:49:23, Ken Russell wrote:
>
https://codereview.chromium.org/23689004/diff/1/Source/core/platform/audio/Re...
> File Source/core/platform/audio/ReverbConvolverStage.cpp (right):
> 
>
https://codereview.chromium.org/23689004/diff/1/Source/core/platform/audio/Re...
> Source/core/platform/audio/ReverbConvolverStage.cpp:62:
> m_directKernel->copyToRange(impulseResponse + stageOffset, 0,
> std::min(stageLength, fftSize / 2));
> Why does this guarantee that we won't read beyond the end of the
impulseResponse
> array?
> 
> The header says that the second argument to this constructor is called
> "responseLength". However, this argument is *ignored* here in the
> implementation. Why is the correct value then not (something like):
> 
>   std::min(std::max(0, responseLength - stageOffset), fftSize / 2)
> 
> Though taking into account that responseLength and stageOffset are both
> unsigned?

The caller in ReverbConvolver.cpp enforces stageLength (called stageSize there)
to always be <= responseLength (totalResponseLength there).  And this branch of
the if only happens when stageOffset = 0.  But your suggested solution is much
safer.

[Raymond Toy (Google), 2013-08-30 16:38:44]

PTAL.  I've simplified the implementation; it now looks more like the other if
branch and added some ASSERTS.

The ASAN test passes, as do all of the layout tests, in a debug build.

[Ken Russell (switch to Gerrit), 2013-08-30 17:06:26]

OK, if you think this is the right fix. LGTM

[Raymond Toy (Google), 2013-08-30 17:29:59]

On 2013/08/30 17:06:26, Ken Russell wrote:
> OK, if you think this is the right fix. LGTM

Yeah, I think this is right.

Thanks.

[commit-bot: I haz the power, 2013-08-30 17:30:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/23689004/7001

[commit-bot: I haz the power, 2013-08-30 19:16:51]

Change committed as 157007