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

[Michael Niedermayer]

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


> 
> 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://github.com/google/oss-fuzz/tree/master/projects/ffmpeg

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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- 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/20180611/67d60c68/attachment.sig>