On 10/08/2012 4:58 AM, Michael Niedermayer wrote:
> On Thu, Aug 09, 2012 at 08:28:43PM +0200, Reimar Döffinger wrote:
>> On Thu, Aug 09, 2012 at 07:50:04PM +0200, Michael Niedermayer wrote:
>>> On Thu, Aug 09, 2012 at 11:54:56PM +1000, Loune Lam wrote:
>>>> Hi All,
>>>>>>>> Recently been trying VLC on android, and discovered a crash with a
>>>> corrupted DVB mpeg2 ts video. Traced it down to libavcodec/mpeg12.c,
>>>> Snippet mpeg2_fast_decode_block_intra and Snippet
>>>> mpeg2_fast_decode_block_non_intra. Basically they index scantable
>>>> without checking if the bounds. The indexable range maximum seems to
>>>> be 63 so doing a > 63 check fixes the crash. I've got no idea about
>>>> how mpeg decodes, so not sure if that fix is the best. Also, there
>>>> seems to be a few other methods (i.e. mpeg1/non-fast) which suffer
>>>> from this same mistake of not validating the index when accessing
>>>> the scantable. Some of them do have a (i > 63) check (error ac-tex
>>>> damaged at...) but it's after accessing the scantable. I've added a
>>>> range check to all places which looks at the scantable.
>>> the fast methods skip various checks to be fast they arent used unless
>>> explicitly enabled. the fast methods CAN crash but should only crash
>>> harmlessly through reads not writes
>>>>>> If we can make the fast methods more robust without slowing them
>>> down, this is very welcome of course but making them slow is kinda
>>> defeating their purpose
>> I agree. Since this is the second time someone sent this exact patch
>> maybe we should add some comments to the code like "if you have crash
>> problems here don't use 'fast'"?
> agree
>Seems like VLC for android has the fast flag enabled by default. Tried
the non-fast methods and it doesn't crash on my sample. If the
difference between fast and non-fast is not great, then it probably
wouldn't make sense to have yet another variant. Although, I do wonder
about the impact of a simple if statement on the performance. It would
be less confusing if it didn't crash. I guess failing that, a comment
should be added to the place where crashes occur.