Issue 667504: WebRTC UsingFlexibleMode OOB memory write from picture id

UserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce the problem:
The problem is reproducible in our web app while playing and swapping around lots of video. Unfortunately, the repro is not fully deterministic. Though it does almost always crash after a period of several minutes.

However, the bug itself is easily visible by looking at the source code, so nailing down an exact repro (which may be related to a race condition or other randomness) is less a priority for us than just reporting this. Details in the "What went wrong" box.

What is the expected behavior?
Chrome tab doesn't crash while playing video.

What went wrong?
There are several potentials for OOB memory writes in webrtc/modules/video_coding/main/source/decoding_state.cc in VCMDecodingState::SetState, with at least one confirmed in several minidumps.

We suspect this commit introduced the bug: https://chromium.googlesource.com/external/webrtc/+blame/cfc319be1d6afec77bd41eeb70d3e7886dd524db/webrtc/modules/video_coding/main/source/decoding_state.cc#76

The one we have recorded in several minidumps happens at line 93 (different line in master due to other commits since that one) :
 frame_decoded_[frame_index] = true;
Here, frame_index is (uint16_t)0xffff, and 
  bool frame_decoded_[kFrameDecodedLength=(1<<7)];
Tracing back:
 uint16_t frame_index = picture_id_ % kFrameDecodedLength;
picture_id_ is a signed int with value -1 (kNoPictureId). thus the % operation returns -1 and truncates down to 0xffff when converting to uint16_t. Since frame_decoded_ has only 128 entries, this allows out of bounds accesses. The while loop on line 86 may also be able to write large blocks of memory beyond the end of the array. However, all the minidumps we collected are on the above line mentioned.

It seems the problem stems from:

  picture_id_ = frame->PictureId();

which bubbles to the session_info.cc code:

int VCMSessionInfo::PictureId() const {
  if (packets_.empty())
    return kNoPictureId;
  if (packets_.front().video_header.codec == kRtpVideoVp8) {
    return packets_.front().video_header.codecHeader.VP8.pictureId;
  } else if (packets_.front().video_header.codec == kRtpVideoVp9) {
    return packets_.front().video_header.codecHeader.VP9.picture_id;
  } else {
    return kNoPictureId;
  }
}

for which there are many cases where it can return kNoPictureId (-1), but a lack of error(?) checking allows  this -1 value to be used as an unsigned index, allowing OOB memory writes.

Without knowing the code myself, I can't suggest the best bugfix for this, but at the very least, checking for a -1 picture id value as well as making sure frame_decoded_[]'s length is never exceeded, would be a good start.

Did this work before? Yes Before the commit that introduced the bug

Chrome version: 54.0.2840.99  Channel: stable
OS Version: 6.3
Flash Version: 

The minidumps we have are for an internal version of our Electron app, so probably not terribly useful to you without symbols etc, or I would upload the .dmp files. However, it also reproduces in a Chrome 54 browser tab (crashes). We won't be able to provide an external repro website until a later date, but if the explanation given for the code was insufficient, we will try to follow up with that after release dates etc.