[FFmpeg-devel] [PATCH] Rewrite emu_edge functions to have separate src/dst_stride arguments.

[Michael Niedermayer]

On Fri, Sep 27, 2013 at 08:15:13PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Sep 24, 2013 at 5:54 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Fri, Sep 20, 2013 at 08:03:00AM -0400, Ronald S. Bultje wrote:
> > > From: "Ronald S. Bultje" <rsbultje at gmail.com>
> > >
> > > This allows supporting files for which the image stride is smaller than
> > > the max. block size + number of subpel mc taps, e.g. a 64x64 VP9 file
> > > or a 16x16 VP8 file with -fflags +emu_edge.
> > > ---
> > >  libavcodec/cavs.c              |   7 +-
> > >  libavcodec/diracdec.c          |   3 +-
> > >  libavcodec/h264.c              |  10 +-
> > >  libavcodec/mpegvideo.c         |  17 +-
> > >  libavcodec/mpegvideo_enc.c     |   6 +-
> > >  libavcodec/mpegvideo_motion.c  |  74 ++--
> > >  libavcodec/rv34.c              |  15 +-
> > >  libavcodec/snow.c              |   4 +-
> > >  libavcodec/svq3.c              |   6 +-
> > >  libavcodec/vc1dec.c            |  47 +--
> > >  libavcodec/videodsp.h          |  20 +-
> > >  libavcodec/videodsp_template.c |  33 +-
> > >  libavcodec/vp3.c               |   5 +-
> > >  libavcodec/vp56.c              |   2 +-
> > >  libavcodec/vp8.c               |  38 +-
> > >  libavcodec/vp8.h               |   2 +-
> > >  libavcodec/wmv2.c              |  16 +-
> > >  libavcodec/x86/dsputil_mmx.c   |   7 +-
> > >  libavcodec/x86/videodsp.asm    | 840
> > ++++++++++++++++-------------------------
> > >  libavcodec/x86/videodsp_init.c | 182 +++++++--
> > >  20 files changed, 658 insertions(+), 676 deletions(-)
> >
> > this patch causes segfaults with crafted bitstreams
> > you can simulate such bitstream by:
> > --- a/libavcodec/ituh263dec.c
> > +++ b/libavcodec/ituh263dec.c
> > @@ -325,6 +325,12 @@ static int h263p_decode_umotion(MpegEncContext * s,
> > int pred)
> >     code >>= 1;
> >
> >     code = (sign) ? (pred - code) : (pred + code);
> > +
> > +   static unsigned r=1;
> > +   r+= 12345;
> > +   r*= 11111;
> > +   code = r;
> > +
> >     av_dlog(s->avctx,"H.263+ UMV Motion = %d\n", code);
> >     return code;
> >
> > ----------
> > similar issues can likely be triggered with other decoders calling
> > the emu edge code
> >
> >
> > ==23430== Invalid read of size 8
> > ==23430==    at 0xB35060: ??? (videodsp.asm:325)
> > ==23430==    by 0xA780D0: emulated_edge_mc_sse (videodsp_init.c:175)
> > ==23430==    by 0x8DA9BD: mpeg_motion (mpegvideo_motion.c:296)
> > ==23430==    by 0x8DC825: ff_MPV_motion (mpegvideo_motion.c:872)
> > ==23430==    by 0x8C4F11: ff_MPV_decode_mb (mpegvideo.c:2778)
> > ==23430==    by 0x6F536F: decode_slice (h263dec.c:261)
> > ==23430==    by 0x6F63F8: ff_h263_decode_frame (h263dec.c:688)
> > ==23430==    by 0x9A1F4A: avcodec_decode_video2 (utils.c:1994)
> > ==23430==    by 0x48228C: output_packet (ffmpeg.c:1668)
> > ==23430==    by 0x4855FF: process_input (ffmpeg.c:3089)
> > ==23430==    by 0x47426D: main (ffmpeg.c:3185)
> > ==23430==  Address 0x11112591b0 is not stack'd, malloc'd or (recently)
> > free'd
> > ==23430==
> > ==23430==
> > ==23430== Process terminating with default action of signal 11 (SIGSEGV)
> > ==23430==  Access not within mapped region at address 0x11112591B0
> > ==23430==    at 0xB35060: ??? (videodsp.asm:325)
> > ==23430==    by 0xA780D0: emulated_edge_mc_sse (videodsp_init.c:175)
> > ==23430==    by 0x8DA9BD: mpeg_motion (mpegvideo_motion.c:296)
> > ==23430==    by 0x8DC825: ff_MPV_motion (mpegvideo_motion.c:872)
> > ==23430==    by 0x8C4F11: ff_MPV_decode_mb (mpegvideo.c:2778)
> > ==23430==    by 0x6F536F: decode_slice (h263dec.c:261)
> > ==23430==    by 0x6F63F8: ff_h263_decode_frame (h263dec.c:688)
> > ==23430==    by 0x9A1F4A: avcodec_decode_video2 (utils.c:1994)
> > ==23430==    by 0x48228C: output_packet (ffmpeg.c:1668)
> > ==23430==    by 0x4855FF: process_input (ffmpeg.c:3089)
> > ==23430==    by 0x47426D: main (ffmpeg.c:3185)
> 
> 
> Attached patch will prevent that. Feel free to merge or apply before or
> after the original patch.
> 
> Ronald

>  cavs.h             |    2 +-
>  diracdec.c         |    2 +-
>  h264.c             |    2 +-
>  h264.h             |    6 +++---
>  mpegvideo.c        |    9 +++++----
>  mpegvideo.h        |    4 ++--
>  mpegvideo_enc.c    |    4 ++--
>  mpegvideo_motion.c |   11 +++++++----
>  snow.c             |    2 +-
>  snow.h             |    2 +-
>  videodsp.h         |    2 --
>  vp3.c              |    2 +-
>  vp56.c             |    2 +-
>  vp8.c              |    4 ++--
>  wmv2.c             |    3 ++-
>  15 files changed, 30 insertions(+), 27 deletions(-)
> 49eb5d507041321a6f2e4626ca03772f16d4b6dc  0001-Convert-multiplier-for-MV-from-int-to-ptrdiff_t.patch
> From 1545a9e566f6986f380864e5d1ec724c54058735 Mon Sep 17 00:00:00 2001
> From: "Ronald S. Bultje" <rsbultje at gmail.com>
> Date: Fri, 27 Sep 2013 20:13:59 -0400
> Subject: [PATCH] Convert multiplier for MV from int to ptrdiff_t.
> 
> This prevents emulated_edge_mc from not undoing mvy*stride-related
> integer overflows.

applied

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20130928/69a3b641/attachment.asc>