[FFmpeg-devel,PATCH/RFC] libavcodec/cinepak: Separate decoding from parsing

Submitted by Tomas Härdin on Sept. 3, 2019, 5:29 p.m.

Details

Message ID 5c6faf611f4f497af427e9c20c82406d91c7cff6.camel@acc.umu.se
State New
Headers show

Commit Message

Tomas Härdin Sept. 3, 2019, 5:29 p.m.
tis 2019-09-03 klockan 13:53 +0200 skrev Michael Niedermayer:
> On Sun, Sep 01, 2019 at 10:58:27PM +0200, Tomas Härdin wrote:
> >  libavcodec/cinepak.c        |  337 ++++++++++++++++++++++++++++---
> > -------------
> >  tests/ref/fate/cvid-palette |    1 
> >  tests/ref/fate/cvid-partial |    1 
> >  3 files changed, 215 insertions(+), 124 deletions(-)
> > 197f986bfb297c54ee0f208449d4053fcb948645  0001-libavcodec-cinepak-
> > Separate-decoding-from-parsing.patch
> > From d4bda2edbecb93fa5c5910b8b91a866210368a95 Mon Sep 17 00:00:00
> > 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
> > Date: Sun, 1 Sep 2019 15:37:13 +0200
> > Subject: [PATCH] libavcodec/cinepak: Separate decoding from parsing
> > 
> > This is rather strict and replaces the previous hacks with checks
> > that should hopefully placate the fuzzers for the forseeable future
> > ---
> >  libavcodec/cinepak.c        | 337 +++++++++++++++++++++++---------
> > ----
> >  tests/ref/fate/cvid-palette |   1 -
> >  tests/ref/fate/cvid-partial |   1 -
> >  3 files changed, 215 insertions(+), 124 deletions(-)
> 
> this does not seem to apply automatically here

Oops, forgot to squash WIP commits into a single patch. Squashed,
rebased and tested patch attached. Moved some functions around and used
format-patch --patience to make the diff smaller. And again, it's still
preliminary as I want feedback before going any further

/Tomas

Patch hide | download patch | download mbox

From bfc4fb532b340d62eb4cd1db5d4b93a273f95b6c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
Date: Sun, 18 Aug 2019 17:42:27 +0200
Subject: [PATCH] libavcodec/cinepak: Separate decoding from parsing

This is rather strict and replaces the previous hacks with checks that should hopefully placate the fuzzers for the forseeable future
---
 libavcodec/cinepak.c        | 268 +++++++++++++++++++++++++-----------
 tests/ref/fate/cvid-palette |   1 -
 tests/ref/fate/cvid-partial |   1 -
 3 files changed, 191 insertions(+), 79 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index aeb15de0ed..03cbb1777a 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -46,6 +46,8 @@ 
 typedef uint8_t cvid_codebook[12];
 
 #define MAX_STRIPS      32
+#define MAX_HEIGHT      UINT16_MAX
+#define MB_AREA         16
 
 typedef struct cvid_strip {
     uint16_t          id;
@@ -53,6 +55,8 @@  typedef struct cvid_strip {
     uint16_t          x2, y2;
     cvid_codebook     v4_codebook[256];
     cvid_codebook     v1_codebook[256];
+    const uint8_t     *data;    // points to strip data, excluding header
+    int               size;
 } cvid_strip;
 
 typedef struct CinepakContext {
@@ -60,9 +64,6 @@  typedef struct CinepakContext {
     AVCodecContext *avctx;
     AVFrame *frame;
 
-    const unsigned char *data;
-    int size;
-
     int width, height;
 
     int palette_video;
@@ -71,6 +72,10 @@  typedef struct CinepakContext {
     int sega_film_skip_bytes;
 
     uint32_t pal[256];
+
+    int frame_flags;
+    int num_strips;
+    uint8_t strip_cover[MAX_HEIGHT];
 } CinepakContext;
 
 static void cinepak_decode_codebook (cvid_codebook *codebook,
@@ -269,12 +274,6 @@  static int cinepak_decode_strip (CinepakContext *s,
     const uint8_t *eod = (data + size);
     int      chunk_id, chunk_size;
 
-    /* coordinate sanity checks */
-    if (strip->x2 > s->width   ||
-        strip->y2 > s->height  ||
-        strip->x1 >= strip->x2 || strip->y1 >= strip->y2)
-        return AVERROR_INVALIDDATA;
-
     while ((data + 4) <= eod) {
         chunk_id   = data[0];
         chunk_size = AV_RB24 (&data[1]) - 4;
@@ -315,16 +314,20 @@  static int cinepak_decode_strip (CinepakContext *s,
     return AVERROR_INVALIDDATA;
 }
 
-static int cinepak_predecode_check (CinepakContext *s)
+static int cinepak_parse_header(AVCodecContext *avctx, const AVPacket *avpkt)
 {
-    int           num_strips;
-    int           encoded_buf_size;
-
-    num_strips  = AV_RB16 (&s->data[8]);
-    encoded_buf_size = AV_RB24(&s->data[1]);
+    CinepakContext *s = avctx->priv_data;
+    int w, h, encoded_buf_size;
 
-    if (s->size < encoded_buf_size * (int64_t)(100 - s->avctx->discard_damaged_percentage) / 100)
+    if (avpkt->size < 10) {
         return AVERROR_INVALIDDATA;
+    }
+
+    s->frame_flags   = avpkt->data[0];
+    encoded_buf_size = AV_RB24(&avpkt->data[1]);
+    w                = AV_RB16(&avpkt->data[4]);
+    h                = AV_RB16(&avpkt->data[6]);
+    s->num_strips    = AV_RB16(&avpkt->data[8]);
 
     /* if this is the first frame, check for deviant Sega FILM data */
     if (s->sega_film_skip_bytes == -1) {
@@ -332,20 +335,20 @@  static int cinepak_predecode_check (CinepakContext *s)
             avpriv_request_sample(s->avctx, "encoded_buf_size 0");
             return AVERROR_PATCHWELCOME;
         }
-        if (encoded_buf_size != s->size && (s->size % encoded_buf_size) != 0) {
+        if (encoded_buf_size != avpkt->size && (avpkt->size % encoded_buf_size) != 0) {
             /* If the encoded frame size differs from the frame size as indicated
              * by the container file, this data likely comes from a Sega FILM/CPK file.
              * If the frame header is followed by the bytes FE 00 00 06 00 00 then
              * this is probably one of the two known files that have 6 extra bytes
              * after the frame header. Else, assume 2 extra bytes. The container
              * size also cannot be a multiple of the encoded size. */
-            if (s->size >= 16 &&
-                (s->data[10] == 0xFE) &&
-                (s->data[11] == 0x00) &&
-                (s->data[12] == 0x00) &&
-                (s->data[13] == 0x06) &&
-                (s->data[14] == 0x00) &&
-                (s->data[15] == 0x00))
+            if (avpkt->size >= 16 &&
+                (avpkt->data[10] == 0xFE) &&
+                (avpkt->data[11] == 0x00) &&
+                (avpkt->data[12] == 0x00) &&
+                (avpkt->data[13] == 0x06) &&
+                (avpkt->data[14] == 0x00) &&
+                (avpkt->data[15] == 0x00))
                 s->sega_film_skip_bytes = 6;
             else
                 s->sega_film_skip_bytes = 2;
@@ -353,70 +356,188 @@  static int cinepak_predecode_check (CinepakContext *s)
             s->sega_film_skip_bytes = 0;
     }
 
-    if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
+    // compare w and h to the width and height in CinepakContext, not AVCodecContext
+    // the former has been appropriately rounded up
+    if (w != s->width || h != s->height) {
+        av_log(avctx, AV_LOG_ERROR, "dimension mismatch (%ix%i vs %ix%i)\n", w, h, avctx->width, avctx->height);
         return AVERROR_INVALIDDATA;
+    }
 
-    if (num_strips) {
-        const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes;
-        int strip_size = AV_RB24 (data + 1);
-        if (strip_size < 12 || strip_size > encoded_buf_size)
-            return AVERROR_INVALIDDATA;
+    if (s->num_strips == 0) {
+        // the old code would accept num_strips == 0 if there was at least a palette change
+        // I know of no sample that requires this, and the VfW 1.1 decoder
+        // will produce garbage if the entire frame isn't covered by strips
+        av_log(avctx, AV_LOG_ERROR, "num_strips == 0\n");
+        return AVERROR_INVALIDDATA;
+    }
+
+    // unfortunately we can't demand that the sizes match exactly
+    // example: http://samples.mplayerhq.hu/V-codecs/CVID/bad_cinepak_frame_size.mov
+    // at least require that the header value is no larger than the size of the packet
+    if (encoded_buf_size + s->sega_film_skip_bytes > avpkt->size) {
+        av_log(avctx, AV_LOG_ERROR, "CVID size larger than actual packet: %i vs %i\n", encoded_buf_size + s->sega_film_skip_bytes, avpkt->size);
+        return AVERROR_INVALIDDATA;
+    }
+
+    if (s->sega_film_skip_bytes) {
+        int skip_diff = avpkt->size - encoded_buf_size - s->sega_film_skip_bytes;
+        if (skip_diff != 2 && skip_diff != 6) {
+            // there doesn't seem to be much pattern to these 2 or 6 last bytes
+            // getting samples with different skip_diffs might be useful for further RE work
+            avpriv_request_sample(s->avctx, "SEGA FILM file with skip_diff %i not equal to 2 or 6", skip_diff);
+            return AVERROR_PATCHWELCOME;
+        }
     }
 
     return 0;
 }
 
-static int cinepak_decode (CinepakContext *s)
+static int cinepak_parse_strip_headers(AVCodecContext *avctx, const AVPacket *avpkt)
 {
-    const uint8_t  *eod = (s->data + s->size);
-    int           i, result, strip_size, frame_flags, num_strips;
-    int           y0 = 0;
+    CinepakContext *s = avctx->priv_data;
+    const uint8_t  *ptr = avpkt->data + 10 + s->sega_film_skip_bytes;
+    int i, y0 = 0, bytes_left = avpkt->size, htot = 0;
 
-    frame_flags = s->data[0];
-    num_strips  = AV_RB16 (&s->data[8]);
+    if (s->num_strips > MAX_STRIPS) {
+        avpriv_request_sample(avctx, "num_strips %i > %i", s->num_strips, MAX_STRIPS);
+        return AVERROR_PATCHWELCOME;
+    }
 
-    s->data += 10 + s->sega_film_skip_bytes;
+    // the frame is a keyframe only if none of the strips are inter coded
+    s->frame->key_frame = 1;
 
-    num_strips = FFMIN(num_strips, MAX_STRIPS);
+    memset(s->strip_cover, 0, s->height);
 
-    s->frame->key_frame = 0;
+    for (i = 0; i < s->num_strips; i++) {
+        int id, w, h, pixels;
 
-    for (i=0; i < num_strips; i++) {
-        if ((s->data + 12) > eod)
-            return AVERROR_INVALIDDATA;
+        if (bytes_left < 12) {
+            avpriv_request_sample(avctx, "only %i bytes left for strip %i header", bytes_left, i);
+            return AVERROR_PATCHWELCOME;
+        }
+
+        id = s->strips[i].id = ptr[0];
+
+        if (id != 0x10 && id != 0x11 &&
+            // http://samples.mplayerhq.hu/V-codecs/CVID/1984.apple_ad.mov
+            id != 0x00 && id != 0x01) {
+            avpriv_request_sample(s->avctx, "unexpected strip ID %02x", s->strips[i].id);
+            return AVERROR_PATCHWELCOME;
+        }
 
-        s->strips[i].id = s->data[0];
 /* zero y1 means "relative to the previous stripe" */
-        if (!(s->strips[i].y1 = AV_RB16 (&s->data[4])))
-            s->strips[i].y2 = (s->strips[i].y1 = y0) + AV_RB16 (&s->data[8]);
+        if (!(s->strips[i].y1 = AV_RB16 (&ptr[4])))
+            s->strips[i].y2 = (s->strips[i].y1 = y0) + AV_RB16 (&ptr[8]);
         else
-            s->strips[i].y2 = AV_RB16 (&s->data[8]);
-        s->strips[i].x1 = AV_RB16 (&s->data[6]);
-        s->strips[i].x2 = AV_RB16 (&s->data[10]);
+            s->strips[i].y2 = AV_RB16 (&ptr[8]);
+        s->strips[i].x1 = AV_RB16 (&ptr[6]);
+        s->strips[i].x2 = AV_RB16 (&ptr[10]);
+
+        /* coordinate sanity checks */
+        if (s->strips[i].x2 > s->width   ||
+            s->strips[i].y2 > s->height  ||
+            s->strips[i].x1 >= s->strips[i].x2 ||
+            s->strips[i].y1 >= s->strips[i].y2) {
+            av_log(avctx, AV_LOG_ERROR, "strip %i with bad coordinates (%i,%i %i,%i)", i,
+                   (int)s->strips[i].x1, (int)s->strips[i].y1,
+                   (int)s->strips[i].x2, (int)s->strips[i].y2);
+            return AVERROR_INVALIDDATA;
+        }
 
-        if (s->strips[i].id == 0x10)
-            s->frame->key_frame = 1;
+        // cinepak.txt claims size is 16-bit, but
+        // http://samples.mplayerhq.hu/V-codecs/CVID/tn.avi
+        // has strips which are bigger than that
+        s->strips[i].size = AV_RB24(&ptr[1]);
 
-        strip_size = AV_RB24 (&s->data[1]) - 12;
-        if (strip_size < 0)
+        if (s->strips[i].size < 12 || s->strips[i].size > bytes_left) {
+            av_log(avctx, AV_LOG_ERROR, "invalid strip %i size: %i vs %i\n", i, s->strips[i].size, bytes_left);
             return AVERROR_INVALIDDATA;
-        s->data   += 12;
-        strip_size = ((s->data + strip_size) > eod) ? (eod - s->data) : strip_size;
+        }
 
-        if ((i > 0) && !(frame_flags & 0x01)) {
+        w = s->strips[i].x2 - s->strips[i].x1;
+        h = s->strips[i].y2 - s->strips[i].y1;
+        htot += h;
+        pixels = w*h;
+
+        // I'm not sure if vertical splitting is allowed, all samples available use only horizontal splitting
+        // disallow it for now, since allowing it would make the strip coverage check further down more expensive
+        // ideally we should check what both the VfW 1.1 and MacOS decoders do in this case
+        if (w != s->width) {
+            avpriv_request_sample(s->avctx, "strip with %i != frame width %i", w, s->width);
+            return AVERROR_PATCHWELCOME;
+        }
+
+        // id == 0x11 vs id != 0x10? 1984.apple_ad.mov makes this tricky
+        if (id != 0x10) {
+            // inter-coded -> not a keyframe
+            // there can be a mix of intra and inter strips in a single frame
+            s->frame->key_frame = 0;
+
+            // inter strips can be MODE_MC, which are at least 1 bit per MB
+            if (s->strips[i].size < pixels/(MB_AREA*8)) {
+                avpriv_request_sample(s->avctx, "strip %i too small to be inter (%i B vs %i pixels)",
+                    i, s->strips[i].size, pixels);
+                return AVERROR_PATCHWELCOME;
+            }
+        } else if (s->strips[i].size < pixels/MB_AREA) {
+            // intra strips are at least 8 bits per MB (MODE_V1_ONLY)
+            // MODE_V1_V4 is 9 bits per MB, but we don't know if the strip is that mode at this stage
+            avpriv_request_sample(s->avctx, "strip %i too small to be intra (%i B vs %i pixels)",
+                i, s->strips[i].size, pixels);
+            return AVERROR_PATCHWELCOME;
+        }
+
+        // mark coded lines
+        for (int y = s->strips[i].y1; y < s->strips[i].y2; y++) {
+            s->strip_cover[y] = 1;
+        }
+
+        // point into the strip data
+        s->strips[i].data  = ptr + 12;
+        ptr               += s->strips[i].size;
+        bytes_left        -= s->strips[i].size;
+        s->strips[i].size -= 12;
+        y0                 = s->strips[i].y2;
+    }
+
+    // require that every single line be coded at least once
+    for (int y = 0; y < s->height; y++) {
+        if (!s->strip_cover[y]) {
+            avpriv_request_sample(s->avctx, "line %i not covered by a strip", y);
+            return AVERROR_PATCHWELCOME;
+        }
+    }
+
+    // require that every line be coded *exactly* once
+    if (htot != s->height) {
+        avpriv_request_sample(s->avctx, "strip heights not adding up to frame height");
+        return AVERROR_PATCHWELCOME;
+    }
+
+    return 0;
+}
+
+static int cinepak_decode (CinepakContext *s)
+{
+    int i, result;
+
+    for (i = 0; i < s->num_strips; i++) {
+        // cinepak.txt claims that codebooks should be copied from the last strip of the previous frame
+        // I am not sure what to do in case we have just seeked to a keyframe
+        // this also requires keeping track of which strip was the last one,
+        // which I'm not doing quite yet
+        // the samples we have seem to decode fine the way this is right now, no need to mess with it unless a user complains
+        if ((i > 0) && !(s->frame_flags & 0x01)) {
             memcpy (s->strips[i].v4_codebook, s->strips[i-1].v4_codebook,
                 sizeof(s->strips[i].v4_codebook));
             memcpy (s->strips[i].v1_codebook, s->strips[i-1].v1_codebook,
                 sizeof(s->strips[i].v1_codebook));
         }
 
-        result = cinepak_decode_strip (s, &s->strips[i], s->data, strip_size);
+        result = cinepak_decode_strip (s, &s->strips[i], s->strips[i].data, s->strips[i].size);
 
         if (result != 0)
             return result;
-
-        s->data += strip_size;
-        y0    = s->strips[i].y2;
     }
     return 0;
 }
@@ -429,6 +550,13 @@  static av_cold int cinepak_decode_init(AVCodecContext *avctx)
     s->width = (avctx->width + 3) & ~3;
     s->height = (avctx->height + 3) & ~3;
 
+    // dimensions are limited to 16-bit in the format,
+    // so it would be exceedingly unlikely that this would ever be hit outside of fuzzing
+    if (s->height > MAX_HEIGHT) {
+        avpriv_request_sample(s->avctx, "height > UINT16_MAX");
+        return AVERROR_PATCHWELCOME;
+    }
+
     s->sega_film_skip_bytes = -1;  /* uninitialized state */
 
     // check for paletted data
@@ -451,25 +579,11 @@  static int cinepak_decode_frame(AVCodecContext *avctx,
                                 void *data, int *got_frame,
                                 AVPacket *avpkt)
 {
-    const uint8_t *buf = avpkt->data;
-    int ret = 0, buf_size = avpkt->size;
     CinepakContext *s = avctx->priv_data;
-    int num_strips;
-
-    s->data = buf;
-    s->size = buf_size;
-
-    if (s->size < 10)
-        return AVERROR_INVALIDDATA;
-
-    num_strips = AV_RB16 (&s->data[8]);
-
-    //Empty frame, do not waste time
-    if (!num_strips && (!s->palette_video || !av_packet_get_side_data(avpkt, AV_PKT_DATA_PALETTE, NULL)))
-        return buf_size;
+    int ret = 0;
 
-    if ((ret = cinepak_predecode_check(s)) < 0) {
-        av_log(avctx, AV_LOG_ERROR, "cinepak_predecode_check failed\n");
+    if ((ret = cinepak_parse_header(avctx, avpkt)) < 0 ||
+        (ret = cinepak_parse_strip_headers(avctx, avpkt)) < 0) {
         return ret;
     }
 
@@ -500,7 +614,7 @@  static int cinepak_decode_frame(AVCodecContext *avctx,
     *got_frame = 1;
 
     /* report that the buffer was completely consumed */
-    return buf_size;
+    return avpkt->size;
 }
 
 static av_cold int cinepak_decode_end(AVCodecContext *avctx)
diff --git a/tests/ref/fate/cvid-palette b/tests/ref/fate/cvid-palette
index eae41618b5..975778bafb 100644
--- a/tests/ref/fate/cvid-palette
+++ b/tests/ref/fate/cvid-palette
@@ -58,4 +58,3 @@ 
 0,         52,         52,        1,    57600, 0xdae585c7
 0,         53,         53,        1,    57600, 0xf99baa60
 0,         54,         54,        1,    57600, 0x28a8b1ee
-0,         55,         55,        1,    57600, 0xcd5587f8
diff --git a/tests/ref/fate/cvid-partial b/tests/ref/fate/cvid-partial
index 990939db1c..38be650ed5 100644
--- a/tests/ref/fate/cvid-partial
+++ b/tests/ref/fate/cvid-partial
@@ -81,4 +81,3 @@ 
 0,         75,         75,        1,   224400, 0x920ee561
 0,         76,         76,        1,   224400, 0x8a2c1bbf
 0,         77,         77,        1,   224400, 0x6150c072
-0,         78,         78,        1,   224400, 0x499dc869
-- 
2.20.1