Prevent crash with bogus VC-1 display extension info

Message ID BANLkTikqkDu2WrGVc3-Svt3jOpMPc=ayug@mail.gmail.com
State New
Headers show

Commit Message

Alberto Delmás April 13, 2011, 5:45 p.m.
2011/4/13 Kostya <kostya.shishkov@gmail.com>:
> I've seen display information used for cropping - e.g. adjust picture
> dimensions for odd dimensions.

The wording on the spec isn't very clear, but it sounds to me like it is
not intended to crop.

In any case, mpegvideo does not currently support arbitrary cropping, so
the options are:

* Use it for aspect ratio only (as in the patch)
* Use it for limited cropping (less than 16 pixels)
* Ignore it completely

Here's an updated patch that also checks the entry point dimensions not
to be larger than the ones specified in the sequence header. The spec
says they can't be larger, and they can cause a crash too (uploaded
sample to incoming/VC1_EPH_crash.wmv)
From df8c9ad4c7789712b8eba60cbd183b8cfc912b83 Mon Sep 17 00:00:00 2001
From: Alberto Delmas <adelmas@gmail.com>
Date: Wed, 13 Apr 2011 19:10:51 +0200
Subject: [PATCH] Prevent crashes with bogus VC-1 entry point or display extension info

---
 libavcodec/vc1.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

Comments

Kostya Shishkov April 13, 2011, 8:28 p.m. | #1
On Wed, Apr 13, 2011 at 07:45:01PM +0200, Alberto Delmás wrote:
> 2011/4/13 Kostya <kostya.shishkov@gmail.com>:
> > I've seen display information used for cropping - e.g. adjust picture
> > dimensions for odd dimensions.
> 
> The wording on the spec isn't very clear, but it sounds to me like it is
> not intended to crop.
> 
> In any case, mpegvideo does not currently support arbitrary cropping, so
> the options are:
> 
> * Use it for aspect ratio only (as in the patch)
> * Use it for limited cropping (less than 16 pixels)
> * Ignore it completely
> 
> Here's an updated patch that also checks the entry point dimensions not
> to be larger than the ones specified in the sequence header. The spec
> says they can't be larger, and they can cause a crash too (uploaded
> sample to incoming/VC1_EPH_crash.wmv)

probably OK, I trust Ronald to test it on random samples from VC-1 testsuite
Ronald Bultje April 13, 2011, 8:29 p.m. | #2
Hi,

On Wed, Apr 13, 2011 at 4:28 PM, Kostya <kostya.shishkov@gmail.com> wrote:
> On Wed, Apr 13, 2011 at 07:45:01PM +0200, Alberto Delmás wrote:
>> 2011/4/13 Kostya <kostya.shishkov@gmail.com>:
>> > I've seen display information used for cropping - e.g. adjust picture
>> > dimensions for odd dimensions.
>>
>> The wording on the spec isn't very clear, but it sounds to me like it is
>> not intended to crop.
>>
>> In any case, mpegvideo does not currently support arbitrary cropping, so
>> the options are:
>>
>> * Use it for aspect ratio only (as in the patch)
>> * Use it for limited cropping (less than 16 pixels)
>> * Ignore it completely
>>
>> Here's an updated patch that also checks the entry point dimensions not
>> to be larger than the ones specified in the sequence header. The spec
>> says they can't be larger, and they can cause a crash too (uploaded
>> sample to incoming/VC1_EPH_crash.wmv)
>
> probably OK, I trust Ronald to test it on random samples from VC-1 testsuite

Will do, but only this weekend. Please be patient, Alberto, sorry in advance.

Ronald
Diego Biurrun June 15, 2011, 10:25 a.m. | #3
On Wed, Apr 13, 2011 at 04:29:56PM -0400, Ronald S. Bultje wrote:
> 
> On Wed, Apr 13, 2011 at 4:28 PM, Kostya <kostya.shishkov@gmail.com> wrote:
> > On Wed, Apr 13, 2011 at 07:45:01PM +0200, Alberto Delmás wrote:
> >> 2011/4/13 Kostya <kostya.shishkov@gmail.com>:
> >> > I've seen display information used for cropping - e.g. adjust picture
> >> > dimensions for odd dimensions.
> >>
> >> The wording on the spec isn't very clear, but it sounds to me like it is
> >> not intended to crop.
> >>
> >> In any case, mpegvideo does not currently support arbitrary cropping, so
> >> the options are:
> >>
> >> * Use it for aspect ratio only (as in the patch)
> >> * Use it for limited cropping (less than 16 pixels)
> >> * Ignore it completely
> >>
> >> Here's an updated patch that also checks the entry point dimensions not
> >> to be larger than the ones specified in the sequence header. The spec
> >> says they can't be larger, and they can cause a crash too (uploaded
> >> sample to incoming/VC1_EPH_crash.wmv)
> >
> > probably OK, I trust Ronald to test it on random samples from VC-1 testsuite
> 
> Will do, but only this weekend. Please be patient, Alberto, sorry in advance.

ping

Diego

Patch

diff --git a/libavcodec/vc1.c b/libavcodec/vc1.c
index b058a38..c9bc1c4 100644
--- a/libavcodec/vc1.c
+++ b/libavcodec/vc1.c
@@ -503,10 +503,10 @@  static int decode_sequence_header_adv(VC1Context *v, GetBitContext *gb)
     if(get_bits1(gb)) { //Display Info - decoding is not affected by it
         int w, h, ar = 0;
         av_log(v->s.avctx, AV_LOG_DEBUG, "Display extended info:\n");
-        v->s.avctx->width  = w = get_bits(gb, 14) + 1;
-        v->s.avctx->height = h = get_bits(gb, 14) + 1;
+        w = get_bits(gb, 14) + 1;
+        h = get_bits(gb, 14) + 1;
         av_log(v->s.avctx, AV_LOG_DEBUG, "Display dimensions: %ix%i\n", w, h);
-        if(get_bits1(gb))
+        if(get_bits1(gb)){
             ar = get_bits(gb, 4);
         if(ar && ar < 14){
             v->s.avctx->sample_aspect_ratio = ff_vc1_pixel_aspect[ar];
@@ -515,6 +515,13 @@  static int decode_sequence_header_adv(VC1Context *v, GetBitContext *gb)
             h = get_bits(gb, 8);
             v->s.avctx->sample_aspect_ratio = (AVRational){w, h};
         }
+        }else{
+            av_reduce(&v->s.avctx->sample_aspect_ratio.num,
+                      &v->s.avctx->sample_aspect_ratio.den,
+                      v->s.avctx->height * w,
+                      v->s.avctx->width * h,
+                      1<<30);
+        }
         av_log(v->s.avctx, AV_LOG_DEBUG, "Aspect: %i:%i\n", v->s.avctx->sample_aspect_ratio.num, v->s.avctx->sample_aspect_ratio.den);
 
         if(get_bits1(gb)){ //framerate stuff
@@ -577,8 +584,15 @@  int vc1_decode_entry_point(AVCodecContext *avctx, VC1Context *v, GetBitContext *
     }
 
     if(get_bits1(gb)){
-        avctx->coded_width = (get_bits(gb, 12)+1)<<1;
-        avctx->coded_height = (get_bits(gb, 12)+1)<<1;
+        int w, h;
+        w = (get_bits(gb, 12)+1)<<1;
+        h = (get_bits(gb, 12)+1)<<1;
+        if(w <= avctx->width && h <= avctx->height){
+            avctx->coded_width = w;
+            avctx->coded_height = h;
+        }else{
+            av_log(avctx, AV_LOG_ERROR, "Entry point coded frame size too large\n");
+        }
     }
     if(v->extended_mv)
         v->extended_dmv = get_bits1(gb);