Check for out-of-bounds access on |kIntrpCoef|

Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953
Committed: https://crrev.com/7bf939c72061b48789304315c857c65a6ad8a1ae
Cr-Commit-Position: refs/heads/master@{#13039}

[Martin Barbella, 2016-05-28 03:03:43]

Description was changed from

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=
==========

to

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953
==========

[Martin Barbella, 2016-05-28 03:03:43]

mbarbella@chromium.org changed reviewers:
+ kwiberg@webrtc.org

[Martin Barbella, 2016-05-28 03:04:32]

PTAL. Not sure if this is the best way to solve this.

[Martin Barbella, 2016-05-30 21:19:00]

Ping.

[kwiberg-webrtc, 2016-05-31 02:52:44]

kwiberg@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[kwiberg-webrtc, 2016-05-31 02:52:45]

Thanks.

And me neither. In particular, I have no idea what the numbers in kIntrpCoef
mean. Adding Tina, who will sometimes admit to being familiar with this sort of
code.

However, an educated guess: since the problem is only found in fuzzer tests (?),
the fix is likely to work fine even if it isn't strictly speaking the right
thing to do. In other words, I suspect that if the condition you test for is
true, any output will do as long as the code doesn't misbehave while producing
it.

We have a bunch of tests that use iSAC, among them some that check that its
output is bit-for-bit identical to a reference value. Have you unleashed the try
bots?

[Martin Barbella, 2016-05-31 15:43:09]

On 2016/05/31 02:52:45, kwiberg-webrtc wrote:
> Thanks.
> 
> And me neither. In particular, I have no idea what the numbers in kIntrpCoef
> mean. Adding Tina, who will sometimes admit to being familiar with this sort
of
> code.
> 
> However, an educated guess: since the problem is only found in fuzzer tests
(?),
> the fix is likely to work fine even if it isn't strictly speaking the right
> thing to do. In other words, I suspect that if the condition you test for is
> true, any output will do as long as the code doesn't misbehave while producing
> it.
> 
> We have a bunch of tests that use iSAC, among them some that check that its
> output is bit-for-bit identical to a reference value. Have you unleashed the
try
> bots?

Trying a few now. If we ever hit this case in any of the tests, the ASan bots
should have detected a buffer overflow. I suspect you're right that the behavior
doesn't matter all that much if we haven't seen issues here before, but I'd be
happy to update it if it seems like there is something that would be more
appropriate.

[Martin Barbella, 2016-06-01 22:26:11]

Would either of you mind taking another look?

[Martin Barbella, 2016-06-02 22:41:34]

Another ping. We'd like to get this security bug closed soon.

[tlegrand-webrtc, 2016-06-03 05:17:02]

On 2016/06/02 22:41:34, Martin Barbella wrote:
> Another ping. We'd like to get this security bug closed soon.

Sorry for keeping you waiting. Since there are no test added for this, I need to
find time to run the code and check what I think the code should do in this
case. I haven't had any time to do that yet.

[tlegrand-webrtc, 2016-06-03 07:20:40]

On 2016/06/03 05:17:02, tlegrand-webrtc wrote:
> On 2016/06/02 22:41:34, Martin Barbella wrote:
> > Another ping. We'd like to get this security bug closed soon.
> 
> Sorry for keeping you waiting. Since there are no test added for this, I need
to
> find time to run the code and check what I think the code should do in this
> case. I haven't had any time to do that yet.

I didn't run the code, but looked closer at what it does, and think this is a
safe change. Thanks for taking the time to fix this! LGTM

[Martin Barbella, 2016-06-03 15:35:24]

The CQ bit was checked by mbarbella@chromium.org

[commit-bot: I haz the power, 2016-06-03 15:35:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025493002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2025493002/1

[commit-bot: I haz the power, 2016-06-03 17:06:01]

Description was changed from

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953
==========

to

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953
==========

[commit-bot: I haz the power, 2016-06-03 17:06:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-03 17:06:09]

Description was changed from

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953
==========

to

==========
Check for out-of-bounds access on |kIntrpCoef|.

BUG=chromium:600953

Committed: https://crrev.com/7bf939c72061b48789304315c857c65a6ad8a1ae
Cr-Commit-Position: refs/heads/master@{#13039}
==========

[commit-bot: I haz the power, 2016-06-03 17:06:10]

Patchset 1 (id:??) landed as
https://crrev.com/7bf939c72061b48789304315c857c65a6ad8a1ae
Cr-Commit-Position: refs/heads/master@{#13039}