Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Automatically adding ccs based on OWNERS file / target commit history.

If this is incorrect, please add ClusterFuzz-Wrong label.

Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/68a3b425ac9cc520cefb62a2adf673c8b94d8bb6 (Switch AomVideoDecoder to zero copy now that libaom fixes are in.).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.

+wtc, looks like me might be getting a null fb_priv value. Is that expected?

Dale: I agree we are getting a null img->fb_priv value. I inspected the relevant code in media/filters/aom_video_decoder.cc and media/filters/frame_buffer_pool.cc. It looks correct to me. Specifically, if FrameBufferPool::GetFrameBuffer() had stored a null pointer in *fb_priv, then it would have dereferenced that null pointer earlier and crashed:

uint8_t* FrameBufferPool::GetFrameBuffer(size_t min_size, void** fb_priv) {
  ...
  auto& frame_buffer = *it;

  // Resize the frame buffer if necessary.
  frame_buffer->held_by_library = true;
  if (frame_buffer->data_size < min_size) {
    ...
  }


  // Provide the client with a private identifier.
  *fb_priv = frame_buffer.get();
  return frame_buffer->data.get();
}

I will inspect the aom_codec_get_frame() code and report back.

==================================================

Below is my analysis of the detailed report to show img->fb_priv is null.

Frame #0 in the call stack doesn't have the line number:

==2241==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000024 (pc 0x5591ba97acf5 bp 0x7ffdf77cc4d0 sp 0x7ffdf77cc440 T0)
==2241==The signal is caused by a WRITE memory access.
==2241==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
     #0 0x5591ba97acf4 in media::FrameBufferPool::CreateFrameCallback(void*) media/filters/frame_buffer_pool.cc
     #1 0x5591ba9fb3d3 in media::AomVideoDecoder::DecodeBuffer(media::DecoderBuffer const*) media/filters/aom_video_decoder.cc:282:23

The media::FrameBufferPool::CreateFrameCallback() method is very short:

base::Closure FrameBufferPool::CreateFrameCallback(void* fb_priv) {
  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

  auto* frame_buffer = static_cast<FrameBuffer*>(fb_priv);
  ++frame_buffer->held_by_frame;

  return base::Bind(&FrameBufferPool::OnVideoFrameDestroyed, this,
                    base::SequencedTaskRunnerHandle::Get(), frame_buffer);
}

By inspction, the only WRITE access is the increment of frame_buffer->held_by_frame. The FrameBuffer struct is defined as:

struct FrameBufferPool::FrameBuffer {
  // Not using std::vector<uint8_t> as resize() calls take a really long time
  // for large buffers.
  std::unique_ptr<uint8_t[]> data;
  size_t data_size = 0u;
  std::unique_ptr<uint8_t[]> alpha_data;
  size_t alpha_data_size = 0u;
  bool held_by_library = false;
  // Needs to be a counter since a frame buffer might be used multiple times.
  int held_by_frame = 0;
  base::TimeTicks last_use_time;
};

In a 64-bit build, the held_by_frame field has an offset of 36 (decimal) or 0x24.

Dale: I just realized that this is a fuzzing bug with a reproducer test case. The best way to debug it is to reproduce the crash. Can you reproduce the crash locally?

If you can teach me how to reproduce the crash locally, then I should be able to track it down more efficiently.

oh, just build the media_pipeline_integration_fuzzer target and execute the test case locally and it should reproduce?

This page has some more about the args you need to build:
https://chromium.googlesource.com/chromium/src/testing/libfuzzer/+/HEAD/getting_started.md

I built the fuzzer test program with ASAN Debug as follows:

gn gen out/libfuzzer "--args=use_libfuzzer=true is_asan=true is_debug=true" --check

autoninja -C out/libfuzzer media_pipeline_integration_fuzzer

I then saved the reproducer testcase as the file "reproducer-chromium-906381" and reproduced the bug as follows:

./out/libfuzzer/media_pipeline_integration_fuzzer reproducer-chromium-906381

The first chunk of data passed to aom_codec_decode() causes decode_one() to be called twice.

Note: decoder_decode(), which implements aom_codec_decode(), calls decode_one() in a while loop.

The first decode_one() call successfully decodes an image. (This output image has a frame buffer index of 0.) cm->new_fb_idx is 0.

Two bytes remain in the input buffer for the second decode_one() call. These two bytes are 0x12 0x00. They are decoded as a "temporal delimiter OBU". The second decode_one() call does not produce an output image. At this point, cm->new_fb_idx is 1. Note that this index 1 is actually a "dangling index", because we have already released our reference to the frame buffer at that index.

When we call aom_codec_get_frame(), we get the output image at frame buffer index 0. However, we then copy the fb_priv field from the frame buffer at index cm->new_fb_idx (1). Since we have already released our reference to that frame buffer, its fb_priv is NULL.

I propose the following fix.

1. Set cm->new_fb_idx to INVALID_IDX (-1) after we release our reference to it. This is just defensive programming and does not fix this crash.

2. Get the fb_priv field from the frame buffer of the actual output image, rather than from cm->new_fb_idx.

I will implement this fix in libaom first. Then I will update the libaom in Chromium.

libaom upstream CL:
https://aomedia-review.googlesource.com/c/aom/+/75485

The following revision refers to this bug:
  https://aomedia.googlesource.com/aom/+/26776f2d67fe491787eb84f84148d58fa2de2cd7

commit 26776f2d67fe491787eb84f84148d58fa2de2cd7
Author: Wan-Teh Chang <wtc@google.com>
Date: Sat Nov 24 18:02:23 2018

decoder_get_frame() should use output frame index.

decoder_get_frame() should use pbi->output_frame_index[*index], not
cm->new_fb_idx. After decoding an output frame, if the remaining input
data are valid OBUs but don't contain a frame, cm->new_fb_idx will be
released and not copied to the pbi->output_frame_index[] array.

Tested:
  ./test_libaom --gtest_filter=*InvalidFileTest*
  ./test_libaom --gtest_filter=*ExternalFrameBuffer*
  ./test_libaom --gtest_filter=*TestVectorTest*

BUG= chromium:906381 

Change-Id: Id20f8d25fe8809445d917c6822a0edd9a132483a

[modify] https://crrev.com/26776f2d67fe491787eb84f84148d58fa2de2cd7/test/test_data_util.cmake
[modify] https://crrev.com/26776f2d67fe491787eb84f84148d58fa2de2cd7/av1/av1_dx_iface.c
[modify] https://crrev.com/26776f2d67fe491787eb84f84148d58fa2de2cd7/test/invalid_file_test.cc
[modify] https://crrev.com/26776f2d67fe491787eb84f84148d58fa2de2cd7/test/test-data.sha1

The following revision refers to this bug:
  https://aomedia.googlesource.com/aom/+/67223a304e098da309776bac253456f75f613fc2

commit 67223a304e098da309776bac253456f75f613fc2
Author: Wan-Teh Chang <wtc@google.com>
Date: Mon Nov 26 22:15:37 2018

decoder_get_frame() should use output frame index.

decoder_get_frame() should use pbi->output_frame_index[*index], not
cm->new_fb_idx. After decoding an output frame, if the remaining input
data are valid OBUs but don't contain a frame, cm->new_fb_idx will be
released and not copied to the pbi->output_frame_index[] array.

Tested:
  ./test_libaom --gtest_filter=*InvalidFileTest*
  ./test_libaom --gtest_filter=*ExternalFrameBuffer*
  ./test_libaom --gtest_filter=*TestVectorTest*

BUG= chromium:906381 

Change-Id: Id20f8d25fe8809445d917c6822a0edd9a132483a
(cherry picked from commit 26776f2d67fe491787eb84f84148d58fa2de2cd7)

[modify] https://crrev.com/67223a304e098da309776bac253456f75f613fc2/test/test_data_util.cmake
[modify] https://crrev.com/67223a304e098da309776bac253456f75f613fc2/av1/av1_dx_iface.c
[modify] https://crrev.com/67223a304e098da309776bac253456f75f613fc2/test/invalid_file_test.cc
[modify] https://crrev.com/67223a304e098da309776bac253456f75f613fc2/test/test-data.sha1

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

commit d0bc58de5b9102540f11c6f144eabe2a77fc0ffe
Author: Wan-Teh Chang <wtc@google.com>
Date: Tue Nov 27 20:19:05 2018

Roll src/third_party/libaom/source/libaom/ 716f2896c..67223a304 (2 commits)

https://aomedia.googlesource.com/aom.git/+log/716f2896c6ba..67223a304e09

$ git log 716f2896c..67223a304 --date=short --no-merges --format='%ad %ae %s'
2018-11-20 wtc decoder_get_frame() should use output frame index.
2018-11-21 wtc Declare pbi->output_frame_index as an array of int

Created with:
  roll-dep src/third_party/libaom/source/libaom
R=tomfinegan@chromium.org,johannkoenig@google.com,jzern@chromium.org

Bug:  906381 
Change-Id: I8f80378c1608761310a273ecdb3556d401e71cc2
Reviewed-on: https://chromium-review.googlesource.com/c/1351715
Reviewed-by: James Zern <jzern@google.com>
Commit-Queue: Wan-Teh Chang <wtc@google.com>
Cr-Commit-Position: refs/heads/master@{#611282}
[modify] https://crrev.com/d0bc58de5b9102540f11c6f144eabe2a77fc0ffe/DEPS
[modify] https://crrev.com/d0bc58de5b9102540f11c6f144eabe2a77fc0ffe/third_party/libaom/README.chromium
[modify] https://crrev.com/d0bc58de5b9102540f11c6f144eabe2a77fc0ffe/third_party/libaom/source/config/config/aom_version.h

ClusterFuzz has detected this issue as fixed in range 611252:611288.

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

Fuzzer: afl_media_pipeline_integration_fuzzer
Job Type: afl_chrome_asan
Platform Id: linux

Crash Type: Null-dereference WRITE
Crash Address: 0x000000000024
Crash State:
  media::FrameBufferPool::CreateFrameCallback
  media::AomVideoDecoder::DecodeBuffer
  media::AomVideoDecoder::Decode
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=605117:605131
Fixed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=611252:611288

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

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md 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 6308266000515072 is verified as fixed, so closing issue as verified.

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