Opened 7 years ago

Last modified 6 years ago

#6626 new defect

ff_vdpau_common_init writes past the end of av_alloc_vdpaucontext memory

Reported by: Aaron Plattner 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 (5)

comment:1 by jkqxz, 7 years ago

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 by Aaron Plattner, 7 years ago

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 by jkqxz, 7 years ago

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.

comment:4 by Peter Bennett, 6 years ago

MythTV is getting a segmentation fault due to memory corruption caused by this:

AVCodecContext.hwaccel_context is supposed to point to an AVVDPAUContext as per the documentation of AVVDPAUContext. The user application allocates this using av_vdpau_alloc_context according to that documentation.
However - in FFmpeg/libavcodec/vdpau.c, in ff_vdpau_common_init it assumes that AVCodecContext.hwaccel_context contains a pointer to VDPAUHWContext. VDPAUHWContext is a structure than contains an AVVDPAUContext plus other stuff and is therefore longer than AVVDPAUContext. ff_vdpau_common_init then proceeds to set the field VDPAUHWContext::reset to zero. This field is beyond the allocated data of AVVDPAUContext and therefore overwrites other storage in the caller, causing a segmentation fault. Note that av_vdpau_alloc_context only allocates enough memory for AVVDPAUContext and therefore not enough for VDPAUHWContext.

My workaround in MythTV - use structure VDPAUHWContext to determine the size of memory to allocate and do not use av_vdpau_alloc_context.

comment:5 by Hendrik, 6 years ago

Your proper fix should be calling av_vdpau_bind_context, as mentioned earliner in this ticket. Basically, anyone that uses the old VDPAU API is using it wrong if you're not calling that function.

An even better fix would be to switch to the new API - or even switch to NVDEC, since VDPAU is basically dead.

Note: See TracTickets for help on using tickets.