This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

dalecurtis@: Can you please have a look at this?

Likely regressed by the following:
https://chromium.googlesource.com/chromium/src/+/1510710c6a4b06430723a6b90f74c09888a0009b

Thanks! Will take a look, that's a pretty old patch, so this affects 61.0.3147.0 or higher if it's that. Don't have a windows box with me today, so will take a look tomorrow.

[Bulk Edit]
URGENT - PTAL.
Your bug is labelled as M61 Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP.

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Thank you.

Hmm, can't repro on Linux sadly, will try to get it reproducing on Windows.

+jzern, is it possible for vpx_codec_decode() + vpx_codec_get_frame() to return the same vpx_image->fb_priv value multiple times? E.g., maybe because an output frame is the same multiple times in a row?

The crash here seems to be that we're getting such a circumstance.

Can confirm that's what's happening. Adding a DCHECK(!frame_buffer->held_by_frame) here fails on Windows (not Linux though):

https://cs.chromium.org/chromium/src/media/filters/vpx_video_decoder.cc?l=357

Full log:
[8876:5732:0810/161258.165:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546950
[8876:5732:0810/161258.168:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.168:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595469F0
[8876:5732:0810/161258.169:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359547120
[8876:5732:0810/161258.169:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595470D0
[8876:5732:0810/161258.189:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359546950
[8876:5732:0810/161258.189:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD610
[8876:5732:0810/161258.190:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD890
[8876:5732:0810/161258.216:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359546B30
[8876:5732:0810/161258.217:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.246:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595469F0
[8876:5732:0810/161258.247:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595469F0
[8876:5732:0810/161258.276:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359547120
[8876:5732:0810/161258.277:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359547120
[8876:5732:0810/161258.306:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595470D0
[8876:5732:0810/161258.338:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359AAD610
[8876:5732:0810/161258.339:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595470D0
[8876:5732:0810/161258.368:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359AAD890
[8876:5732:0810/161258.369:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD610
[8876:5732:0810/161258.398:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359546B30
[8876:5732:0810/161258.399:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.430:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595469F0
[8876:5732:0810/161258.430:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595469F0
[8876:5732:0810/161258.459:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359547120
[8876:5732:0810/161258.459:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359547120
[8876:5732:0810/161258.490:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD890
[8876:5732:0810/161258.519:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595470D0
[8876:5732:0810/161258.520:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595470D0
[8876:5732:0810/161258.550:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359AAD610
[8876:5732:0810/161258.551:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD610
[8876:5732:0810/161258.580:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359546B30
[8876:5732:0810/161258.581:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.609:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595469F0
[8876:5732:0810/161258.610:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595469F0
[8876:5732:0810/161258.640:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359547120
[8876:5732:0810/161258.671:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359AAD890
[8876:5732:0810/161258.671:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359547120
[8876:5732:0810/161258.701:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595470D0
[8876:5732:0810/161258.702:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595470D0
[8876:5732:0810/161258.732:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359AAD610
[8876:5732:0810/161258.733:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359AAD610
[8876:5732:0810/161258.762:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359546B30
[8876:5732:0810/161258.763:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.793:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 00000213595469F0
[8876:5732:0810/161258.794:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 00000213595469F0
[8876:5732:0810/161258.824:ERROR:vpx_video_decoder.cc(427)] OnVideoFrameDestroyed: 0000021359547120
[8876:5732:0810/161258.825:ERROR:vpx_video_decoder.cc(966)] CreateFrame: image: 00000213593A9E88, priv: 0000021359546B30
[8876:5732:0810/161258.825:FATAL:vpx_video_decoder.cc(357)] Check failed: !frame_buffer->held_by_frame.

+some other libvpx folk since jzern@ is ooo.

It's super strange that it happens on Windows but not Linux. My understanding is that the bitstream allows for this (can mark any of the framebuffers in use such as previous frame, alt-ref frame, any of the reference buffers, as "display")

What signal is the framework using to release a framebuffer? Is libvpx explicitly releasing that buffer? Or is it assuming that after it is displayed it can be deallocated?

It looks like this impacts HEAD - we have not made any recent changes to decoder threading. There is a bunch of work tweaking the encoder for realtime and a big recent change on when to decide to re-code a frame, but I believe all the decode changes have been strictly performance related, and pretty much all SIMD speedups (no behavior change expected, just faster)

Possibly it happens on Linux too but we return the frame before the next frame is generated, so it isn't counted as in-use by display.

We keep two booleans, "held by libvpx" and "held by display"

https://cs.chromium.org/chromium/src/media/filters/vpx_video_decoder.cc?l=221

These are separated since libvpx doesn't seem to always clean up its refs on shutdown, so we needed a way to distinguish the two.

In this case libvpx has not called the release function nor have we given this buffer in response to a request for a new buffer (since it's in use both by display and library).

Are you saying it's legit for vpx_codec_get_frame() to return a the same frame buffer multiple times without any intervening new frame buffer requests? If so I'll change the frame out for display bool to a counter.

"These are separated since libvpx doesn't seem to always clean up its refs on shutdown,"
If this is true please file a bug, libvpx should not hold onto any framebuffers after shutdown.

"Are you saying it's legit for vpx_codec_get_frame() to return a the same frame buffer multiple times without any intervening new frame buffer requests?"
In general I don't think this is true.

Are you calling vpx_codec_get_frame() multiple times in a row?

- I'll see if I can track down the test case which has the shutdown issues.
- Not without a vpx_codec_decode() in between.

So if you are calling vpx_codec_decode() in between, then you could definitely get the same frame buffer twice (or more) in a row.

Here is one example of where it could happen:
https://cs.corp.google.com/piper///depot/google3/third_party/libvpx/git_root/vp9/decoder/vp9_decodeframe.c?q=existing+file:%5E//depot/google3/third_party/libvpx/+package:%5Epiper$&dr=C&l=1800

Great, thanks for the clarification. I'll update our code to use a counter here instead. 

[Bulk Edit]
URGENT - PTAL.
Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Thank you!

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label or move to M62.

+awhalley@ (Security TPM)

Hmm, can't repro the refs issue anymore. I've tried all the test cases that should have triggered it and bots seem happy. Will remove that on M62.  issue 737889  was the one that I added the check for but it doesn't repro.

Fix for the security issue here though: https://chromium-review.googlesource.com/c/619699

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7457c49f753270d5f88293fa9403bf40493c7c1e

commit 7457c49f753270d5f88293fa9403bf40493c7c1e
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Sat Aug 19 01:25:03 2017

Allow a frame to be vended multiple times from VpxVideoDecoder.

The old memory pool assumed a frame would only be vended once,
this is not true per the libvpx folk. We need to track an actual
ref_count for frames outstanding.

BUG= 753722 
TEST=new unittest, fuzzer test passes

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Id99f617883a49413553d36d959b6d9e1cb0823c9
Reviewed-on: https://chromium-review.googlesource.com/619699
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495780}
[modify] https://crrev.com/7457c49f753270d5f88293fa9403bf40493c7c1e/media/filters/vpx_video_decoder.cc
[modify] https://crrev.com/7457c49f753270d5f88293fa9403bf40493c7c1e/media/filters/vpx_video_decoder_unittest.cc
[add] https://crrev.com/7457c49f753270d5f88293fa9403bf40493c7c1e/media/test/data/vp9-duplicate-frame.webm

ClusterFuzz has detected this issue as fixed in range 495542:495750.

Detailed report: https://clusterfuzz.com/testcase?key=6344733420158976

Fuzzer: inferno_flicker
Job Type: windows_asan_chrome_no_sandbox
Platform Id: windows

Crash Type: Heap-use-after-free WRITE 1
Crash Address: 0x11a25199
Crash State:
  media::VpxVideoDecoder::MemoryPool::OnVideoFrameDestroyed
  base::internal::Invoker<struct base::internal::BindState<void
  base::debug::TaskAnnotator::RunTask
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=483879:483893
Fixed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=495542:495750

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6344733420158976

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6344733420158976 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

awhalley@ for M61 merge review.

This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

govind@ - good for 61

Approving merge to M61 branch 3163 based on comment #30. Please merge ASAP. Thank you.

Please merge your change to M61 branch 3163 by 4:00 PM PT tomorrow, Friday (08/25) so we can take it in for next week last M61 Beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53af23f6bffedd122ec391c53c3b42797d92da48

commit 53af23f6bffedd122ec391c53c3b42797d92da48
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Aug 24 20:37:11 2017

Merge M61: "Allow a frame to be vended multiple times from VpxVideoDecoder."

The old memory pool assumed a frame would only be vended once,
this is not true per the libvpx folk. We need to track an actual
ref_count for frames outstanding.

BUG= 753722 
TEST=new unittest, fuzzer test passes
TBR=dalecurtis@chromium.org

(cherry picked from commit 7457c49f753270d5f88293fa9403bf40493c7c1e)

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Id99f617883a49413553d36d959b6d9e1cb0823c9
Reviewed-on: https://chromium-review.googlesource.com/619699
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#495780}
Reviewed-on: https://chromium-review.googlesource.com/634247
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#860}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/53af23f6bffedd122ec391c53c3b42797d92da48/media/filters/vpx_video_decoder.cc
[modify] https://crrev.com/53af23f6bffedd122ec391c53c3b42797d92da48/media/filters/vpx_video_decoder_unittest.cc
[add] https://crrev.com/53af23f6bffedd122ec391c53c3b42797d92da48/media/test/data/vp9-duplicate-frame.webm

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot