diff mbox series

[FFmpeg-devel,08/13] avcodec/wnv1: Use LE bitstream reader, avoid copying packet, fix memleak

Message ID 20200829175626.11682-8-andreas.rheinhardt@gmail.com
State Accepted
Commit 0166b1d1a6d052ef49aba3523d64f3c6d4f26372
Headers show
Series [FFmpeg-devel,01/13] avcodec/cinepakenc: Cleanup generically after init failure | 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. 29, 2020, 5:56 p.m. UTC 
The Winnov WNV1 format is designed for a little-endian bitstream reader;
yet our decoder reversed every byte bitwise (in a buffer only
allocated for this purpose) to use a big-endian bitstream reader. This
commit stops this.

Two things needed to be done to achieve this: The codes in the table used
to initialize a VLC reader needed to be reversed bitwise (when
initializing a VLC in LE mode, it is expected that the first bit to be
read is in the least significant bit; with BE codes the first bit to be
read is the most significant bit of the code) and the following
expression needed to be adapted:

ff_reverse[get_bits(&w->gb, 8 - w->shift)]

But this is easy: When only the bits read are reversed, they coincide
with what a little-endian bitstream reader reads that reads the
original, not-reversed data. But ff_reverse always reverses the full
eight bits and this also performs a shift by (8 - (8 - w->shift)) on top
of reversing the bits read. So the above line needs to be changed to

get_bits(&w->gb, 8 - w->shift) << w->shift

and this also shows why the variable shift is named the way it is.

Finally, this also fixes a hypothetical memleak: For gigantic packets,
initializing a GetBitContext can fail and in this case, the buffer
containing the reversed data would leak.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/wnv1.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

Comments

Paul B Mahol Aug. 29, 2020, 6:04 p.m. UTC | #1 
On 8/29/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> The Winnov WNV1 format is designed for a little-endian bitstream reader;
> yet our decoder reversed every byte bitwise (in a buffer only
> allocated for this purpose) to use a big-endian bitstream reader. This
> commit stops this.
>
> Two things needed to be done to achieve this: The codes in the table used
> to initialize a VLC reader needed to be reversed bitwise (when
> initializing a VLC in LE mode, it is expected that the first bit to be
> read is in the least significant bit; with BE codes the first bit to be
> read is the most significant bit of the code) and the following
> expression needed to be adapted:
>
> ff_reverse[get_bits(&w->gb, 8 - w->shift)]
>
> But this is easy: When only the bits read are reversed, they coincide
> with what a little-endian bitstream reader reads that reads the
> original, not-reversed data. But ff_reverse always reverses the full
> eight bits and this also performs a shift by (8 - (8 - w->shift)) on top
> of reversing the bits read. So the above line needs to be changed to
>
> get_bits(&w->gb, 8 - w->shift) << w->shift
>
> and this also shows why the variable shift is named the way it is.
>
> Finally, this also fixes a hypothetical memleak: For gigantic packets,
> initializing a GetBitContext can fail and in this case, the buffer
> containing the reversed data would leak.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/wnv1.c | 28 +++++++---------------------
>  1 file changed, 7 insertions(+), 21 deletions(-)
>

LGTM, nice.
diff mbox series

Patch

diff --git a/libavcodec/wnv1.c b/libavcodec/wnv1.c
index 915e9c7dc9..857807a951 100644
--- a/libavcodec/wnv1.c
+++ b/libavcodec/wnv1.c
@@ -24,10 +24,10 @@ 
  * Winnov WNV1 codec.
  */
 
+#define BITSTREAM_READER_LE
 #include "avcodec.h"
 #include "get_bits.h"
 #include "internal.h"
-#include "mathops.h"
 
 
 typedef struct WNV1Context {
@@ -36,9 +36,9 @@  typedef struct WNV1Context {
 } WNV1Context;
 
 static const uint16_t code_tab[16][2] = {
-    { 0x1FD, 9 }, { 0xFD, 8 }, { 0x7D, 7 }, { 0x3D, 6 }, { 0x1D, 5 }, { 0x0D, 4 }, { 0x005, 3 },
+    { 0x17F, 9 }, { 0xBF, 8 }, { 0x5F, 7 }, { 0x2F, 6 }, { 0x17, 5 }, { 0x0B, 4 }, { 0x005, 3 },
     { 0x000, 1 },
-    { 0x004, 3 }, { 0x0C, 4 }, { 0x1C, 5 }, { 0x3C, 6 }, { 0x7C, 7 }, { 0xFC, 8 }, { 0x1FC, 9 }, { 0xFF, 8 }
+    { 0x01, 3 }, { 0x03, 4 }, { 0x07, 5 }, { 0x0F, 6 }, { 0x1F, 7 }, { 0x3F, 8 }, { 0x07F, 9 }, { 0xFF, 8 }
 };
 
 #define CODE_VLC_BITS 9
@@ -50,7 +50,7 @@  static inline int wnv1_get_code(WNV1Context *w, int base_value)
     int v = get_vlc2(&w->gb, code_vlc.table, CODE_VLC_BITS, 1);
 
     if (v == 15)
-        return ff_reverse[get_bits(&w->gb, 8 - w->shift)];
+        return get_bits(&w->gb, 8 - w->shift) << w->shift;
     else
         return base_value + ((v - 7U) << w->shift);
 }
@@ -66,30 +66,17 @@  static int decode_frame(AVCodecContext *avctx,
     unsigned char *Y,*U,*V;
     int i, j, ret;
     int prev_y = 0, prev_u = 0, prev_v = 0;
-    uint8_t *rbuf;
 
     if (buf_size < 8 + avctx->height * (avctx->width/2)/8) {
         av_log(avctx, AV_LOG_ERROR, "Packet size %d is too small\n", buf_size);
         return AVERROR_INVALIDDATA;
     }
 
-    rbuf = av_malloc(buf_size + AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!rbuf) {
-        av_log(avctx, AV_LOG_ERROR, "Cannot allocate temporary buffer\n");
-        return AVERROR(ENOMEM);
-    }
-    memset(rbuf + buf_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
-
-    if ((ret = ff_get_buffer(avctx, p, 0)) < 0) {
-        av_free(rbuf);
+    if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
         return ret;
-    }
     p->key_frame = 1;
 
-    for (i = 8; i < buf_size; i++)
-        rbuf[i] = ff_reverse[buf[i]];
-
-    if ((ret = init_get_bits8(&l->gb, rbuf + 8, buf_size - 8)) < 0)
+    if ((ret = init_get_bits8(&l->gb, buf + 8, buf_size - 8)) < 0)
         return ret;
 
     if (buf[2] >> 4 == 6)
@@ -127,7 +114,6 @@  static int decode_frame(AVCodecContext *avctx,
 
 
     *got_frame      = 1;
-    av_free(rbuf);
 
     return buf_size;
 }
@@ -142,7 +128,7 @@  static av_cold int decode_init(AVCodecContext *avctx)
     code_vlc.table_allocated = 1 << CODE_VLC_BITS;
     init_vlc(&code_vlc, CODE_VLC_BITS, 16,
              &code_tab[0][1], 4, 2,
-             &code_tab[0][0], 4, 2, INIT_VLC_USE_NEW_STATIC);
+             &code_tab[0][0], 4, 2, INIT_VLC_USE_NEW_STATIC | INIT_VLC_LE);
 
     return 0;
 }