New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 658440 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 591845



Sign in to add a comment

Attempting free in buffer_replace

Project Member Reported by ClusterFuzz, Oct 21 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6274608317857792

Fuzzer: libfuzzer_radamsa_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Attempting free
Crash Address: add
Crash State:
  buffer_replace
  av_frame_unref
  media::FFmpegAudioDecoder::FFmpegDecode
  

Minimized Testcase (0.10 Kb): https://cluster-fuzz.appspot.com/download/AMIfv953az1Ulw5p8E1Qfq4fE4snBxn3LMBrZkSKvfxA68OUer_BpwERui-5DowHirfpufqDDCnMoFcXLog8MOILA-2RqThxPvb_Yey2OQLuTj7JacIJyv4E9dTa4iOcAvxnCL2vAF6tyamDeouXRx7OS1zSfKThMQ?testcase_id=6274608317857792

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Oct 22 2016

Labels: M-54
Project Member

Comment 2 by sheriffbot@chromium.org, Oct 22 2016

Labels: Pri-1
Components: Internals>Media
Labels: -OS-Linux OS-All
Owner: xhw...@chromium.org
Status: Assigned (was: Untriaged)
xhwang: I'm having a bit of trouble figuring out what might be happening here. Would you mind helping us find an owner for this one?

Comment 4 by xhw...@chromium.org, Oct 24 2016

Cc: xhw...@chromium.org wolenetz@chromium.org
Owner: liber...@chromium.org
Tentatively assign to liberato@chromium.org who's scheduled for the next FFmpeg rotation. Feel free to reassign if it's not the case.

Comment 5 by aarya@google.com, Oct 26 2016

Cc: och...@chromium.org mmoroz@chromium.org infe...@chromium.org kcc@chromium.org dalecur...@chromium.org aizatsky@chromium.org
Labels: -Pri-1 Pri-0
This is blocking per-day corpus merging/pruning as well. Here is the stack. Liberato@, can you please take a look soon.

=================================================================
==27639==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x60500001c120 in thread T0
SCARINESS: 40 (bad-free)
#0 0x4d88bb in __interceptor_free (/mnt/scratch0/clusterfuzz/slave-bot/builds/chromium-browser-libfuzzer_linux-release-asan_ae530a86793cd6b8b56ce9af9159ac101396e802/revisions/libfuzzer-linux-release-427511/media_pipeline_integration_fuzzer+0x4d88bb)
#1 0x1722915 in buffer_replace third_party/ffmpeg/libavutil/buffer.c:116:9
#2 0x1728412 in av_frame_unref third_party/ffmpeg/libavutil/frame.c:483:9
#3 0x901156 in media::FFmpegAudioDecoder::FFmpegDecode(scoped_refptr<media::DecoderBuffer> const&, bool*) media/filters/ffmpeg_audio_decoder.cc:362:7
#4 0x8fff55 in media::FFmpegAudioDecoder::DecodeBuffer(scoped_refptr<media::DecoderBuffer> const&, base::Callback<void (media::DecodeStatus), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) media/filters/ffmpeg_audio_decoder.cc:237:10
#5 0x8ff832 in media::FFmpegAudioDecoder::Decode(scoped_refptr<media::DecoderBuffer> const&, base::Callback<void (media::DecodeStatus), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) media/filters/ffmpeg_audio_decoder.cc:205:3
#6 0x9876e0 in media::DecoderStream<(media::DemuxerStream::Type)1>::DecodeInternal(scoped_refptr<media::DecoderBuffer> const&) media/filters/decoder_stream.cc:382:13
#7 0x987131 in media::DecoderStream<(media::DemuxerStream::Type)1>::Decode(scoped_refptr<media::DecoderBuffer> const&) media/filters/decoder_stream.cc:354:5
#8 0x98982e in media::DecoderStream<(media::DemuxerStream::Type)1>::OnBufferReady(media::DemuxerStream::Status, scoped_refptr<media::DecoderBuffer> const&) media/filters/decoder_stream.cc:678:3
#9 0x909e00 in base::internal::RunMixin<base::Callback<void (media::DemuxerStream::Status, scoped_refptr<media::DecoderBuffer> const&), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> >::Run(media::DemuxerStream::Status, scoped_refptr<media::DecoderBuffer> const&) const base/callback.h:64:12
#10 0x85f820 in base::internal::RunMixin<base::Callback<void (), (base::internal::CopyMode)0, (base::internal::RepeatMode)0> >::Run() && base/callback.h:47:12
#11 0x85f30e in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:52:33
#12 0x748694 in base::MessageLoop::RunTask(base::PendingTask*) base/message_loop/message_loop.cc:413:19
#13 0x7490af in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) base/message_loop/message_loop.cc:422:5
#14 0x749cca in base::MessageLoop::DoWork() base/message_loop/message_loop.cc:515:13
#15 0x756786 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_default.cc:35:31
#16 0x747f4b in base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:378:10
#17 0x793821 in base::RunLoop::Run() base/run_loop.cc:35:10
#18 0x50d829 in media::PipelineIntegrationTestBase::Seek(base::TimeDelta) media/test/pipeline_integration_test_base.cc:214:19
#19 0x50543a in LLVMFuzzerTestOneInput media/test/pipeline_integration_fuzzertest.cc:58:8
#20 0x55f852 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) third_party/libFuzzer/src/FuzzerLoop.cpp:516:13
#21 0x55fd0f in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) third_party/libFuzzer/src/FuzzerLoop.cpp:468:3
#22 0x560cc2 in fuzzer::Fuzzer::FindExtraUnits(std::__1::vector<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::allocator<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > > const&, std::__1::vector<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::allocator<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > > const&) third_party/libFuzzer/src/FuzzerLoop.cpp:608:11
#23 0x56160e in fuzzer::Fuzzer::Merge(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) third_party/libFuzzer/src/FuzzerLoop.cpp:647:14
#24 0x549b5e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) third_party/libFuzzer/src/FuzzerDriver.cpp:497:7
#25 0x565a58 in main third_party/libFuzzer/src/FuzzerMain.cpp:20:10
#26 0x7f0e30ae1ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

Address 0x60500001c120 is a wild pointer.
SUMMARY: AddressSanitizer: bad-free (/mnt/scratch0/clusterfuzz/slave-bot/builds/chromium-browser-libfuzzer_linux-release-asan_ae530a86793cd6b8b56ce9af9159ac101396e802/revisions/libfuzzer-linux-release-427511/media_pipeline_integration_fuzzer+0x4d88bb) in __interceptor_free
==27639==ABORTING
***
***
***
*** NOTE: merge did not succeed due to a failure on one of the inputs.
*** You will need to filter out crashes from the corpus, e.g. like this:
*** for f in WITH_CRASHES/*; do ./fuzzer $f && cp $f NO_CRASHES; done
*** Future versions may have crash-resistant merge, stay tuned.
***
***
***
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0xff,0xf9,0x72,0x72,0x72,0x0,0x0,0xe8,0x0,0xf1,0x40,0x0,0x0,0x0,0x64,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x80,0x49,0x44,0x4a,0x33,0x2,0x7,0x14,0x29,0x16,0x5c,0x4e,0x0,0x0,0x64,0x0,0x0,0x80,0x49,0x44,0x4a,0x33,0x2,0x7,0x4e,0x4e,0x7f,0x46,0x4c,0x41,0x43,
\xff\xf9rrr\x00\x00\xe8\x00\xf1@\x00\x00\x00d\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80IDJ3\x02\x07\x14)\x16\\N\x00\x00d\x00\x00\x80IDJ3\x02\x07NN\x7fFLAC
artifact_prefix='./'; Test unit written to ./crash-e4e0bbb4dc081cef4fa3743d854ea32b5faa1d44
Base64: //lycnIAAOgA8UAAAABkAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgElESjMCBxQpFlxOAABkAACASURKMwIHTk5/RkxBQw==

Also, this fuzzer is logging really noisy, e.g. is there a way to suppress there.
Actual: never called - unsatisfied and active
../../media/test/pipeline_integration_test_base.cc:144: Failure
Mock function called more times than expected - returning directly.
Function call: OnDurationChange()
Expected: to be called at most once
Actual: called twice - over-saturated and active
../../media/test/pipeline_integration_test_base.cc:69: Failure
Value of: pipeline_->GetMediaTime()
Actual: 0.001267s
Expected: seek_time
Which is: 0s
i'm trying to repro it locally.
Status: Started (was: Assigned)
turns out when i actually read the repro instructions, things get a lot easier.  i can now repro it.

tracing how that random pointer get put into AVFrame leads me out of ffmpeg and back into chrome (media::GetAudioBuffer), so maybe it's our bug after all.  still tracing.
Labels: -Pri-0 Pri-1
This is not Pri-0, it's nonsensical to make a longstanding but recently found issue Pri-0 :) If it's suddenly blocking something you need to resolve that on your side. We'll work at this with due speed of course, but it's not Pri-0.
wolenetz: Can you help liberato@ out here as well since you're still working on the roll?
Blocking: 591845
liberato@: if you can't nail down this repro to something wholly in Chromium, I'll take a look once I get a build working locally with updated ffmpeg in chromium to see if it still repros. ETA this week for such an attempt.

Comment 11 by aarya@google.com, Oct 26 2016

Sorry Dale about Pri-0 here, should have left Pri-1. This does not have workaround on our side, but we can wait this week for fix.
interesting stuff -- turns out that the problem doesn't seem like it's in chrome code after all.  during decoding of flac audio, ffmpeg calls this function called 'decorrelate()' to do the actual work of converting an array of input data into an array of usable output samples.  calling it causes the asan crash.

it's not a buffer overrun, either, than i can tell.  i took out the original call, and replaced it with one that uses fake output buffers that have large guard regions, and nothing is touched that's out of place.  however, the crash remains the same.

further, if i free() the memory before decorrelate(), then the free succeeds.  if i free immediately after, then the asan error shows up on that free instead.  somehow, decorrelate() is messing up the free list.  given how free lists are usually structured, it's very unexpected.

the function itself is an asm-only implementation (flacdsp.asm), ff_flac_decorrelate_indep8_16_avx that is still pretty opaque to me.  i've made the asm early-out to find what part of it is triggering the crash, but until i figure out what the function is trying to do, that information doesn't really help me very much.  :)

it hasn't been modified lately (aug 2015), and won't change in matt's upcoming ffmpeg roll either, from what github tells me.

i'll keep looking.
I'd just try building a standalone ffmpeg instance with asan enabled and see if ffplay repros this. You can then send it on to Michael from ffmpeg if so then.
one other thing -- if i turn off the x86 asm version, in favor of the c implementation, things work as expected.  i suspect "do that and file a bug against ffmpeg" will be my proposed solution, unless i get better quite suddenly at deciphering the asm impl.
c13: that's a good idea.  i'll try that.
standalone ffplay doesn't play the file at all.  instead, i tried transcoding with ffmpeg.  i tried both asan and valgrind.

however, i just noticed that the fuzzer test fails on the second iteration, not the first.  same data both times.  adding "for(.. i < 2..)" to the fuzzer main has the same effect -- crash on the second loop iteration.

trying the same with "ffmpeg -f concat", listing the fuzzer input twice, works fine in valgrind and with asan.  tried both the current ffmpeg from github, and the most recent shared commit with chrome.

will check out the chrome-specific commits to see if anything's interesting there.
Hmm, this bug is weird, ASAN claims the address isn't from malloc(), but that's not true:

malloc()'d buffer at: 0x605000000240
[1027/151220:ERROR:ffmpeg_audio_decoder.cc(134)] Provided: 0x605000000240
[1027/151220:ERROR:ffmpeg_audio_decoder.cc(302)] Decoded frame...
=================================================================
==25715==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x605000000240 in thread T0

What logic does ASAN use to determine if this address is malloc'd? This file does not crash normally. Add we frequently deref the pointer address prior to this point -- so it seems like if this was truly a bad address, ASAN would have complained earlier? Is asan missing this malloc since it's provided by posix_memalign() ?
Cc: liber...@chromium.org
Owner: infe...@chromium.org
Investigated a bit more and this is definitely a valid address to free() (that has not been free'd before), posix_memalign returns this address a few calls prior.

inferno@ can you help find the right owner for this? It seems like this is an ASAN bug.

Comment 19 by kcc@chromium.org, Oct 28 2016

Couldn't reproduce from the first attempt. dalecurtis@, any hint? 
asan supports posix_memalign and a bug in this place would be most surprising
(still possible, of course) 
c#17: yes, the address is really returned from a posix_memalign, which should be freeable by free (at least on linux).

i've narrowed it down to a loop inside the x86 asm impl for flac decoding that, if i execute it, fails.  i can't log in right now to provide the exact line, but it's in libavcodec/x86/flacdsp.asm .  i can send it when i'm back at my desk.

if one returns early before that magic loop, then one can free() the ptr (immediately) with no problem, until asan gets mad (correctly) about using memory after free, or double free, or some such.  if one returns from the asm routine after, and immediately calls free(), then asan gets mad about it.

note that it's extra weird in that it's not a normal buffer overrun -- i can trigger the failure even if i provide an output buffer with very large guard ranges.
from libavcodec/x86/flacdsp.asm, near line 284:

%assign %%i 0                                                                   
%rep REPCOUNT                                                                   
    mova [outq + %%i*mmsize], m %+ %%i                                      
%assign %%i %%i+1                                                               
%endrep   

comment out the movea, and it doesn't fail.  haven't gotten gdb to let me break there yet to inspect the target addresses.
Cc: -xhw...@chromium.org
kcc@ You mean you are not able to repro the asan crash at all? I'm building with these flags and using the minimized test case downloaded from the bug.

$ export GN_DEFINES='use_libfuzzer=true is_asan=true is_component_build=true ffmpeg_branding="ChromeOS" proprietary_codecs=true enable_nacl=false dcheck_always_on=true use_goma=true is_clang=true symbol_level=1 is_debug=false'

$ ninja -C out/Release media_pipeline_integration_fuzzer -j 2000

$ ./out/Release/media_pipeline_integration_fuzzer ~/Downloads/fuzz-3-media_pipeline_integration_fuzzer

I then added logging to av_buffer_create() and buffer_replace() in third_party/ffmpeg/libavutil/buffer.c followed by some logging to av_malloc() and av_freep() in third_party/ffmpeg/libavutil/mem.c to be sure.

It's possible the asm code Frank points out is stomping on some ASAN memory, but I guess I'd expect ASAN to complain at that point instead? I forget if ASAN is properly able to verify externally compiled code (YASM in this case).
(Make sure you're not building an msan build since that will force disable-asm)

Comment 25 by kcc@chromium.org, Oct 28 2016

One of your GN flags made the difference, I can now see the failure. 

Comment 26 by kcc@chromium.org, Oct 28 2016

there is also a check failure that the fuzzer finds w/o initial corpus in ~ 1 sec:

[1028/111819:FATAL:audio_timestamp_validator.cc(45)] Check failed: kNoTimestamp != buffer->timestamp() (-9.22337e+12s vs. -9.22337e+12s)
#0 0x00000046d7b1 __interceptor_backtrace
#1 0x7f591ca2e7f6 base::debug::StackTrace::StackTrace()
#2 0x7f591cae3117 logging::LogMessage::~LogMessage()
#3 0x7f591b8d0112 media::AudioTimestampValidator::CheckForTimestampGap()
#4 0x7f591b939fb4 media::DecoderStream<>::DecodeInternal()
#5 0x7f591b939255 media::DecoderStream<>::Decode()
#6 0x7f591b94017f media::DecoderStream<>::OnBufferReady()

That's  issue 659218  I think.
i unrolled the loop in c#21.  it's the fourth and final write that amounts to:

mova destination addr = [allocated buffer + 48], src addr = (some source data).

i'll check if 48 bytes is part of the allocation or what.
The allocation ASAN is complaining about is one for the AVBufferRef() and should have no relation to where the flac code is writing. I'd expect the flac code is writing into the AVFrame->data[] output vectors or some other scratch space.

Comment 30 by kcc@chromium.org, Oct 28 2016

This is is fun!!! 
I claim that this is a buffer overflow in the assembly code. 

Sequence of events: 

Allocating 24 bytes here, the result is 0x605000000240

    #5 0x7f0bacd5100a in av_malloc third_party/ffmpeg/libavutil/mem.c:98:9
    #6 0x7f0bacd5100a in av_mallocz third_party/ffmpeg/libavutil/mem.c:255
    #7 0x7f0bacd06f4c in av_buffer_create third_party/ffmpeg/libavutil/buffer.c:48:11
    #8 0x7f0bb4ca958d in media::GetAudioBuffer(AVCodecContext*, AVFrame*, int) media/filters/ffmpeg_audio_decoder.cc:132:19
    #9 0x7f0bacba5a0d in get_buffer_internal third_party/ffmpeg/libavcodec/utils.c:883:11

Trying to deallocate that pointer here: 
    #4 0x7f0bacd07892 in buffer_replace third_party/ffmpeg/libavutil/buffer.c:116:9
    #5 0x7f0bacd07892 in av_buffer_unref third_party/ffmpeg/libavutil/buffer.c:129
    #6 0x7f0bacd36a80 in av_frame_unref third_party/ffmpeg/libavutil/frame.c:483:9

asan complains that this address is not know to it.

asan uses 16 bytes left to the pointer to store its header and when free() is called 
asan checks that's there. If it does not see what it expects to see it complains.
The header might be corrupted by a buffer overflow, but hey, this is asan -- it catches buffer overflows as they happen. 
.. but only if they happen in instrumented code. 

Now, let's run under gdb:

gdb --args ./out/media/media_pipeline_integration_fuzzer  ~/Downloads/fuzz-3-media_pipeline_integration_fuzzer

(gdb) watch *(long*)(0x605000000240 - 8)
Hardware watchpoint 1: *(long*)(0x605000000240 - 8)
(gdb) r

First time we touch this memory in malloc. 
But then, boom: 


#0  0x00007fffef29c26e in ff_flac_decorrelate_indep8_16_avx () from /usr/local/google/home/kcc/chromium/src/out/media/./libffmpeg.so
#1  0x0000000000000008 in ?? ()
#2  0x00007fffef045949 in flac_decode_frame () at ../../third_party/ffmpeg/libavcodec/flacdec.c:608
#3  0x00007fffee8a5088 in avcodec_decode_audio4 () at ../../third_party/ffmpeg/libavcodec/utils.c:2241
#4  0x00007ffff699421e in FFmpegDecode () at ../../media/filters/ffmpeg_audio_decoder.cc:272
#5  0x00007ffff69921f9 in DecodeBuffer () at ../../media/filters/ffmpeg_audio_decoder.cc:237
#6  0x00007ffff6991343 in Decode () at ../../media/filters/ffmpeg_audio_decoder.cc:205
#7  0x00007ffff67358b3 in DecodeInternal () at ../../media/filters/decoder_stream.cc:382
#8  0x00007ffff6734255 in Decode () at ../../media/filters/decoder_stream.cc:354
#9  0x00007ffff673b17f in OnBufferReady () at ../../media/filters/decoder_stream.cc:678


This is a function apparently written in assembly, so asan does not catch this buffer overflow: 

Dump of assembler code for function ff_flac_decorrelate_indep8_16_avx:
   0x00007fffef29c1b0 <+0>:	push   %rbx
   0x00007fffef29c1b1 <+1>:	vmovd  %r8d,%xmm5
   0x00007fffef29c1b6 <+6>:	mov    0x8(%rsi),%rdx
   0x00007fffef29c264 <+180>:	vmovdqa %xmm3,0x20(%rdi)
   0x00007fffef29c269 <+185>:	vmovdqa %xmm4,0x30(%rdi)
=> 0x00007fffef29c26e <+190>:	add    $0x10,%rsi
   0x00007fffef29c272 <+194>:	add    $0x40,%rdi

...






Cc: -liber...@chromium.org
Owner: liber...@chromium.org
Great, thanks for the help!

liberato@ I'd check to see if the buffer given to decorrelate() is properly 32-byte padded. It's possible this is fixed in more recent ffmpeg builds which is why we don't see it with your local build.
liberato@ try repo with an ffplay built from our current downstream chromium ffpmpeg. If it repros there, but not in current upstream ffmpeg, then yes, the roll should fix this. Extra points if you can bisect/figure out which upstream commit fixed this :)
c#29: i don't follow what the concern is.  we don't know where dst+48 is with respect to the memory that's failing to be freed; presumably, it overlaps.

c32: unfortunately, i tried reverting to the most common commit between chromium and upstream ffmpeg, but no repro in either case.  did this shortly after c#16 yesterday.

c30: 16 bytes prior -- excellent!  thanks!

happily, that disassembly looks like the manually unrolled loop in c#28.  the two following add instructions line up, at least, and i don't think that they're elsewhere in that fn.

AudioBuffer::AudioBuffer doesn't seem to pad for interleaved sample formats, only planar. https://cs.chromium.org/chromium/src/media/base/audio_buffer.cc?rcl=0&l=84

it seems okay to me to add the padding there, too.  dalecurtis@ -- is this a sane thing to do?
Hmm, but that should be happening implicitly with this step?

https://cs.chromium.org/chromium/src/media/filters/ffmpeg_audio_decoder.cc?l=87
perhaps we're calling it incorrectly

// align == 0 causes alignment, align !=0 causes no alignment.
// that is confusing on several levels :)
int av_samples_get_buffer_size(int *linesize, int nb_channels, int nb_samples,  
                               enum AVSampleFormat sample_fmt, int align)

we're calling it with kChannelAlignment, which is 32.
hrm, wait -- upon closer inspection, the docs were just wrong.  let me check further.
Cc: chcunningham@chromium.org
the asm code is assuming that it has 64 bytes of space in which to output, not 16. in particular, the 0x30 in the following is the start of a 16 byte write:

0x00007fffef29c269 <+185>:	vmovdqa %xmm4,0x30(%rdi)

changing kChannelAlignment to 64 avoids the crash.  however, we don't need 64 byte alignment in this case -- we just need a buffer that's an even multiple of 64 bytes long.  av_samples_get_buffer_size() seems to think of 'align' as the the amount of padding, while everything else (correctly) considers it to be an even divisor of the base address.

sure enough, if i just cal av_samples_get_buffer_size(...., 64) rather than , kChannelAlignment, it works.

that makes sense, including c#36.
Hmm, update_frame_pool() is calling av_samples_get_buffer_size() with an alignment of zero in the default case. Since this doesn't repro with ffplay they must be allocating a sufficiently sized buffer here (or getting lucky in that they already have a buffer in the pool > the size needed). What happens in our code if you request an alignment of zero?
it pads the number of samples to a multiple of 32 before computing the buffer size.  when align is nonzero, it pads the total allocation to match instead.

we're using 8 channels and 2 bytes per sample, and requesting one sample.

so, for align == 0, we get (1 rounded up to 32) samples * 16 => 512 bytes total.

align == 32 => 1 sample * 16 => (16 rounded up to 32) bytes
align == 64 => 1 sample * 16 => (16 rounded up to 64) bytes.

align==0 probably makes more sense.
Oof, an 8x increase (64 -> 512) is kind of crazy. I'm still surprised it's expecting 64 bytes of space to write into. Those are only 256bit instructions right? Where are you getting the 64 byte number from?
Oh wait, nvm, 512 makes sense in this case since this is 8 channels. Changing align=0 sgtm, especially since that's what ffmpeg is already doing. For common codecs like aac and opus this should be a no-op since default sample sizes are already powers of 2.
they're 128 bit instructions, though they might be as big as 256 for avx.  however, the destination offset per loop goes up by 16 bytes, so i suspect that's what's intended.

i get 64 because there's a 4x unrolled loop that moves 16 bytes forward each time, ending in this one:

0x00007fffef29c269 <+185>:	vmovdqa %xmm4,0x30(%rdi)

where rdi == data[0].  definitely writing to data[0]+48, and i think it's 16 bytes.

i should probably check the input buffer size, too.

personally, i don't think that the 8x increase is that big of a deal.  it's only going to at most add 31 samples.  we just happened to ask for one sample.
Project Member

Comment 46 by bugdroid1@chromium.org, Nov 1 2016

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

commit b8733772baf52a693ab6a887373feaa6b91c7cdd
Author: liberato <liberato@chromium.org>
Date: Tue Nov 01 16:22:56 2016

Use FFmpeg default padding when allocating audio buffers.

Previously, we would pad to the alignment size when allocating an
audio buffer.  However, this did not always provide enough padding
to avoid overruns.

Instead, we now let FFmpeg determine the correct padding for us.

BUG= 658440 

Review-Url: https://codereview.chromium.org/2464963003
Cr-Commit-Position: refs/heads/master@{#429014}

[modify] https://crrev.com/b8733772baf52a693ab6a887373feaa6b91c7cdd/media/filters/ffmpeg_audio_decoder.cc

Project Member

Comment 47 by ClusterFuzz, Nov 3 2016

ClusterFuzz has detected this issue as fixed in range 428862:429212.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6274608317857792

Fuzzer: libfuzzer_radamsa_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Attempting free
Crash Address: add
Crash State:
  buffer_replace
  av_frame_unref
  media::FFmpegAudioDecoder::FFmpegDecode
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=428862:429212

Minimized Testcase (0.10 Kb): https://cluster-fuzz.appspot.com/download/AMIfv953az1Ulw5p8E1Qfq4fE4snBxn3LMBrZkSKvfxA68OUer_BpwERui-5DowHirfpufqDDCnMoFcXLog8MOILA-2RqThxPvb_Yey2OQLuTj7JacIJyv4E9dTa4iOcAvxnCL2vAF6tyamDeouXRx7OS1zSfKThMQ?testcase_id=6274608317857792

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

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

Comment 48 by ClusterFuzz, Nov 3 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase is verified as fixed, closing issue.

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

Comment 49 by sheriffbot@chromium.org, Nov 3 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 50 by sheriffbot@chromium.org, Nov 5 2016

Labels: Merge-Request-55

Comment 51 by dimu@chromium.org, Nov 5 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 52 by bugdroid1@chromium.org, Nov 7 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a4696cdc766320ab841bb1ad4ef7c4665e6431e

commit 6a4696cdc766320ab841bb1ad4ef7c4665e6431e
Author: liberato@chromium.org <liberato@chromium.org>
Date: Mon Nov 07 19:39:13 2016

[M55] Use FFmpeg default padding when allocating audio buffers.

Previously, we would pad to the alignment size when allocating an
audio buffer.  However, this did not always provide enough padding
to avoid overruns.

Instead, we now let FFmpeg determine the correct padding for us.

BUG= 658440 

Review-Url: https://codereview.chromium.org/2464963003
Cr-Commit-Position: refs/heads/master@{#429014}
(cherry picked from commit b8733772baf52a693ab6a887373feaa6b91c7cdd)

Review URL: https://codereview.chromium.org/2481913002 .

Cr-Commit-Position: refs/branch-heads/2883@{#480}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/6a4696cdc766320ab841bb1ad4ef7c4665e6431e/media/filters/ffmpeg_audio_decoder.cc

Labels: M-55
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M55
Project Member

Comment 56 by sheriffbot@chromium.org, Feb 9 2017

Labels: -Restrict-View-SecurityNotify allpublic
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

Sign in to add a comment