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

Michael Niedermayer michael at niedermayer.cc
Wed Jun 13 01:41:02 EEST 2018


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 at ffmpeg.org] On 
> > > > Behalf Of Michael Niedermayer
> > > > Sent: Saturday, June 9, 2018 9:17 PM
> > > > To: FFmpeg development discussions and patches 
> > > > <ffmpeg-devel at 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...

this is not mandatory but trying with some basic fuzzer seems like a good idea
look at the examples in the manpage of zzuf for example, its very easy to use

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180613/0aa2bd91/attachment.sig>


More information about the ffmpeg-devel mailing list