From patchwork Sat Aug 1 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: 21433 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 B7C7C44AA8D for ; Sat, 1 Aug 2020 16:49:43 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9983B68BAAD; Sat, 1 Aug 2020 16:49:43 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com [209.85.208.67]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 231FD68BA7B for ; Sat, 1 Aug 2020 16:49:36 +0300 (EEST) Received: by mail-ed1-f67.google.com with SMTP id df16so7874555edb.9 for ; Sat, 01 Aug 2020 06:49:36 -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=d43Gj1GzHz+XRGEgOBM2RVq2/1j6py63NkvCTPWBHOY=; b=AOu5BevFCInhiBHBK5fX1s6sSOUAARbEoIEFiU3DQZ2K3O2nCFrSD6EWcwqIr2dJuH nhuJyMIkQkPvIj4nDbbMg6K3MydjM2Sl8Oyqv9fdFbJKbqJIkM4HaGHzcRf1ENiZr8uQ mgPfpvyb0VYFJPtRAXyocNTSsTtvA7/B2GaHc930NxZ8kt9xBluuQuKZI35XwJjEpYjH XCZAmxuMoDzTzO3yfm6APrBOpHTOE9unigEyBd5PFi1HApP3RpZIrhhzA1k6GoId36Aj vrWqZcjxdILOUd7aP2nvIT7o/4DAT8WlZP3X31551EP6UkIsyql2F0iAc8z/d+AmHMKd 2PyQ== 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=d43Gj1GzHz+XRGEgOBM2RVq2/1j6py63NkvCTPWBHOY=; b=f0XDirnvgVX0wPRtaLyE7aRJ90JNmHZps4yIyUwSVBkT3KEkYZKvWw1Pc6TtGpR4BZ BLeRZLo9XDgJlSmR68+f/EPtLt/WskrvQIcT6FNtO+Bu+xCSTSCoTZHIUCLW3hxvxEAc 27PW/Ax2xYtJAGXcBknTilt0JA1GuQ3Bqp3R3GGZ0LdKxksa/SeihMCLNKIB9xvvgQam MT6RGSU6zSc2hYE1TJyk31YnOhjS1NNPXbArIF0QmIVUI8TMTIadErmFDvqB+KP9zeSf mZ4m/UoaoZYFH2QrLbnvskPvcxFeAqdWHkgfKJ9lg9tfHs/pnohBnZQyOiYkJe1sT3jI ZwDg== X-Gm-Message-State: AOAM531TuC+dxLspMfMZbJdNS3DK6njBzFcoQs0D22IDUO7SkB1rjMmg kE6YSljGJ97QLRQrVo60lTmlgrZO X-Google-Smtp-Source: ABdhPJwWTJO2wRJCAV9NSjAu39365f1Pr4Gd7OsURO9iFcQltSnijE+pAjytxDZA3BJbEwx1mr3wEA== X-Received: by 2002:aa7:d387:: with SMTP id x7mr8231330edq.219.1596289775061; Sat, 01 Aug 2020 06:49:35 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id b24sm12178501edn.33.2020.08.01.06.49.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 01 Aug 2020 06:49:34 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sat, 1 Aug 2020 15:46:58 +0200 Message-Id: <20200801134704.3647-7-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200731112241.8948-1-andreas.rheinhardt@gmail.com> References: <20200731112241.8948-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 15/21] avcodec/smacker: Replace implicit checks for overread by explicit ones 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" Using explicit checks has the advantage that one can combine several checks into one and does not have to check every time. E.g. reading a 16bit PCM sample involves two calls to get_vlc2(), each of which may read up to three times up to SMKTREE_BITS (= 9) bits. But given that the padding that the input packet is supposed to have is large enough, it is no problem to only check once for each sample. This turned out to be beneficial for performance: For GCC 9, the time for one call of smka_decode_frame() for the sample from ticket #2425 went down from 2055905 to 1804751 decicycles; for Clang 9 it went down from 1510538 to 1479680 decicycles. Signed-off-by: Andreas Rheinhardt --- libavcodec/smacker.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c index 5e88e18aec..72e007077d 100644 --- a/libavcodec/smacker.c +++ b/libavcodec/smacker.c @@ -33,19 +33,27 @@ #include "libavutil/channel_layout.h" -#define BITSTREAM_READER_LE #include "avcodec.h" -#include "bytestream.h" -#include "get_bits.h" -#include "internal.h" -#include "mathops.h" #define SMKTREE_BITS 9 #define SMK_NODE 0x80000000 -#define SMKTREE_DECODE_MAX_RECURSION 32 +#define SMKTREE_DECODE_MAX_RECURSION FFMIN(32, 3 * SMKTREE_BITS) #define SMKTREE_DECODE_BIG_MAX_RECURSION 500 +/* The maximum possible unchecked overread happens in decode_header_trees: + * Decoding the MMAP tree can overread by 6 * SMKTREE_BITS + 1, followed by + * three get_bits1, followed by at most 2 + 3 * 16 read bits when reading + * the TYPE tree before the next check. 64 is because of 64 bit reads. */ +#if (6 * SMKTREE_BITS + 1 + 3 + (2 + 3 * 16) + 64) <= 8 * AV_INPUT_BUFFER_PADDING_SIZE +#define UNCHECKED_BITSTREAM_READER 1 +#endif +#define BITSTREAM_READER_LE +#include "bytestream.h" +#include "get_bits.h" +#include "internal.h" +#include "mathops.h" + typedef struct SmackVContext { AVCodecContext *avctx; AVFrame *pic; @@ -92,6 +100,9 @@ enum SmkBlockTypes { /** * Decode local frame tree + * + * Can read SMKTREE_DECODE_MAX_RECURSION before the first check; + * does not overread gb on success. */ static int smacker_decode_tree(GetBitContext *gb, HuffContext *hc, uint32_t prefix, int length) { @@ -105,6 +116,8 @@ static int smacker_decode_tree(GetBitContext *gb, HuffContext *hc, uint32_t pref av_log(NULL, AV_LOG_ERROR, "Tree size exceeded!\n"); return AVERROR_INVALIDDATA; } + if (get_bits_left(gb) < 8) + return AVERROR_INVALIDDATA; hc->bits[hc->current] = prefix; hc->lengths[hc->current] = length; hc->values[hc->current] = get_bits(gb, 8); @@ -122,6 +135,8 @@ static int smacker_decode_tree(GetBitContext *gb, HuffContext *hc, uint32_t pref /** * Decode header tree + * + * Checks before the first read, can overread by 6 * SMKTREE_BITS on success. */ static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx, int length) @@ -136,6 +151,8 @@ static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, av_log(NULL, AV_LOG_ERROR, "Tree size exceeded!\n"); return AVERROR_INVALIDDATA; } + if (get_bits_left(gb) <= 0) + return AVERROR_INVALIDDATA; if(!get_bits1(gb)){ //Leaf int val, i1, i2; i1 = ctx->v1->table ? get_vlc2(gb, ctx->v1->table, SMKTREE_BITS, 3) : 0; @@ -172,6 +189,9 @@ static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, /** * Store large tree as FFmpeg's vlc codes + * + * Can read FFMAX(1 + SMKTREE_DECODE_MAX_RECURSION, 2 + 3 * 16) bits + * before the first check; can overread by 6 * SMKTREE_BITS + 1 on success. */ static int smacker_decode_header_tree(SmackVContext *smk, GetBitContext *gb, int **recodes, int *last, int size) { @@ -329,7 +349,7 @@ static int decode_header_trees(SmackVContext *smk) { if (ret < 0) return ret; } - if (skip == 4) + if (skip == 4 || get_bits_left(&gb) < 0) return AVERROR_INVALIDDATA; return 0; @@ -339,7 +359,8 @@ static av_always_inline void last_reset(int *recode, int *last) { recode[last[0]] = recode[last[1]] = recode[last[2]] = 0; } -/* get code and update history */ +/* Get code and update history. + * Checks before reading, does not overread. */ static av_always_inline int smk_get_code(GetBitContext *gb, int *recode, int *last) { register int *table = recode; int v; @@ -545,7 +566,7 @@ static av_cold int decode_init(AVCodecContext *avctx) return AVERROR(ENOMEM); /* decode huffman trees from extradata */ - if(avctx->extradata_size < 16){ + if (avctx->extradata_size <= 16){ av_log(avctx, AV_LOG_ERROR, "Extradata missing!\n"); return AVERROR(EINVAL); }