[FFmpeg-devel] [HACK] fix CAVS decoder crashes

[Michael Niedermayer]

On Sun, Dec 19, 2010 at 12:56:03PM +0100, Reimar D?ffinger wrote:
> On Sat, Dec 18, 2010 at 02:44:40AM +0100, Michael Niedermayer wrote:
> > On Sun, Dec 12, 2010 at 05:04:58PM +0100, Reimar D?ffinger wrote:
> > > Hello,
> > > I have the suspicion this decoder needs heavy fuzzing testing.
> > > Anyway, trying to play http://samples.mplayerhq.hu/AVS/AVSFileFormat/AVSFileFormat.es
> > > results in crashes which below hack "fixes".
> > > Index: libavcodec/cavs.h
> > > ===================================================================
> > > --- libavcodec/cavs.h   (revision 25928)
> > > +++ libavcodec/cavs.h   (working copy)
> > > @@ -242,6 +242,7 @@
> > >  extern const cavs_vector ff_cavs_dir_mv;
> > >  
> > >  static inline void modify_pred(const int_fast8_t *mod_table, int *mode) {
> > > +    if (*mode < 0) *mode = 0;
> > >      *mode = mod_table[*mode];
> > >      if(*mode < 0) {
> > >          av_log(NULL, AV_LOG_ERROR, "Illegal intra prediction mode\n");
> > > Index: libavcodec/cavsdec.c
> > > ===================================================================
> > > --- libavcodec/cavsdec.c        (revision 25928)
> > > +++ libavcodec/cavsdec.c        (working copy)
> > > @@ -122,7 +122,7 @@
> > >  
> > >      for(i=0;i<65;i++) {
> > >          level_code = get_ue_code(gb,r->golomb_order);
> > > -        if(level_code >= ESCAPE_CODE) {
> > > +        if(level_code >= ESCAPE_CODE || level_code < 0) {
> > >              run = ((level_code - ESCAPE_CODE) >> 1) + 1;
> > >              esc_code = get_ue_code(gb,esc_golomb_order);
> > >              level = esc_code + (run > r->max_run ? 1 : r->level_add[run]);
> > 
> > > @@ -234,7 +234,7 @@
> > >      for(block=0;block<4;block++) {
> > >          d = h->cy + h->luma_scan[block];
> > >          ff_cavs_load_intra_pred_luma(h, top, &left, block);
> > > -        h->intra_pred_l[h->pred_mode_Y[ff_cavs_scan3x3[block]]]
> > > +        h->intra_pred_l[FFMAX(h->pred_mode_Y[ff_cavs_scan3x3[block]], 0)]
> > 
> > as stefan has no time, some comments that might help debuging
> > The intra pred stuff (likely) has to be >= 0 for intra blocks
> > It (likely) is allowed to be negative for non intra blocks
> > i suspect the bug is where this becomes inconsistent (aka intra with negative)
> > a few asserts() might help finding this
> 
> They actually seem to come from uninitialized memory, however valgrind had such
> an insane amount of issue with this code that I first had to fix loads of other stuff
> (most of the input validity checks were just not working, and I doubt I caught all).
> Note that using mallocz is definitely wrong, some of those need to be initialized
> to e.g. -1.
> Index: ffmpeg/libavcodec/cavs.c
> ===================================================================
> --- ffmpeg/libavcodec/cavs.c    (revision 25928)
> +++ ffmpeg/libavcodec/cavs.c    (working copy)
> @@ -577,6 +577,8 @@
>  int ff_cavs_next_mb(AVSContext *h) {
>      int i;
>  
> +    if (get_bits_left(&h->s.gb) < 0)
> +        return 0;
>      h->flags |= A_AVAIL;
>      h->cy += 16;
>      h->cu += 8;
> @@ -653,17 +655,17 @@
>   */
>  void ff_cavs_init_top_lines(AVSContext *h) {
>      /* alloc top line of predictors */
> -    h->top_qp       = av_malloc( h->mb_width);
[...]
> +    h->top_qp       = av_mallocz( h->mb_width);

all uses of top_qp are under B_AVAIL checks so this seems wrong no matter what
value is used in "memset"
ive not checked the other mallocz changes

[...]

> @@ -162,7 +163,7 @@
>      int block;
>  
>      /* get coded block pattern */
> -    int cbp= get_ue_golomb(&h->s.gb);
> +    unsigned cbp= get_ue_golomb(&h->s.gb);
>      if(cbp > 63){
>          av_log(h->s.avctx, AV_LOG_ERROR, "illegal inter cbp\n");
>          return -1;

Its more work to review a type change to make sure it has no effect except the
comparission. Also the return type is signed so one could argue this is
actually wrong



> @@ -187,9 +188,10 @@
>   *
>   ****************************************************************************/
>  
> -static int decode_mb_i(AVSContext *h, int cbp_code) {
> +static int decode_mb_i(AVSContext *h, unsigned cbp_code) {
>      GetBitContext *gb = &h->s.gb;
> -    int block, pred_mode_uv;
> +    int block;
> +    unsigned pred_mode_uv;
>      uint8_t top[18];
>      uint8_t *left = NULL;
>      uint8_t *d;

> @@ -557,11 +559,11 @@
>                  skip_count = -1;
>              if(h->skip_mode_flag && (skip_count < 0))
>                  skip_count = get_ue_golomb(&s->gb);
> -            if(h->skip_mode_flag && skip_count--) {
> +            if(h->skip_mode_flag && skip_count-- > 0) {
>                  decode_mb_p(h,P_SKIP);
>              } else {

This is wrong, skip_count must be checked for being valid (<=mb_count id guess)


>                  mb_type = get_ue_golomb(&s->gb) + P_SKIP + h->skip_mode_flag;
> -                if(mb_type > P_8X8)
> +                if((unsigned)mb_type > P_8X8)
>                      decode_mb_i(h, mb_type - P_8X8 - 1);
>                  else
>                      decode_mb_p(h,mb_type);

This is a convoluted hack and undocumented too
mb_type should be checked for being valid here and not invalid values being
passed into decode_mb_i() that then detects them checking the cbp and printing
a cbp error while it should be a mb_type error.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101219/b0605707/attachment.pgp>