[FFmpeg-devel] [PATCH] Fix mpeg1/2 scantable overflow

[Michael Niedermayer]

On Sun, Aug 12, 2012 at 12:33:03AM +1000, Loune Lam wrote:
> 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.

patches that add such comments are welcome!

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120811/c2ac2758/attachment.asc>