Added boundary check for array access as a short-term way of fixing the bug of out-of-bounds reads …

Added boundary check for array access as a short-term way of fixing the bug of out-of-bounds reads into the array

BUG=chromium:529527, chromium:529552
Committed: https://crrev.com/fc9dd1710d1e9f52846931cabed97134393d0b82
Cr-Commit-Position: refs/heads/master@{#9930}

[peah-webrtc, 2015-09-14 07:09:45]

peah@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, solenberg@webrtc.org

[peah-webrtc, 2015-09-14 07:09:46]

This is a bug fix for ensuring that the out-of-bounds array reads reported in a
fuzz-test does not occur. The fix is a short-term fix, as the related statistic
is not reliable with the fix (nor with the original erroneous behavior).

[hlundin-webrtc, 2015-09-14 07:17:43]

LG. But please, see comments.

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:823: const int histogram_length =
sizeof( self->delay_histogram ) /
No spaces inside parentheses. (3 places.)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Horizontal_Wh...

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:826: if ( i < histogram_length )
Why not make this part of the loop criterion? Once i >= histogram_length, all
subsequent iterations in the loop will be no-op => might just as well exit the
loop.

[peah-webrtc, 2015-09-14 07:27:20]

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:823: const int histogram_length =
sizeof( self->delay_histogram ) /
On 2015/09/14 07:17:43, hlundin-webrtc wrote:
> No spaces inside parentheses. (3 places.)
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Horizontal_Wh...

Acknowledged.

https://codereview.webrtc.org/1338993003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:826: if ( i < histogram_length )
On 2015/09/14 07:17:43, hlundin-webrtc wrote:
> Why not make this part of the loop criterion? Once i >= histogram_length, all
> subsequent iterations in the loop will be no-op => might just as well exit the
> loop.

Acknowledged.

[hlundin-webrtc, 2015-09-14 07:34:49]

LGTM modulo one more nit.

https://codereview.webrtc.org/1338993003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1338993003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:826: ((i < lookahead +
self->num_partitions) && (i < histogram_length));
You don't need the outer parentheses, and I don't think they improve
readability.

[peah-webrtc, 2015-09-14 07:56:08]

https://codereview.webrtc.org/1338993003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1338993003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:826: ((i < lookahead +
self->num_partitions) && (i < histogram_length));
On 2015/09/14 07:34:48, hlundin-webrtc wrote:
> You don't need the outer parentheses, and I don't think they improve
> readability.

Acknowledged.

[peah-webrtc, 2015-09-14 12:12:55]

Does this look ok? I moved back to using a separate if-statement for the
boundary check.

[hlundin-webrtc, 2015-09-14 12:27:21]

lgtm

[the sun, 2015-09-14 13:32:59]

lgtm

[peah-webrtc, 2015-09-14 13:41:10]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2015-09-14 13:41:20]

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

[commit-bot: I haz the power, 2015-09-14 13:54:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-14 13:54:09]

Patchset 4 (id:??) landed as
https://crrev.com/fc9dd1710d1e9f52846931cabed97134393d0b82
Cr-Commit-Position: refs/heads/master@{#9930}