Opened 3 months ago

Last modified 3 months ago

#6626 new defect

ff_vdpau_common_init writes past the end of av_alloc_vdpaucontext memory

Reported by: aaronp24 Owned by:
Priority: normal Component: undetermined
Version: unspecified Keywords:
Cc: sw@jkqxz.net Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Summary of the bug:
libavcodec writes past the end of data it allocated, when mplayer calls ff_vdpau_common_init() without calling av_vdpau_bind_context()

How to reproduce:

% valgrind mplayer -vc ffh264vdpau <any H.264 video>
ffmpeg version 1:3.3.3-2 from Arch Linux

Details:
I'm looking into a bug where mplayer -vc ffh264vdpau causes libavcodec to write past the end of an allocation and corrupt data in the vdpau driver. According to Valgrind,

VO: [vdpau] 1280x536 => 1280x536 H.264 VDPAU acceleration 
==19159== Invalid write of size 1
==19159==    at 0xAF0326F: ff_vdpau_common_init (vdpau.c:145)
==19159==    by 0xAEA9B89: setup_hwaccel (utils.c:1100)
==19159==    by 0xAEA9B89: ff_get_format (utils.c:1165)
==19159==    by 0xAB4C365: get_pixel_format.isra.6 (h264_slice.c:864)
==19159==    by 0xAB4D1D2: h264_init_ps (h264_slice.c:1102)
==19159==    by 0xAB4D1D2: h264_field_start.isra.7 (h264_slice.c:1385)
==19159==    by 0xAB520B7: ff_h264_queue_decode_slice (h264_slice.c:2091)
==19159==    by 0xAB55BBE: decode_nal_units (h264dec.c:674)
==19159==    by 0xAB55BBE: h264_decode_frame (h264dec.c:1016)
==19159==    by 0xAEA58AD: avcodec_decode_video2 (utils.c:2275)
==19159==    by 0x334BB2: ??? (in /usr/bin/mplayer)
==19159==    by 0x262000: ??? (in /usr/bin/mplayer)
==19159==    by 0x1B2A57: ??? (in /usr/bin/mplayer)
==19159==    by 0x1B4A8D: ??? (in /usr/bin/mplayer)
==19159==    by 0x1B8A06: ??? (in /usr/bin/mplayer)
==19159==  Address 0x2701c8c8 is 16 bytes after a block of size 760 alloc'd
==19159==    at 0x4C2E2A6: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19159==    by 0x4C2E3E2: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19159==    by 0xBF640FB: av_malloc (mem.c:87)
==19159==    by 0xBF642C9: av_mallocz (mem.c:224)
==19159==    by 0x332D7C: ??? (in /usr/bin/mplayer)
==19159==    by 0x333CEA: ??? (in /usr/bin/mplayer)
==19159==    by 0x333F1A: ??? (in /usr/bin/mplayer)
==19159==    by 0x335656: ??? (in /usr/bin/mplayer)
==19159==    by 0xAEA9AF1: ff_get_format (utils.c:1140)
==19159==    by 0xAB4C365: get_pixel_format.isra.6 (h264_slice.c:864)
==19159==    by 0xAB4D1D2: h264_init_ps (h264_slice.c:1102)
==19159==    by 0xAB4D1D2: h264_field_start.isra.7 (h264_slice.c:1385)
==19159==    by 0xAB520B7: ff_h264_queue_decode_slice (h264_slice.c:2091)

I tracked down the problem to mplayer doing this:

avctx->hwaccel_context = vdpc = av_alloc_vdpaucontext();

where av_alloc_vdpaucontext() does this:

return av_mallocz(sizeof(AVVDPAUContext));

However, ff_vdpau_common_init() casts the hwaccel_context pointer to a VDPAUHWContext*, which is a larger structure that contains an AVVDPAUContext as its first element. When this function tries to write the reset field, it writes past the end of the data allocated by av_alloc_vdpaucontext().

hwctx->reset            = 0;

It looks like there is code in av_vdpau_bind_context() to reallocate this structure to the larger size:

VDPAUHWContext *hwctx;
if (av_reallocp(&avctx->hwaccel_context, sizeof(*hwctx)))
    return AVERROR(ENOMEM);

However, mplayer doesn't call this function, so the realloc never happens.

According to the git history, there used to be a call to av_vdpau_bind_context() from vdpau_alloc() (called from vdpau_init()) but Mark Thompson removed that code in commit e462ace84b92e54d2a5fa651d6469aefe0f1efbf.

What is the proper fix here? Should mplayer be calling av_vdpau_bind_context(), or should av_alloc_vdpaucontext() be allocating enough space up front?

Change History (3)

comment:1 Changed 3 months ago by jkqxz

The change you point at removed the use of the old API from the ffmpeg utility, and did not affect libavcodec at all.

In the old API, I think av_vdpau_bind_context() must be called to give the VDPAU device to libavcodec before returning AV_PIX_FMT_VDPAU from get_format().

comment:2 Changed 3 months ago by aaronp24

Oh, sorry I didn't notice that code was outside libavcodec. But that brings up the fact that now ffmpeg doesn't call av_vdpau_bind_context() either.

comment:3 Changed 3 months ago by jkqxz

ffmpeg uses the generic hwaccel API by setting hw_device_ctx, so it doesn't need to do anything VDPAU-specific like calling av_vdpau_bind_context(). AVCodecContext.hwaccel_context is not used at all.

Note: See TracTickets for help on using tickets.