[FFmpeg-devel] [PATCH] Moving netgem dvbsubdec fixes in (Part 1)

[Reimar Döffinger]

On Sat, Apr 11, 2009 at 03:03:41PM -0400, Reynaldo H. Verdejo Pinochet wrote:
> As on subject. There is some more context here:
> 
> https://roundup.ffmpeg.org/roundup/ffmpeg/issue536
> 
> I'd be applying this on Monday if OK.

I really don't think this should be applied in one batch.

> Index: libavcodec/dvbsubdec.c
> ===================================================================
> --- libavcodec/dvbsubdec.c	(revision 18443)
> +++ libavcodec/dvbsubdec.c	(working copy)
> @@ -439,27 +439,31 @@
>      int run_length;
>      int pixels_read = 0;
>  
> -    init_get_bits(&gb, *srcbuf, buf_size << 8);
> +    init_get_bits(&gb, *srcbuf, buf_size << 3);
>  
> -    while (get_bits_count(&gb) < (buf_size << 8) && pixels_read < dbuf_len) {
> +    while (get_bits_count(&gb) < (buf_size << 3)) {

The 8 -> 3 is obviously ok, but the pixels_read < dbuf_len should stay

>          bits = get_bits(&gb, 2);
>  
>          if (bits) {
> -            if (non_mod != 1 || bits != 1) {
> -                if (map_table)
> -                    *destbuf++ = map_table[bits];
> -                else
> -                    *destbuf++ = bits;
> +            if (pixels_read < dbuf_len) {
> +                if (non_mod != 1 || bits != 1)  {
> +                    if (map_table)
> +                        *destbuf++ = map_table[bits];
> +                    else
> +                        *destbuf++ = bits;
> +                }
> +                pixels_read++;
>              }
> -            pixels_read++;


Pointless if you just leave the while condition as-is.

>          } else {
>              bits = get_bits1(&gb);
>              if (bits == 1) {
>                  run_length = get_bits(&gb, 3) + 3;
>                  bits = get_bits(&gb, 2);
>  
> -                if (non_mod == 1 && bits == 1)
> -                    pixels_read += run_length;
> +                if (non_mod == 1 && bits == 1)  {
> +                    if (pixels_read < dbuf_len - run_length)
> +                        pixels_read += run_length;
> +                }

The same. You can still clamp pixels_read if you want the function to
never return more than dbuf_len

>                  else {
>                      if (map_table)
>                          bits = map_table[bits];
> @@ -476,8 +480,10 @@
>                          run_length = get_bits(&gb, 4) + 12;
>                          bits = get_bits(&gb, 2);
>  
> -                        if (non_mod == 1 && bits == 1)
> -                            pixels_read += run_length;
> +                        if (non_mod == 1 && bits == 1) {
> +                            if (pixels_read < dbuf_len - run_length)
> +                                pixels_read += run_length;
> +                        }
>                          else {
>                              if (map_table)
>                                  bits = map_table[bits];

The same.

> @@ -490,8 +496,10 @@
>                          run_length = get_bits(&gb, 8) + 29;
>                          bits = get_bits(&gb, 2);
>  
> -                        if (non_mod == 1 && bits == 1)
> -                            pixels_read += run_length;
> +                        if (non_mod == 1 && bits == 1)  {
> +                            if (pixels_read < dbuf_len - run_length)
> +                                pixels_read += run_length;
> +                        }
>                          else {
>                              if (map_table)
>                                  bits = map_table[bits];

The same.

> @@ -501,34 +509,32 @@
>                              }
>                          }
>                      } else if (bits == 1) {
> -                        pixels_read += 2;
>                          if (map_table)
>                              bits = map_table[0];
>                          else
>                              bits = 0;
> -                        if (pixels_read <= dbuf_len) {
> +                        if (pixels_read <= dbuf_len -1) {
>                              *destbuf++ = bits;
>                              *destbuf++ = bits;
> +                            pixels_read += 2;
>                          }

Huh? Looks to me like that introduces a buffer overflow...

>                      } else {
> -                        (*srcbuf) += (get_bits_count(&gb) + 7) >> 3;
> -                        return pixels_read;
> +                        break;

No idea, but certainly must be a separate patch

>                      }
>                  } else {
>                      if (map_table)
>                          bits = map_table[0];
>                      else
>                          bits = 0;
> -                    *destbuf++ = bits;
> -                    pixels_read++;
> +                    if (pixels_read < dbuf_len) {
> +                        *destbuf++ = bits;
> +                        pixels_read++;
> +                    }

Pointless as above.

> -    if (get_bits(&gb, 6))
> -        av_log(0, AV_LOG_ERROR, "DVBSub error: line overflow\n");
> -

Certainly unrelated.