diff mbox series

[FFmpeg-devel,15/21] avcodec/smacker: Replace implicit checks for overread by explicit ones

Message ID 20200801134704.3647-7-andreas.rheinhardt@gmail.com
State Accepted
Commit 527b853d1a72beb08ffaa63239d13b2c0c327c66
Headers show
Series [FFmpeg-devel,1/8] avcodec/smacker: Remove write-only and unused variables | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 1, 2020, 1:46 p.m. UTC 
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 <andreas.rheinhardt@gmail.com>
---
 libavcodec/smacker.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

Comments

Paul B Mahol Aug. 1, 2020, 2:11 p.m. UTC | #1 
On 8/1/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> 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 <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/smacker.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>

Should be OK if tested under ASAN.
diff mbox series

Patch

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);
     }