[FFmpeg-devel] [PATCH 2/2] Do not fail DVB sub decoding because of a few padding bytes

[Justin Ruggles]

On 02/10/2011 02:13 PM, Reimar D?ffinger wrote:

> On Wed, Feb 09, 2011 at 07:02:43PM +0000, M?ns Rullg?rd wrote:
>> Justin Ruggles <justin.ruggles at gmail.com> writes:
>>> On 02/09/2011 01:32 PM, Reimar D?ffinger wrote:
>>>
>>>> Instead of returning an error when bytes are left over, just return
>>>> the number of actually used bytes as other decoders do.
>>>> Instead add a special case so an error will be returned when none
>>>> of the data looks valid to avoid making debugging a pain.
>>>> ---
>>>>  libavcodec/dvbsubdec.c |    9 ++-------
>>>>  1 files changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
>>>> index 8cc8d4f..401144f 100644
>>>> --- a/libavcodec/dvbsubdec.c
>>>> +++ b/libavcodec/dvbsubdec.c
>>>> @@ -1423,7 +1423,7 @@ static int dvbsub_decode(AVCodecContext *avctx,
>>>>  
>>>>  #endif
>>>>  
>>>> -    if (buf_size <= 2)
>>>> +    if (buf_size <= 2 || *buf != 0x0f)
>>>>          return -1;
>>>>  
>>>>      p = buf;
>>>> @@ -1467,12 +1467,7 @@ static int dvbsub_decode(AVCodecContext *avctx,
>>>>          p += segment_length;
>>>>      }
>>>>  
>>>> -    if (p != p_end) {
>>>> -        av_dlog(avctx, "Junk at end of packet\n");
>>>> -        return -1;
>>>> -    }
>>>> -
>>>> -    return buf_size;
>>>> +    return p - buf;
>>>>  }
>>>
>>> This looks fine.  Does the decoder still work ok if the first byte of
>>> the "junk" happens to be 0xF?
>>
>> Doesn't look like it to me.  If the first byte is 0x0f, it immediately
>> proceeds to read 6 bytes from the packet, two of which are used
>> unchecked as a 16-bit size value.  Even without this patch, this code
>> could easily over-read the buffer if fed with bad data.
> 
> The only thing that changes is the return value, nothing else.
> Instead of the first part of the patch I could have changed the last one
> to
> return p == buf ? -1 : p - buf;
> but I considered the method in the patch to be better to understand in the long term.


Sorry, I was getting confused with audio decoding, which will call
decode() again with the remaining bytes.  This does not appear to be the
case with subtitles.  The decoder still needs some more error
resilience, but the patch looks ok as-is.

-Justin