| Message ID | 20200801134704.3647-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 3899adc29869301bfab77a9064a21da7a5a5163d |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avcodec/smacker: Remove write-only and unused variables | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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.
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.
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. > I have now finally fuzzed my patches here and found no bugs at all (neither with ASAN nor with UBSan). I'll therefore apply it. - Andreas
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]; }
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(-)