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

[Eran Kornblau]

> 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...

> thx
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> It is what and why we do it that matters, not just one of them.
> 

Thanks!

Eran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qt-faststart-add-fate-test-for-stco-overflow.patch
Type: application/octet-stream
Size: 3147 bytes
Desc: 0001-qt-faststart-add-fate-test-for-stco-overflow.patch
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180611/e50eda0b/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: faststart-4gb-overflow.mov
Type: video/quicktime
Size: 1301 bytes
Desc: faststart-4gb-overflow.mov
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180611/e50eda0b/attachment.mov>