[issue2392] Memory leak in mpegts demuxer on samples with too large PES packets

[Joakim Plate]

New submission from Joakim Plate <***@ecce.se>:

valgrind --tool=memcheck --leak-check=full ./ffmpeg_g -v 9 -loglevel 99
-i ~/brokenCut_issue899.ts 2> ~/test4.log



==3164== Memcheck, a memory error detector
==3164== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==3164== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for
copyright info
==3164== Command: ./ffmpeg_g -v 9 -loglevel 99 -i
/home/joakim/brokenCut_issue899.ts
==3164==
FFmpeg version git-svn-r25688, Copyright (c) 2000-2010 the FFmpeg
developers
built on Nov 27 2010 13:42:29 with gcc 4.4.3
configuration: --enable-static --enable-muxers --disable-
decoder=mpeg_xvmc --disable-devices --disable-shared --enable-postproc -
-enable-gpl --enable-protocol=http --enable-pthreads --enable-runtime-
cpudetect
libavutil 50.32. 6 / 50.32. 6
libavcore 0.12. 0 / 0.12. 0
libavcodec 52.94. 3 / 52.94. 3
libavformat 52.84. 0 / 52.84. 0
libavdevice 52. 2. 2 / 52. 2. 2
libavfilter 1.56. 0 / 1.56. 0
libswscale 0.11. 0 / 0.11. 0
libpostproc 51. 2. 0 / 51. 2. 0
[NULL @ 0x420a760] Probed with size=2048 and score=100
[mpegts @ 0x420a760] stream=0 stream_type=2 pid=2ff prog_reg_desc=
[mpegts @ 0x420a760] stream=1 stream_type=3 pid=300 prog_reg_desc=
[mpegts @ 0x420a760] stream=2 stream_type=0 pid=666 prog_reg_desc=
[mpeg2video @ 0x4229310] mpeg_decode_postinit() failure
Last message repeated 15 times
[mpegts @ 0x420a760] max_analyze_duration reached
[NULL @ 0x424abd0] start time is not set in av_estimate_timings_from_pts
Input #0, mpegts, from '/home/joakim/brokenCut_issue899.ts':
Duration: 00:00:30.64, start: 40354.429344, bitrate: 5346 kb/s
Program 17502
Stream #0.0[0x2ff], 131, 1/90000: Video: mpeg2video, yuv420p,
720x576 [PAR 16:15 DAR 4:3], 1/50, 15000 kb/s, 28.04 fps, 25 tbr, 90k
tbn, 50 tbc
Stream #0.1[0x300](deu), 211, 1/90000: Audio: mp2, 48000 Hz, 2
channels, s16, 192 kb/s
No Program
Stream #0.2[0x666], 147, 1/90000: Data: [0][0][0][0] / 0x0000
At least one output file must be specified
==3164==
==3164== HEAP SUMMARY:
==3164== in use at exit: 32,564,472 bytes in 159 blocks
==3164== total heap usage: 1,716 allocs, 1,557 frees, 68,413,906 bytes
allocated
==3164==
==3164== 1,228,848 bytes in 6 blocks are definitely lost in loss record
1 of 4
==3164== at 0x4024106: memalign (vg_replace_malloc.c:581)
==3164== by 0x4024163: posix_memalign (vg_replace_malloc.c:709)
==3164== by 0x85B41C0: av_malloc (mem.c:83)
==3164== by 0x80DFF54: handle_packet (mpegts.c:1219)
==3164== by 0x80E2690: mpegts_read_packet (mpegts.c:1293)
==3164== by 0x8122EEE: av_read_packet (utils.c:686)
==3164== by 0x81283C3: av_find_stream_info (utils.c:1929)
==3164== by 0x8077705: opt_input_file (ffmpeg.c:3129)
==3164== by 0x80834C6: parse_options (cmdutils.c:204)
==3164== by 0x807E77E: main (ffmpeg.c:4224)
==3164==
==3164== 1,433,656 bytes in 7 blocks are possibly lost in loss record 2
of 4
==3164== at 0x4024106: memalign (vg_replace_malloc.c:581)
==3164== by 0x4024163: posix_memalign (vg_replace_malloc.c:709)
==3164== by 0x85B41C0: av_malloc (mem.c:83)
==3164== by 0x80DFF54: handle_packet (mpegts.c:1219)
==3164== by 0x80E2690: mpegts_read_packet (mpegts.c:1293)
==3164== by 0x8122EEE: av_read_packet (utils.c:686)
==3164== by 0x81283C3: av_find_stream_info (utils.c:1929)
==3164== by 0x8077705: opt_input_file (ffmpeg.c:3129)
==3164== by 0x80834C6: parse_options (cmdutils.c:204)
==3164== by 0x807E77E: main (ffmpeg.c:4224)
==3164==
==3164== 11,674,056 bytes in 57 blocks are definitely lost in loss
record 3 of 4
==3164== at 0x4024106: memalign (vg_replace_malloc.c:581)
==3164== by 0x4024163: posix_memalign (vg_replace_malloc.c:709)
==3164== by 0x85B41C0: av_malloc (mem.c:83)
==3164== by 0x80DFF54: handle_packet (mpegts.c:1219)
==3164== by 0x80E2690: mpegts_read_packet (mpegts.c:1293)
==3164== by 0x8122EEE: av_read_packet (utils.c:686)
==3164== by 0x81255A2: av_read_frame_internal (utils.c:1126)
==3164== by 0x8126919: av_find_stream_info (utils.c:2265)
==3164== by 0x8077705: opt_input_file (ffmpeg.c:3129)
==3164== by 0x80834C6: parse_options (cmdutils.c:204)
==3164== by 0x807E77E: main (ffmpeg.c:4224)
==3164==
==3164== 18,227,912 bytes in 89 blocks are possibly lost in loss record
4 of 4
==3164== at 0x4024106: memalign (vg_replace_malloc.c:581)
==3164== by 0x4024163: posix_memalign (vg_replace_malloc.c:709)
==3164== by 0x85B41C0: av_malloc (mem.c:83)
==3164== by 0x80DFF54: handle_packet (mpegts.c:1219)
==3164== by 0x80E2690: mpegts_read_packet (mpegts.c:1293)
==3164== by 0x8122EEE: av_read_packet (utils.c:686)
==3164== by 0x81255A2: av_read_frame_internal (utils.c:1126)
==3164== by 0x8126919: av_find_stream_info (utils.c:2265)
==3164== by 0x8077705: opt_input_file (ffmpeg.c:3129)
==3164== by 0x80834C6: parse_options (cmdutils.c:204)
==3164== by 0x807E77E: main (ffmpeg.c:4224)
==3164==
==3164== LEAK SUMMARY:
==3164== definitely lost: 12,902,904 bytes in 63 blocks
==3164== indirectly lost: 0 bytes in 0 blocks
==3164== possibly lost: 19,661,568 bytes in 96 blocks
==3164== still reachable: 0 bytes in 0 blocks
==3164== suppressed: 0 bytes in 0 blocks
==3164==
==3164== For counts of detected and suppressed errors, rerun with: -v
==3164== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 21 from 8)

************************************************************************
*


The "reason" for the leak is that mpegts.c doesn't check if it has
already allocated an output packet before
writing a new one to the pkt buffer. I've hacked up this solution in
xbmc to atleast avoid the memleak.

************************************************************************
*


Modified: trunk/xbmc/cores/dvdplayer/Codecs/ffmpeg/libavformat/mpegts.c
===================================================================
--- trunk/xbmc/cores/dvdplayer/Codecs/ffmpeg/libavformat/mpegts.c
2010-11-26 20:56:23 UTC (rev 35474)
+++ trunk/xbmc/cores/dvdplayer/Codecs/ffmpeg/libavformat/mpegts.c
2010-11-26 20:56:48 UTC (rev 35475)
@@ -625,6 +625,10 @@

static void new_pes_packet(PESContext *pes, AVPacket *pkt)
{
+ if(pkt->data) {
+ av_log(pes->stream, AV_LOG_ERROR, "ignoring previously allocated
packet on stream %d\n", pkt->stream_index);
+ av_free_packet(pkt);
+ }
av_init_packet(pkt);

pkt->destruct = av_destruct_packet;
@@ -1572,6 +1576,8 @@
}

ts->pkt = pkt;
+ ts->pkt->data = NULL;
+
ret = handle_packets(ts, 0);
if (ret < 0) {
/* flush pes data left */


************************************************************************
*****


The above code doesn't really fix the issue with the files, it just
avoids ffmpeg leaking memory on them (at the
expense of log spew). A sample file should be in incoming under the name
brokenCut_issue899.ts

----------
messages: 12709
priority: normal
status: new
substatus: new
title: Memory leak in mpegts demuxer on samples with too large PES packets
type: bug

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Carl Eugen Hoyos]

Carl Eugen Hoyos <***@rainbow.studorg.tuwien.ac.at> added the comment:

The memleak can be reproduced.

----------
status: new -> open
substatus: new -> reproduced

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Baptiste Coudurier]

Post by Joakim Plate
[...]
The "reason" for the leak is that mpegts.c doesn't check if it has
already allocated an output packet before
writing a new one to the pkt buffer. I've hacked up this solution in
xbmc to atleast avoid the memleak.

No, the reason this is happening is that the packet given to the demuxer
is already allocated for whatever reason I'm trying to find out.

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Baptiste Coudurier]

Baptiste Coudurier <***@gmail.com> added the comment:

Fixed in r25841.

----------
status: open -> closed
substatus: reproduced -> fixed

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Joakim Plate]

Joakim Plate <***@ecce.se> added the comment:

You don't think something along the lines of the "patch" would be
appropriate too?

At the start of mpegts_push_data it can emit a packet if there is data
left over on is_start. If the given buffer then contains a full pes
header + packet, it will hit the same type of memleak if i'm reading the
code right.

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Baptiste Coudurier]

Post by Joakim Plate
You don't think something along the lines of the "patch" would be
appropriate too?
At the start of mpegts_push_data it can emit a packet if there is data
left over on is_start. If the given buffer then contains a full pes
header + packet, it will hit the same type of memleak if i'm reading the
code right.

It's not data left over, it's data from the previous packet (in the case
of video packets without a pes packet size)

If the current packet has data and is overwritten, there is bug
somewhere else.

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________

[Joakim Plate]

Joakim Plate <***@ecce.se> added the comment:

Right, but that is what i'm saying. If the previous packet was emitted
at the start of that function, and the given buffer contains yet another
packet, it would overwrite the packet written at the start of the
function.

But maybe i'm missing some thing that makes sure the function is called
with buf_size=0 when there is data to be output from previous packet. If
so it would be clearer if it actually bailed out directly after emitting
the data from previous packet instead of entering the while(buf_size >
0) code which can find another payload.

Anyway, this issue is solved. So should i find some other file that
triggers above (if even possible) i'll get back with it.

----------
nosy: +elupus

________________________________________________
FFmpeg issue tracker <***@roundup.ffmpeg.org>
<https://roundup.ffmpeg.org/issue2392>
________________________________________________