[FFmpeg-devel] qt-faststart bug near 4GB

On Mon, Jun 11, 2018 at 12:05:20PM +0000, Eran Kornblau wrote:
> > On Sun, Jun 10, 2018 at 01:20:10PM +0000, Eran Kornblau wrote:
> > > > 
> > > > -----Original Message-----
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On 
> > > > Behalf Of Michael Niedermayer
> > > > Sent: Saturday, June 9, 2018 9:17 PM
> > > > To: FFmpeg development discussions and patches 
> > > > <ffmpeg-devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] qt-faststart bug near 4GB
> > > > 
> > > > > +
> > > > > +        if (atom.size < atom.header_size) {
> > > > 
> > > > > +            printf("atom size %"PRIu64" too small\n", atom.size);
> > > > 
> > > > errors should go to stderr if av_log() is not used i see this is not introduced by the patch but was that way before so its more a comment about the code in git than the patch ...
> > > > but ideally this should be fixed in a seperate patch either before 
> > > > or after this one
> > > 
> > > I agree, had the same thought when I wrote the patch, but left it as it was - only the usage error is printed to stderr.
> > > Will submit a patch for this after we finalize the discussion on this one.
> > > 
> > > > 
> > > > some self test for the newly added feature would also be a good idea
> > > 
> > > Since a real MP4 with this problem is going to be large (4GB), I'm 
> > > thinking about taking a small MP4, and patch some stco offset to 
> > > UINT_MAX. Naturally, it will break the file, but faststart won't care - it should still upgrade the stco to co64, and we can just compare the cksum/md5sum of the result to some fixed value.
> > > What do you think?
> > 
> > hmm, thats probably the most practical, yes
> > 
> > you could also simply compress the file a 4gb file thats 99.99% 0 bytes is not large but the problem would be that to test this it would need to be decompressed and the space requirements seems too problematic for fate clients
> > 
> Ok, took the 'fake offset' approach - created the attached mp4 with -
> ffmpeg -f lavfi -i anullsrc=sample_rate=48000 -t 1 faststart-4gb-overflow.mov
> python
> d = file('faststart-4gb-overflow.mov','rb').read()
> p = d.find('stco')
> d = d[:(p+12)] + '\xff' * 4 + d[(p+16):]
> file('faststart-4gb-overflow.mov','wb').write(d)
> 
> I added a fate test for this under 'mov' (that seemed the closest match...).
> For the faststart output, I'm using a temp file, I tried to avoid it with -
> qt-faststart input.mov >(md5sum -)
> But for some reason, this didn't work from 'make fate' (it did work in bash).
> Another option to avoid the temp file, is that I'll add support for passing '-'
> as the qt-faststart output file name, and have it write to stdout. 
> Since all writes there are sequential (no seeks) it should be easy.
> Let me know what you prefer... anyway, current patch + sample file are attached.
> 
> > 
> > > 
> > > Btw, the tests we ran on this change internally are - 1. compared the 
> > > result of the new version to the previous one on more than 1k files 
> > > (incl small files and large >4gb files) and verified the result was 
> > > exactly the same (the edge case this solves is extremely rare, and "normal" files are not expected to change) 2. checked the specific file that had this issue, and verified it was fixed.
> > > 
> > > > 
> > 
> > > > also, was the new code tested with a fuzzer ?
> > > 
> > > No, can you provide some guidance on this - is there some fuzzing framework that you're using?
> > 
> > hmm, you can probably add qt-faststart to:
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgoogle%2Foss-fuzz%2Ftree%2Fmaster%2Fprojects%2Fffmpeg&data=02%7C01%7Ceran.kornblau%40kaltura.com%7C439d2ebbc07940dc5cda08d5cf345a9b%7C0c503748de3f4e2597e26819d53a42b6%7C0%7C0%7C636642746144001154&sdata=8N9HpfHJ6atTGmtwHmr0Vuccw39W7RzMM%2BLw%2F4Ptj0g%3D&reserved=0
> > 
> > this is probably a bit effort though.
> > 
> > using some arbitrary choosen fuzzer, AFL, zzuf or anything else is probably simpler. adding it to oss-fuzz has the advantage that google will in the future automatically do the fuzzing for qt-faststart in ffmpeg.
> > to add it to oss-fuzz you probably would have to submit changes both to the oss-fuzz ffmpeg repo as well as to ffmpeg ...
> > 
> Is this mandatory? :) it may be because I'm not familiar with these frameworks, but indeed sounds like 
> a significant effort to do it...
> 
> > thx
> > 
> > [...]
> > -- 
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > 
> > It is what and why we do it that matters, not just one of them.
> > 
> 
> Thanks!
> 
> Eran

>  Makefile     |    4 +++-
>  fate/mov.mak |    9 ++++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> a5668223451eb818958fa6943af0a50bf5c11a70  0001-qt-faststart-add-fate-test-for-stco-overflow.patch
> From 2ae7cf2839f9bc36cc762da581d16e3eb3adaaec Mon Sep 17 00:00:00 2001
> From: erankor <eran.kornblau@kaltura.com>
> Date: Mon, 11 Jun 2018 14:45:11 +0300
> Subject: [PATCH] qt-faststart: add fate test for stco overflow
> 
> verify that the stco atom is upgraded to co64 when the addition of moov
> size to the offsets results in an overflow
> ---
>  tests/Makefile     | 4 +++-
>  tests/fate/mov.mak | 9 ++++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)

fails on mingw64 + wine

make fate-mov-faststart-4gb-overflow V=2
Test mov-faststart-4gb-overflow failed. Look at tests/data/fate/mov-faststart-4gb-overflow.err for details.
run-detectors: unable to find an interpreter for tools/qt-faststart.exe
md5sum: faststart-4gb-overflow-output.mov: No such file or directory
rm: cannot remove 'faststart-4gb-overflow-output.mov': No such file or directory
make: *** [fate-mov-faststart-4gb-overflow] Error 1

[...]

[FFmpeg-devel] qt-faststart bug near 4GB

Commit Message

Comments

Patch