Opened 7 months ago

Closed 7 months ago

#13428 closed Patch - Bug Fix (fixed)

Valgrind error in avformatdecoder.cpp

Reported by: Klaas de Waal Owned by: Klaas de Waal <kdewaal@…>
Priority: minor Milestone: 31.0
Component: MythTV - General Version: Master Head
Severity: low Keywords: valgrind
Cc: Stuart Auchterlonie Ticket locked: no

Description

Running mythfrontend with valgrind gives the following error message:

==7659== Conditional jump or move depends on uninitialised value(s)
==7659==    at 0x49627BC: h261_probe (h261dec.c:35)
==7659==    by 0x4959CF4: av_probe_input_format3 (format.c:171)
==7659==    by 0x4959F81: av_probe_input_format2 (format.c:225)
==7659==    by 0x69424DF: AvFormatDecoder::CanHandle(char*, QString const&, int) (avformatdecoder.cpp:961)
==7659==    by 0x68B0CDB: MythPlayer::CreateDecoder(char*, int) (mythplayer.cpp:923)
==7659==    by 0x68B16C8: MythPlayer::OpenFile(unsigned int) (mythplayer.cpp:989)
...
==7659==  Uninitialised value was created by a heap allocation
==7659==    at 0x4839593: operator new[](unsigned long) (vg_replace_malloc.c:433)
==7659==    by 0x68B0F73: MythPlayer::OpenFile(unsigned int) (mythplayer.cpp:958)
...

What happens is that mythplayer.cpp allocates 256kB of buffer memory and fill this with at least 2kB (testreadsize) bytes (line 958). This is passed on to AvFormatDecoder::CanHandle?. There it makes sure that there is at least AVPROBE_PADDING_SIZE (32 bytes) of unused memory available in the buffer. This is memory which has not initialized. According to valgrind this memory is then used in h261_probe and vc1_probe.

There are two places in avformatdecoder.cpp where this happens; in CanHandle? and in OpenFile?. The solution is to clear the 32-byte padding memory in both places.

A patch that implements this solution is attached.

Note: it would also be possible to clear the complete buffer immediately after allocation. However, this means clearing 256kB instead of only 32 bytes and thus comes with a (minor) performance penalty.

Attachments (1)

20190319-avformat-memset.patch (865 bytes) - added by Klaas de Waal 7 months ago.
Clear padding memory at end of video data before probing video format.

Download all attachments as: .zip

Change History (2)

Changed 7 months ago by Klaas de Waal

Clear padding memory at end of video data before probing video format.

comment:1 Changed 7 months ago by Klaas de Waal <kdewaal@…>

Owner: set to Klaas de Waal <kdewaal@…>
Resolution: fixed
Status: newclosed

In eff14d9d1/mythtv:

Uninitialized memory access in video codec probes.

Valgrind detects uninitialized memory access in a memory buffer; the access is inside the buffer itself but beyond the size to which the buffer is written.
This access is done by FFmpeg video type probing routines. This could be viewed as a bug in these routines but the MythTV code does reserve 32 bytes of additional space for this so it is most likely a requirement for using these routines. However, these 32 bytes are never initialized. This is solved by doing a "memset" to clear those 32 bytes.

Fixes #13428

Note: See TracTickets for help on using tickets.