From patchwork Wed Jun 24 13:46:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 20576 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 A031344B3C4 for ; Wed, 24 Jun 2020 16:53:49 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 682A868B5B6; Wed, 24 Jun 2020 16:53:49 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 87C7268A41E for ; Wed, 24 Jun 2020 16:53:43 +0300 (EEST) Received: by mail-wr1-f66.google.com with SMTP id v3so2403462wrc.1 for ; Wed, 24 Jun 2020 06:53:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=lhAeUX+GBuUhSdjBRfUw+CJad7DSch91utqZHxy75+I=; b=DAbXFg70Zb+VEnma+BtPLuwN08MsrIzMt1D3G/gG8W2JhhK9bEAaPhtIGSfxlnCWWW zMgZlIHqTCBfurgpUzn1hfbxlpIurPjTqdqtwWkOkH+g3xKI8slb+Fn9o0JfeqGsg40k PYloK0+9kmDvWCuX73HzEbkTgK8GpEVxZv9OXKid8YhL80nRu9fTm+bUsVPvhf6TZiwO 6CVYFRp32aWf3d8J9V6LEwpFJny2vlnF1o7D80FQ5FYhulq+hoTsuBIrGl7qtY22ymnH hqrLleOny9BxPAAf94LtB1zOZ1uaBeS6G1x3O6H9lcbmkn7azthsQkw634fvIAxgdiYs nTFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=lhAeUX+GBuUhSdjBRfUw+CJad7DSch91utqZHxy75+I=; b=Mj8QuuddrBy03A9LLHddHjNI4MdVUpzanceopdfpCpnkvMpJ45dtERg+FbpsVFLniy DAKwiMUI/mZMyPrlRIGPp0SWaGK3B/sCCPrKrnAuVpuTI4DqroedGvBPhLABh9NAR5Vd nMX9UpBQa/G+08Plex8hKxup1l22ejH/2HJADh+QYZqqAd966EC2IvvmsKcelfr3rJGc MbvhS4K0wm39kd9O17hkmmRx+AeX7FxkomVw9JgcIN7gcESspBA9+kNTZmcYuW5lpZu8 35EXZnnf0ZWZXPw/03kM6IRmhymJhE6EsbaDBucsNNuHzWxtMlC5sghb3DobNe0O3L/f YQ7Q== X-Gm-Message-State: AOAM530j9dwz8Z0lPB0jYi3gVDNcpuh6usCGEa7ltiCfb3DTNI5rxNTO JJ+RvV7kZ3JM5D86PQgYF+yRIGcz X-Google-Smtp-Source: ABdhPJwdmbuK28F5O3YA+N4f29cCzGhazZXhXkBjH1phh8J3FPKtaCitXDpbCR8iPwFZYNr6tBjXfA== X-Received: by 2002:adf:e545:: with SMTP id z5mr30357312wrm.89.1593006822395; Wed, 24 Jun 2020 06:53:42 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id y25sm8172390wma.19.2020.06.24.06.53.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jun 2020 06:53:41 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Wed, 24 Jun 2020 15:46:58 +0200 Message-Id: <20200624134705.14833-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200624134705.14833-1-andreas.rheinhardt@gmail.com> References: <20200624134705.14833-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/9] avformat/smacker: Avoid potential inifinite loop on error 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" When reading a new frame, the Smacker demuxer seeks to the next frame position where it excepts the next frame; then it (potentially) reads the palette, the audio packets associated with the frame and finally the actual video frame. It is only at the end that the frame counter as well as the position where the next frame is expected get updated. This has a downside: If e.g. invalid data is encountered when reading the palette, the demuxer returns immediately (with an error) and if the caller calls av_read_frame again, the demuxer seeks to the position where it already was, reads the very same palette data again and therefore will return an error again. If the caller calls av_read_frame repeatedly (say, until a packet is received or until EOF), this meight become an infinite loop. This could also happen if e.g. the size of one of the audio frames was invalid or if the frame size was gigantic. This commit changes this by skipping a frame if it turns out to be invalid or an error happens otherwise. This ensures that EOF will be returned eventually in the above scenario. Signed-off-by: Andreas Rheinhardt --- libavformat/smacker.c | 48 ++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/libavformat/smacker.c b/libavformat/smacker.c index 14dc4ef406..787c5d8972 100644 --- a/libavformat/smacker.c +++ b/libavformat/smacker.c @@ -57,7 +57,6 @@ typedef struct SmackerContext { int buf_sizes[7]; int stream_id[7]; int curstream; - int64_t nextpos; int64_t aud_pts[7]; } SmackerContext; @@ -229,8 +228,6 @@ static int smacker_read_header(AVFormatContext *s) return AVERROR(EIO); } - smk->nextpos = avio_tell(pb); - return 0; } @@ -238,6 +235,7 @@ static int smacker_read_header(AVFormatContext *s) static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) { SmackerContext *smk = s->priv_data; + int64_t next_frame_pos; int flags; int ret; int i; @@ -249,8 +247,8 @@ static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) /* if we demuxed all streams, pass another frame */ if (smk->curstream <= 0) { - avio_seek(s->pb, smk->nextpos, 0); frame_size = smk->frm_size[smk->cur_frame] & (~3); + next_frame_pos = avio_tell(s->pb) + (unsigned)frame_size; flags = smk->frm_flags[smk->cur_frame]; /* handle palette change event */ if(flags & SMACKER_PAL){ @@ -261,8 +259,10 @@ static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) memcpy(oldpal, pal, 768); size = avio_r8(s->pb); size = size * 4 - 1; - if(size + 1 > frame_size) - return AVERROR_INVALIDDATA; + if (size + 1 > frame_size) { + ret = AVERROR_INVALIDDATA; + goto next_frame; + } frame_size -= size; frame_size--; sz = 0; @@ -279,7 +279,8 @@ static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) av_log(s, AV_LOG_ERROR, "Invalid palette update, offset=%d length=%d extends beyond palette size\n", off, j); - return AVERROR_INVALIDDATA; + ret = AVERROR_INVALIDDATA; + goto next_frame; } off *= 3; while(j-- && sz < 256) { @@ -305,44 +306,45 @@ static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) for(i = 0; i < 7; i++) { if(flags & 1) { uint32_t size; - int err; size = avio_rl32(s->pb) - 4; if (!size || size + 4LL > frame_size) { av_log(s, AV_LOG_ERROR, "Invalid audio part size\n"); - return AVERROR_INVALIDDATA; + ret = AVERROR_INVALIDDATA; + goto next_frame; } frame_size -= size; frame_size -= 4; - if ((err = av_reallocp(&smk->bufs[smk->curstream], size)) < 0) { + if ((ret = av_reallocp(&smk->bufs[smk->curstream], size)) < 0) { smk->buf_sizes[smk->curstream] = 0; - return err; + goto next_frame; } smk->buf_sizes[smk->curstream] = size; - ret = avio_read(s->pb, smk->bufs[smk->curstream], size); - if(ret != size) - return AVERROR(EIO); + ret = ffio_read_size(s->pb, smk->bufs[smk->curstream], size); + if (ret < 0) + goto next_frame; smk->stream_id[smk->curstream] = smk->indexes[i]; smk->curstream++; } flags >>= 1; } - if (frame_size < 0 || frame_size >= INT_MAX/2) - return AVERROR_INVALIDDATA; + if (frame_size < 0 || frame_size >= INT_MAX/2) { + ret = AVERROR_INVALIDDATA; + goto next_frame; + } if ((ret = av_new_packet(pkt, frame_size + 769)) < 0) - return ret; + goto next_frame; if(smk->frm_size[smk->cur_frame] & 1) palchange |= 2; pkt->data[0] = palchange; memcpy(pkt->data + 1, smk->pal, 768); - ret = avio_read(s->pb, pkt->data + 769, frame_size); - if(ret != frame_size) - return AVERROR(EIO); + ret = ffio_read_size(s->pb, pkt->data + 769, frame_size); + if (ret < 0) + goto next_frame; pkt->stream_index = smk->videoindex; pkt->pts = smk->cur_frame; pkt->size = ret + 769; smk->cur_frame++; - smk->nextpos = avio_tell(s->pb); } else { smk->curstream--; if (smk->stream_id[smk->curstream] < 0 || !smk->bufs[smk->curstream]) @@ -357,6 +359,10 @@ static int smacker_read_packet(AVFormatContext *s, AVPacket *pkt) } return 0; +next_frame: + avio_seek(s->pb, next_frame_pos, SEEK_SET); + smk->cur_frame++; + return ret; } static int smacker_read_close(AVFormatContext *s)