Issue metadata
Sign in to add a comment
|
Attempting free in buffer_replace |
||||||||||||||||||||||
Issue descriptionDetailed 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.
,
Oct 22 2016
,
Oct 24 2016
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?
,
Oct 24 2016
Tentatively assign to liberato@chromium.org who's scheduled for the next FFmpeg rotation. Feel free to reassign if it's not the case.
,
Oct 26 2016
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
,
Oct 26 2016
i'm trying to repro it locally.
,
Oct 26 2016
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.
,
Oct 26 2016
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.
,
Oct 26 2016
wolenetz: Can you help liberato@ out here as well since you're still working on the roll?
,
Oct 26 2016
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.
,
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.
,
Oct 26 2016
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.
,
Oct 26 2016
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.
,
Oct 26 2016
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.
,
Oct 26 2016
c13: that's a good idea. i'll try that.
,
Oct 27 2016
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.
,
Oct 27 2016
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() ?
,
Oct 27 2016
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.
,
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)
,
Oct 28 2016
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.
,
Oct 28 2016
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.
,
Oct 28 2016
,
Oct 28 2016
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).
,
Oct 28 2016
(Make sure you're not building an msan build since that will force disable-asm)
,
Oct 28 2016
One of your GN flags made the difference, I can now see the failure.
,
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()
,
Oct 28 2016
That's issue 659218 I think.
,
Oct 28 2016
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.
,
Oct 28 2016
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.
,
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
...
,
Oct 28 2016
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.
,
Oct 28 2016
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 :)
,
Oct 28 2016
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.
,
Oct 31 2016
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?
,
Oct 31 2016
Ooh, good catch, we want to do whatever ffmpeg is doing here: https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/utils.c?q=avcodec_align_dimensions2&sq=package:chromium&dr=C&l=566 https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavutil/samplefmt.c?l=117&cl=GROK&gsn=av_samples_get_buffer_size
,
Oct 31 2016
Hmm, but that should be happening implicitly with this step? https://cs.chromium.org/chromium/src/media/filters/ffmpeg_audio_decoder.cc?l=87
,
Oct 31 2016
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.
,
Oct 31 2016
hrm, wait -- upon closer inspection, the docs were just wrong. let me check further.
,
Oct 31 2016
,
Oct 31 2016
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.
,
Oct 31 2016
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?
,
Oct 31 2016
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.
,
Oct 31 2016
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?
,
Oct 31 2016
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.
,
Oct 31 2016
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.
,
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
,
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.
,
Nov 3 2016
ClusterFuzz testcase is verified as fixed, closing issue. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Nov 3 2016
,
Nov 5 2016
,
Nov 5 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 7 2016
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
,
Nov 11 2016
,
Nov 11 2016
,
Nov 29 2016
,
Feb 9 2017
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 |
|||||||||||||||||||||||
Comment 1 by sheriffbot@chromium.org
, Oct 22 2016