From patchwork Tue Jun 12 21:25:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Niedermayer X-Patchwork-Id: 9381 Delivered-To: ffmpegpatchwork@gmail.com Received: by 2002:a02:11c:0:0:0:0:0 with SMTP id c28-v6csp5951946jad; Tue, 12 Jun 2018 14:25:58 -0700 (PDT) X-Google-Smtp-Source: ADUXVKIz7hqG27du2j4F2eo+loYNiAp0YoYGM04Hx0vwICW0BBeuUFAvLzvxHYyAXzB7ObGl1LgQ X-Received: by 2002:a1c:5f0b:: with SMTP id t11-v6mr1619681wmb.23.1528838757927; Tue, 12 Jun 2018 14:25:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1528838757; cv=none; d=google.com; s=arc-20160816; b=g7zmZIcriIpsSyjkX81RKl2KsPOwEV+8AfdhJTKMdwp1jC9kMdjsrWbxKNUmcxfJaV l/0DSCAdjfkimDJTAaJ1udVIuLvJEt1R/1fk/1PJrABQB+s8grr7zyIDSltSv9nKkUi4 tr2+E2u6JNr6aM/mw9OGIBdOj0gWZTDykyo6ZSWkfUxb/3Yf69oGll2JiFQ0QQGtCY1m DnVNkPPJxQRJS4ixYvhz/3+0qf+Z5PLnD8e9eysW1diorIDQCmXMYZYwBvJGI23wiBy5 1LUVDqXd0bKarJD6FRHnhCDiwOnYB8eh5kj+HUXYJiRC984wkdxTLin3+dDyZvj2W4X9 Xy/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:user-agent :in-reply-to:mime-version:references:message-id:to:from:date :delivered-to:arc-authentication-results; bh=C8TMLxJDmLbSK6+dv78tPTWd+SxvMwyq3q63NuZAnTk=; b=iXu+TUGZBsyKktplglZAA/oNjppw1jHQFIaIXnAiBZ1O8U8GQmiVyEzjMi6OwjqitI tpWTElauWxWLJOz9Ej45wGlDn93kb4r6YTgAStpv8K8L3mE5HyUrZKYjF7URIAuwbUxd qml5XjutwYDnkU0MRu04XWTpg68ZxelTtglYDthWgpbtX7u4KsCwNpXPxn7ibdD9faLT EVPOiYz2fho40a0RpHZo8zKgZ4dKmB3HQSiQvMFri4VLrNFG81Rh4fAAEcp6q9CJmrel aIWbUZ806MPudruG5UuuUuvMmfu9i03s0OslYZaaz0RYX0FGg4HvaFOLmoUnadbmWg+N fw+g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id h2-v6si879104wmg.141.2018.06.12.14.25.57; Tue, 12 Jun 2018 14:25:57 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D26C168AFFE; Wed, 13 Jun 2018 00:25:05 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D251D68AB39 for ; Wed, 13 Jun 2018 00:24:59 +0300 (EEST) X-Originating-IP: 213.47.41.20 Received: from localhost (213-47-41-20.cable.dynamic.surfer.at [213.47.41.20]) (Authenticated sender: michael@niedermayer.cc) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 34CFC1BF204 for ; Tue, 12 Jun 2018 21:25:56 +0000 (UTC) Date: Tue, 12 Jun 2018 23:25:47 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20180612212547.GJ4859@michaelspb> References: <20180531230107.GV4859@michaelspb> <20180609181643.GU4859@michaelspb> <20180611004318.GE4859@michaelspb> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Level: Subject: Re: [FFmpeg-devel] qt-faststart bug near 4GB X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 > > > > > > > > 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 > 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 [...] --- - 2018-06-12 23:23:10.487549933 +0200 +++ tests/data/fate/mov-faststart-4gb-overflow 2018-06-12 23:23:10.479161196 +0200 @@ -1 +0,0 @@ -bc875921f151871e787c4b4023269b29