Prevent OOB reads in FEC packets without complete RED headers.

Prevent OOB reads in FEC packets without complete RED headers.

BUG=webrtc:4771
R=stefan@webrtc.org

Committed: https://crrev.com/2e43b26c78f465d71dfd180d55d04be1b8d4f1fb
Cr-Commit-Position: refs/heads/master@{#9518}

[pbos-webrtc, 2015-06-29 15:26:27]

PTAL

[pbos-webrtc, 2015-06-29 15:26:31]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-29 15:26:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220753003/1

[commit-bot: I haz the power, 2015-06-29 15:34:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 15:34:48]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/8312)

[stefan-webrtc, 2015-06-29 15:35:32]

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right):

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:127: if (payload_data_length
> 4 &&
Shouldn't we bail out if payload_data_length <= 4? Actually, can
payload_data_length even be <= 4 given that you bail out on line 107?

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:133: if (blockLength >
payload_data_length - (REDHeaderLength + 1)) {
Is this +1 any different from the one at line 107? If not it's weird to comment
here and not there.

[pbos-webrtc, 2015-06-29 15:37:05]

sign-comparison

[pbos-webrtc, 2015-06-29 15:37:14]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-29 15:37:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220753003/20001

[pbos-webrtc, 2015-06-29 15:42:22]

feedback + updated comment

[pbos-webrtc, 2015-06-29 15:42:44]

PTAL

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right):

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:127: if (payload_data_length
> 4 &&
On 2015/06/29 15:35:32, stefan-webrtc (holmer) wrote:
> Shouldn't we bail out if payload_data_length <= 4? Actually, can
> payload_data_length even be <= 4 given that you bail out on line 107?

It can't, removed.

https://codereview.webrtc.org/1220753003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:133: if (blockLength >
payload_data_length - (REDHeaderLength + 1)) {
On 2015/06/29 15:35:32, stefan-webrtc (holmer) wrote:
> Is this +1 any different from the one at line 107? If not it's weird to
comment
> here and not there.

Nope, not different. I rewrote this comment to match what we're actually
checking.

[pbos-webrtc, 2015-06-29 16:18:39]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-29 16:18:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220753003/40001

[commit-bot: I haz the power, 2015-06-29 17:56:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 17:56:57]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2015-06-30 08:30:38]

lgtm

[pbos-webrtc, 2015-06-30 08:31:19]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2015-06-30 08:31:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220753003/40001

[commit-bot: I haz the power, 2015-06-30 08:32:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-30 08:32:56]

Patchset 3 (id:??) landed as
https://crrev.com/2e43b26c78f465d71dfd180d55d04be1b8d4f1fb
Cr-Commit-Position: refs/heads/master@{#9518}