From patchwork Sun Aug 18 08:47:26 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Tomas_H=C3=A4rdin?= X-Patchwork-Id: 14573 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 8DF2844766F for ; Sun, 18 Aug 2019 11:47:34 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 75E8068A8C2; Sun, 18 Aug 2019 11:47:34 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from tomyris.acc.umu.se (tomyris.acc.umu.se [130.239.18.190]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4D166689882 for ; Sun, 18 Aug 2019 11:47:28 +0300 (EEST) Received: from localhost (localhost.localdomain [127.0.0.1]) by amavisd-new (Postfix) with ESMTP id C5AFD44B92 for ; Sun, 18 Aug 2019 10:47:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=acc.umu.se; s=mail1; t=1566118047; bh=C9AHpwTm8xSqQjmtH7rgWgVbFQ13twMjAyizZGB1pYQ=; h=Subject:From:To:Date:In-Reply-To:References:From; b=nUk2+cahkWotBSPikyAKKxN5DQZxbE0D4MaEXf9WIKSPLib0UX1pEzkBWRu4GVCZG 8aG1EPiDwlHOMEgMAP3puY5MpGTU8biHgJSEHfsME2heYEwA4Mx7JwGN5VGltHl4Dw 4xRBZVmZptlmVTScjLysr+yopFFVtfJYKb5m1erztf19wEDEet7A5OJPKRk+KIbS+s 5hDex/qP9vI1qX8x5ps+SMQKGHSGlVk86OoXzWGlNl/xc7bupufsxeihfgTyz4O/eO jAB3h8lnRNg7dJInySJJ7Gx3ARkP/LWbc1es/PcReMbvkWikaDHaRzB43q6cyfLKHY fax2IUxq9NI5g== Received: from laptop.lan (h-39-105.A258.priv.bahnhof.se [79.136.39.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: tjoppen) by tomyris.acc.umu.se (Postfix) with ESMTPSA id B6D1044B91 for ; Sun, 18 Aug 2019 10:47:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=acc.umu.se; s=mail1; t=1566118046; bh=C9AHpwTm8xSqQjmtH7rgWgVbFQ13twMjAyizZGB1pYQ=; h=Subject:From:To:Date:In-Reply-To:References:From; b=Kooz8+YYxEgGGTAzHGObd2rtwK2UNHeihC6QPcig2bOt09aOYhP29z1FMyisP5rSk Y6CbFdWGw9YRWDbZhupBoYsOok07gFcXUVbVXpeEVmpNgV1cmbnFQhxLXHcvZgVYjn l9Hwt2AK8ji9PV0P8EYLfNEKjX9ZdqeHQilAKbcMuEqkJ51xNTOQGPhbyELG2ag5zG m9xNUJ/HZdUMPMQn0+DilXSMozUtu8ZXpr44KTpw9l9aVPTKsAD+Z9ic/64nOIijMe B262lPvKq5290575VXP4D/iWiPQcBKVd7sSFRqmin6swBVAdixDNH1HKmTKf99K8bM To4IW/GcKRyHg== Message-ID: From: Tomas =?ISO-8859-1?Q?H=E4rdin?= To: FFmpeg development discussions and patches Date: Sun, 18 Aug 2019 10:47:26 +0200 In-Reply-To: <8b6a6873ad1a224ef3b927597caaf1692cfdbaa3.camel@acc.umu.se> References: <20190812191708.22608-1-michael@niedermayer.cc> <2c6777cba7562d7e677fe36bb2885a0817450c4a.camel@acc.umu.se> <20190815225055.GV3219@michaelspb> <3b50932276b7cfa367dfea6bedf39224280339f9.camel@acc.umu.se> <38b88c8a9d4487ba339b0e4a196523a1f5cbddfc.camel@acc.umu.se> <20190817153349.GD3219@michaelspb> <8b6a6873ad1a224ef3b927597caaf1692cfdbaa3.camel@acc.umu.se> User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input 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" sön 2019-08-18 klockan 02:35 +0200 skrev Tomas Härdin: > lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer: > > On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote: > > > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin: > > > > > > I feel I should point out that being conservative here is at odds with > > > the general "best effort" approach taken in this project. These toy > > > codecs are useful as illustrative examples of this contradiction. I'm > > > sure there are many more examples of files that can cause ffmpeg to do > > > a lot more work than expected, for almost every codec. I know afl-fuzz > > > is likely to find out that it can make the ZMBV decoder do a lot of > > > work for a small input file, because I've seen it do that with gzip. > > > > > > The user base for cinepak is of course miniscule, so I doubt anyone's > > > going to complain that their broken CVID files don't work any more. I > > > certainly don't care since cinepakenc only puts out valid files. > > > But > > > again, for other formats we're just going to have to tell users to put > > > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is > > > vulnerable to DoS-y things. > > > > yes > > > > the question ATM is just what to do here about this codec ? > > apply the patch ? > > change it ? > > Well for a start, the file is 65535 x 209 pixels, 3166 frames. I > wouldn't call decoding that @ 263 fps particularly slow > > Second, it's not the decoder which is slow. If I comment out the > "*got_frame = 1;" then the test also runs fast. I'm not sure what > happens elsewhere with the decoded buffer, but I suspect there's a > bunch of useless malloc()/memset()ing going on. Maybe the decoder is > using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure. I did some investigation, it is indeed ff_reget_buffer(). It copies the frame data for some reason. The fix is simple in this case: just call ff_get_buffer() once in cinepak_decode_init() and keep overwriting the same frame. > As I said on IRC, this class of problems will exist for every codec. > Cinepak is easy to decode, even at these resolutions. Just imagine what > will happens when someone feeds in a 65535x209 av1 stream.. And related to this, ff_reget_buffer() is used for a lot of these codecs which only overwrite pixels in the old frame. flicvideo, gifdec, msrle, roqvideodec and others probably have the same flaw. Patched attached. /Tomas From c85f23ca4c42f9ce27f512be897214b8689c1c94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= Date: Sun, 18 Aug 2019 10:30:59 +0200 Subject: [PATCH] libavcodec/cinepak: Avoid copying frame data We can just keep overwriting the same frame. This speeds up the decoder, especially for very large frames, since skip MBs are turned into nops. clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 32577 ms -> 42 ms --- libavcodec/cinepak.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index aeb15de0ed..e3e6ecfdf0 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -424,6 +424,7 @@ static int cinepak_decode (CinepakContext *s) static av_cold int cinepak_decode_init(AVCodecContext *avctx) { CinepakContext *s = avctx->priv_data; + int ret; s->avctx = avctx; s->width = (avctx->width + 3) & ~3; @@ -444,6 +445,9 @@ static av_cold int cinepak_decode_init(AVCodecContext *avctx) if (!s->frame) return AVERROR(ENOMEM); + if ((ret = ff_get_buffer(avctx, s->frame, 0)) < 0) + return ret; + return 0; } @@ -473,9 +477,6 @@ static int cinepak_decode_frame(AVCodecContext *avctx, return ret; } - if ((ret = ff_reget_buffer(avctx, s->frame)) < 0) - return ret; - if (s->palette_video) { int size; const uint8_t *pal = av_packet_get_side_data(avpkt, AV_PKT_DATA_PALETTE, &size); -- 2.20.1