[FFmpeg-devel] [PATCH] More correct and robust Cinepak decoder.

[u-bo1b at 0w.se]

This is a patch on top of "correct colorspace in Cinepak" one.

The decoder (even the old one) could be tricked into corrupting memory,
this change is meant to prevent this, by more consequently constraining
the dimensions.

Another change is treating of strip y coordinates which previously did
not follow the description (nor did it behave like the binary decoder
on files with absolute strip offsets).

Besides this, a small optimization was made in the y-non-multiple-of-4
scan row check.

Cheers,
Rl
---
 libavcodec/cinepak.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index c537761..5b1bf25 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -140,6 +140,7 @@ static int cinepak_decode_vectors (CinepakContext *s, cvid_strip *strip,
     uint32_t         flag, mask;
     uint8_t         *cb0, *cb1, *cb2, *cb3;
     unsigned int     x, y;
+    int              remaining_height;
     char            *ip0, *ip1, *ip2, *ip3;
 
     flag = 0;
@@ -150,11 +151,12 @@ static int cinepak_decode_vectors (CinepakContext *s, cvid_strip *strip,
 /* take care of y dimension not being multiple of 4, such streams exist */
         ip0 = ip1 = ip2 = ip3 = s->frame.data[0] +
           (s->palette_video?strip->x1:strip->x1*3) + (y * s->frame.linesize[0]);
-        if(s->avctx->height - y > 1) {
+        remaining_height = s->avctx->height - y - 1;
+        if(remaining_height-- > 0) {
             ip1 = ip0 + s->frame.linesize[0];
-            if(s->avctx->height - y > 2) {
+            if(remaining_height-- > 0) {
                 ip2 = ip1 + s->frame.linesize[0];
-                if(s->avctx->height - y > 3) {
+                if(remaining_height > 0) {
                     ip3 = ip2 + s->frame.linesize[0];
                 }
             }
@@ -373,10 +375,13 @@ static int cinepak_decode (CinepakContext *s)
             return AVERROR_INVALIDDATA;
 
         s->strips[i].id = s->data[0];
-        s->strips[i].y1 = y0;
-        s->strips[i].x1 = 0;
-        s->strips[i].y2 = y0 + AV_RB16 (&s->data[8]);
-        s->strips[i].x2 = s->avctx->width;
+/* 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]);
+        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]);
 
         if (s->strips[i].id == 0x10)
             s->frame.key_frame = 1;
@@ -400,7 +405,7 @@ static int cinepak_decode (CinepakContext *s)
             return result;
 
         s->data += strip_size;
-        y0    = s->strips[i].y2;
+        y0 = s->strips[i].y2;
     }
     return 0;
 }
@@ -410,8 +415,12 @@ static av_cold int cinepak_decode_init(AVCodecContext *avctx)
     CinepakContext *s = avctx->priv_data;
 
     s->avctx = avctx;
-    s->width = (avctx->width + 3) & ~3;
-    s->height = (avctx->height + 3) & ~3;
+/* we do not really support x dimension not being multiple of 4
+ * and we do not want to risk to refer outside the frame memory */
+    s->width = (avctx->width) & ~3;
+/* y dimension not being a multiple of 4 is no problem, thus we allow
+ * strips to slightly exceed the frame size */
+    s->height = (avctx->height+3) & ~3;
     s->sega_film_skip_bytes = -1;  /* uninitialized state */
 
     // check for paletted data
-- 
1.5.4