[FFmpeg-devel] [PATCH] Fix buffer over-read in XSUB decoder

[Alexandre Colucci]

On 26 avr. 2011, at 19:17, Reimar Döffinger wrote:

> On Tue, Apr 26, 2011 at 10:58:57AM +0200, Alexandre Colucci wrote:
>> The attached patch fixes a buffer over-read when decoding XSUB subtitles. The rlelen represents the length of the first field only and not the whole buffer. See also xsubenc.c.
> 
> Not that it matters much since that value isn't used as far as I can see.


That value is used by the GetBitContext. If the implementation changes to include range checking, then the current code would fail.


> However to be pedantic I think the below code should be more correct:
> --- a/libavcodec/xsubdec.c
> +++ b/libavcodec/xsubdec.c
> @@ -109,7 +109,11 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
>     bitmap = sub->rects[0]->pict.data[0];
>     for (y = 0; y < h; y++) {
>         // interlaced: do odd lines
> -        if (y == (h + 1) / 2) bitmap = sub->rects[0]->pict.data[0] + w;
> +        if (y == (h + 1) / 2) {
> +            bitmap = sub->rects[0]->pict.data[0] + w;
> +            buf += rlelen;
> +            init_get_bits(&gb, buf, (buf_end - buf) * 8);
> +        }
>         for (x = 0; x < w; ) {
>             int log2 = ff_log2_tab[show_bits(&gb, 8)];
>             int run = get_bits(&gb, 14 - 4 * (log2 >> 1));
> _______________________________________________


Note that this is not a publicly documented format. We have seen test cases where the rlelen was incorrectly set to the size of the whole buffer. In that case your pedantic fix could possibly crash.

Alexandre