[FFmpeg-devel] [PATCH]Fix progressive jpgs with weird pix_fmts

[Carl Eugen Hoyos]

Hi!

On Saturday 07 January 2012 03:21:36 pm Michael Niedermayer wrote:
> On Sat, Jan 07, 2012 at 12:26:29PM +0100, Carl Eugen Hoyos wrote:
> > On Saturday 07 January 2012 03:27:52 am Michael Niedermayer wrote:
> > > On Sat, Jan 07, 2012 at 03:17:36AM +0100, Carl Eugen Hoyos wrote:
> > > > On Saturday 07 January 2012 02:59:06 am Michael Niedermayer wrote:
> > > > > > Attached fixes the samples from ticket #892 for me.
> > > > > >
> > > > > > Please comment, Carl Eugen
> > > > >
> > > > > reset upscale* otherwise this is possibly exploitable if the width
> > > > > or height or "pix_fmt" changes
> > > >
> > > > As in attached?
> > >
> > > not enough
> > >
> > > 1st frame sets upscale_h
> > > 2nd frame changes width/height and branches via -1 from
> > >  ff_mjpeg_find_marker to the_end
> >
> > Iiuc, width/height can only be set in ff_mjpeg_decode_sof(), so new patch
> > resets the value at the beginning of this function.
> >
> > > also wherever the variables are reset i suggest that a av_assert0 is
> > > added to make sure the pixel format matches the expectation that is
> > > theres enough space for chroma
> >
> > I don't understand:
> > If the variables are reset (to 0), the code that may overwrite chroma /
> > corrupt memory is not called, the variables are set together with the
> > pix_fmt.
> >
> > Only s->ls change the pix_fmt later, upscale now also gets reset in this
> > case.
> 
> my concern is if the code is changed in the future. A new code path
> could be added that changes width/height and forgets reseting these
> variables

Thank you for the explanation.

New patch attached.

Please comment, Carl Eugen
-------------- next part --------------
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 458d132..64e5839 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -219,6 +219,7 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
     int len, nb_components, i, width, height, pix_fmt_id;
 
     s->cur_scan = 0;
+    s->upscale_h = s->upscale_v = 0;
 
     /* XXX: verify len field validity */
     len     = get_bits(&s->gb, 16);
@@ -401,6 +402,7 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
         return -1;
     }
     if (s->ls) {
+        s->upscale_h = s->upscale_v = 0;
         if (s->nb_components > 1)
             s->avctx->pix_fmt = PIX_FMT_RGB24;
         else if (s->bits <= 8)
@@ -1233,28 +1235,6 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
                 return -1;
         }
     }
-    if (s->upscale_h) {
-        uint8_t *line = s->picture_ptr->data[s->upscale_h];
-        for (i = 0; i < s->chroma_height; i++) {
-            for (index = s->width - 1; index; index--)
-                line[index] = (line[index / 2] + line[(index + 1) / 2]) >> 1;
-            line += s->linesize[s->upscale_h];
-        }
-    }
-    if (s->upscale_v) {
-        uint8_t *dst = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[(s->height - 1) * s->linesize[s->upscale_v]];
-        for (i = s->height - 1; i; i--) {
-            uint8_t *src1 = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[i / 2 * s->linesize[s->upscale_v]];
-            uint8_t *src2 = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[(i + 1) / 2 * s->linesize[s->upscale_v]];
-            if (src1 == src2) {
-                memcpy(dst, src1, s->width);
-            } else {
-                for (index = 0; index < s->width; index++)
-                    dst[index] = (src1[index] + src2[index]) >> 1;
-            }
-            dst -= s->linesize[s->upscale_v];
-        }
-    }
     emms_c();
     return 0;
  out_of_range:
@@ -1576,6 +1556,7 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
     const uint8_t *unescaped_buf_ptr;
     int unescaped_buf_size;
     int start_code;
+    int i, index;
     AVFrame *picture = data;
 
     s->got_picture = 0; // picture from previous image can not be reused
@@ -1735,6 +1716,36 @@ eoi_parser:
     av_log(avctx, AV_LOG_FATAL, "No JPEG data found in image\n");
     return -1;
 the_end:
+    if (s->upscale_h) {
+        uint8_t *line = s->picture_ptr->data[s->upscale_h];
+        av_assert0(avctx->pix_fmt == PIX_FMT_YUVJ444P ||
+                   avctx->pix_fmt == PIX_FMT_YUV444P  ||
+                   avctx->pix_fmt == PIX_FMT_YUVJ440P ||
+                   avctx->pix_fmt == PIX_FMT_YUV440P);
+        for (i = 0; i < s->chroma_height; i++) {
+            for (index = s->width - 1; index; index--)
+                line[index] = (line[index / 2] + line[(index + 1) / 2]) >> 1;
+            line += s->linesize[s->upscale_h];
+        }
+    }
+    if (s->upscale_v) {
+        uint8_t *dst = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[(s->height - 1) * s->linesize[s->upscale_v]];
+        av_assert0(avctx->pix_fmt == PIX_FMT_YUVJ444P ||
+                   avctx->pix_fmt == PIX_FMT_YUV444P  ||
+                   avctx->pix_fmt == PIX_FMT_YUVJ422P ||
+                   avctx->pix_fmt == PIX_FMT_YUV422P);
+        for (i = s->height - 1; i; i--) {
+            uint8_t *src1 = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[i / 2 * s->linesize[s->upscale_v]];
+            uint8_t *src2 = &((uint8_t *)s->picture_ptr->data[s->upscale_v])[(i + 1) / 2 * s->linesize[s->upscale_v]];
+            if (src1 == src2) {
+                memcpy(dst, src1, s->width);
+            } else {
+                for (index = 0; index < s->width; index++)
+                    dst[index] = (src1[index] + src2[index]) >> 1;
+            }
+            dst -= s->linesize[s->upscale_v];
+        }
+    }
     av_log(avctx, AV_LOG_DEBUG, "mjpeg decode frame unused %td bytes\n",
            buf_end - buf_ptr);
 //  return buf_end - buf_ptr;