diff mbox series

[FFmpeg-devel,13/21] avcodec/smacker: Remove redundant checks when reading VLC codes

Message ID 20200801134704.3647-5-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/8] avcodec/smacker: Remove write-only and unused variables
Related show

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
The VLC codes in question originate from a Huffmann tree and so every
sequence of bits that is longer than the longest code contains an
initial sequence that is a valid code. Given that it has been checked
during reading said tree (and once again in ff_init_vlc_sparse()) that
the length of each code is <= 3 * the number of bits read at once when
reading codes, get_vlc2() will always find a matching entry.

These checks have been added in 71d3c25a7ef442ac2dd7b6fbf7c489ebc0b58e9b
at a time when the length of the codes had not been checked when parsing
the tree.

For GCC 9 and the sample from ticket #2425 this led to a slight
performance regression: The time for one call to smka_decode_frame()
increased from 2053671 to 2064529 decicycles; for Clang 9, performance
improved from 1521288 to 1508459 decicycles.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/smacker.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

Comments

Paul B Mahol Aug. 1, 2020, 2:15 p.m. UTC | #1
On 8/1/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> The VLC codes in question originate from a Huffmann tree and so every
> sequence of bits that is longer than the longest code contains an
> initial sequence that is a valid code. Given that it has been checked
> during reading said tree (and once again in ff_init_vlc_sparse()) that
> the length of each code is <= 3 * the number of bits read at once when
> reading codes, get_vlc2() will always find a matching entry.
>
> These checks have been added in 71d3c25a7ef442ac2dd7b6fbf7c489ebc0b58e9b
> at a time when the length of the codes had not been checked when parsing
> the tree.
>
> For GCC 9 and the sample from ticket #2425 this led to a slight
> performance regression: The time for one call to smka_decode_frame()
> increased from 2053671 to 2064529 decicycles; for Clang 9, performance
> improved from 1521288 to 1508459 decicycles.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/smacker.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
>

Sure this does not continue under bitstream errors?

Try some smart fuzzers.
Andreas Rheinhardt Aug. 1, 2020, 2:25 p.m. UTC | #2
Paul B Mahol:
> On 8/1/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> The VLC codes in question originate from a Huffmann tree and so every
>> sequence of bits that is longer than the longest code contains an
>> initial sequence that is a valid code. Given that it has been checked
>> during reading said tree (and once again in ff_init_vlc_sparse()) that
>> the length of each code is <= 3 * the number of bits read at once when
>> reading codes, get_vlc2() will always find a matching entry.
>>
>> These checks have been added in 71d3c25a7ef442ac2dd7b6fbf7c489ebc0b58e9b
>> at a time when the length of the codes had not been checked when parsing
>> the tree.
>>
>> For GCC 9 and the sample from ticket #2425 this led to a slight
>> performance regression: The time for one call to smka_decode_frame()
>> increased from 2053671 to 2064529 decicycles; for Clang 9, performance
>> improved from 1521288 to 1508459 decicycles.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/smacker.c | 32 --------------------------------
>>  1 file changed, 32 deletions(-)
>>
> 
> Sure this does not continue under bitstream errors?
> 
> Try some smart fuzzers.
> 
Every long enough sequence of bits (namely every sequence longer than
the longest Huffman code) contains an initial sequence that is a valid
Huffman code, so the only detectable bitstream error there could be is
end of bitstream (there is of course also the possibility of outputting
garbage, but that is not detectable in the decoder). This statement
follows directly from the fact that every non-leaf node in a Huffman
tree has exactly two children that correspond to 0 and 1.

- Andreas

PS: Thanks for taking a look at this patchset.
diff mbox series

Patch

diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c
index 991622eb0e..5e1b8c59ff 100644
--- a/libavcodec/smacker.c
+++ b/libavcodec/smacker.c
@@ -140,8 +140,6 @@  static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc,
         int val, i1, i2;
         i1 = ctx->v1->table ? get_vlc2(gb, ctx->v1->table, SMKTREE_BITS, 3) : 0;
         i2 = ctx->v2->table ? get_vlc2(gb, ctx->v2->table, SMKTREE_BITS, 3) : 0;
-        if (i1 < 0 || i2 < 0)
-            return AVERROR_INVALIDDATA;
         val = ctx->recode1[i1] | (ctx->recode2[i2] << 8);
         if(val == ctx->escapes[0]) {
             ctx->last[0] = hc->current;
@@ -678,21 +676,11 @@  static int smka_decode_frame(AVCodecContext *avctx, void *data,
                     res = get_vlc2(&gb, vlc[2].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 val  = h[2].values[res];
                 if(vlc[3].table)
                     res = get_vlc2(&gb, vlc[3].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 val |= h[3].values[res] << 8;
                 pred[1] += (unsigned)sign_extend(val, 16);
                 *samples++ = pred[1];
@@ -701,21 +689,11 @@  static int smka_decode_frame(AVCodecContext *avctx, void *data,
                     res = get_vlc2(&gb, vlc[0].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 val  = h[0].values[res];
                 if(vlc[1].table)
                     res = get_vlc2(&gb, vlc[1].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 val |= h[1].values[res] << 8;
                 pred[0] += (unsigned)sign_extend(val, 16);
                 *samples++ = pred[0];
@@ -736,11 +714,6 @@  static int smka_decode_frame(AVCodecContext *avctx, void *data,
                     res = get_vlc2(&gb, vlc[1].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 pred[1] += sign_extend(h[1].values[res], 8);
                 *samples8++ = pred[1];
             } else {
@@ -748,11 +721,6 @@  static int smka_decode_frame(AVCodecContext *avctx, void *data,
                     res = get_vlc2(&gb, vlc[0].table, SMKTREE_BITS, 3);
                 else
                     res = 0;
-                if (res < 0) {
-                    av_log(avctx, AV_LOG_ERROR, "invalid vlc\n");
-                    ret = AVERROR_INVALIDDATA;
-                    goto error;
-                }
                 pred[0] += sign_extend(h[0].values[res], 8);
                 *samples8++ = pred[0];
             }