Created attachment 229936 [details] [review]
A proposed patch: clearing programs array in MpegTsMux

The crash was caused by holding a dangling pointer in the MpegTsMux programs table.

Clearing progrmas array in MpegTsMux in mpegtsmux_reset().

Added a unit test: test_multiple_state_change

Comment on attachment 229936 [details] [review]
A proposed patch: clearing programs array in MpegTsMux

Thanks for the patch. While this probably fixes your problem, I don't think it's entirely right yet.

Instead of memsetting the programs, it should probably iterate over them and tsmux_program_free() those that are non-NULL, no?

Supported by valgrind as well:


tpm@zingle:~/gst/0.11/gst-plugins-bad/tests/check$ make elements/mpegtsmux.valgrind
GStreamer has detected that it is running inside valgrind.
It might now take different code paths to ease debugging.
Of course, this may also lead to different bugs.
GStreamer has detected that it is running inside valgrind.
It might now take different code paths to ease debugging.
Of course, this may also lead to different bugs.
GStreamer has detected that it is running inside valgrind.
It might now take different code paths to ease debugging.
Of course, this may also lead to different bugs.
Running suite(s): mpegtsmux
==15849== 4,900 (1,360 direct, 3,540 indirect) bytes in 5 blocks are definitely lost in loss record 1,572 of 1,601
==15849==    at 0x4C2A26B: malloc (vg_replace_malloc.c:270)
==15849==    by 0x6303F30: g_malloc (gmem.c:159)
==15849==    by 0x6318322: g_slice_alloc (gslice.c:1003)
==15849==    by 0x5511D8E: gst_buffer_new (gstbuffer.c:580)
==15849==    by 0x5512EE6: gst_buffer_new_wrapped_full (gstbuffer.c:719)
==15849==    by 0x5512F63: gst_buffer_new_wrapped (gstbuffer.c:742)
==15849==    by 0x5083535: gst_adapter_take_buffer (gstadapter.c:780)
==15849==    by 0x90228C6: mpegtsmux_push_packets (mpegtsmux.c:1337)
==15849==    by 0x9023A79: mpegtsmux_collected_buffer (mpegtsmux.c:1177)
==15849==    by 0x50B2BA9: gst_collect_pads_default_collected (gstcollectpads.c:1494)
==15849==    by 0x50AFEF6: gst_collect_pads_check_collected (gstcollectpads.c:1295)
==15849==    by 0x50B1B27: gst_collect_pads_chain (gstcollectpads.c:2009)
==15849==    by 0x553D7A9: gst_pad_push_data (gstpad.c:3654)
==15849==    by 0x402BCB: test_multiple_state_change (mpegtsmux.c:716)
==15849==    by 0x52D6E87: srunner_run_all (check_run.c:372)
==15849==    by 0x52D163C: gst_check_run_suite (gstcheck.c:663)
==15849==    by 0x401CFD: main (mpegtsmux.c:747)
==15849== 
==15849== 5,715 (48 direct, 5,667 indirect) bytes in 2 blocks are definitely lost in loss record 1,573 of 1,601
==15849==    at 0x4C2A26B: malloc (vg_replace_malloc.c:270)
==15849==    by 0x6303F30: g_malloc (gmem.c:159)
==15849==    by 0x6318322: g_slice_alloc (gslice.c:1003)
==15849==    by 0x62FB512: g_list_append (glist.c:228)
==15849==    by 0x9024BB3: new_packet_cb (mpegtsmux.c:1287)
==15849==    by 0x9026D1D: tsmux_write_section (tsmux.c:482)
==15849==    by 0x90274BB: tsmux_write_stream_packet (tsmux.c:1035)
==15849==    by 0x9023886: mpegtsmux_collected_buffer (mpegtsmux.c:1167)
==15849==    by 0x50B2BA9: gst_collect_pads_default_collected (gstcollectpads.c:1494)
==15849==    by 0x50AFEF6: gst_collect_pads_check_collected (gstcollectpads.c:1295)
==15849==    by 0x50B1B27: gst_collect_pads_chain (gstcollectpads.c:2009)
==15849==    by 0x553D7A9: gst_pad_push_data (gstpad.c:3654)
==15849==    by 0x402BCB: test_multiple_state_change (mpegtsmux.c:716)
==15849==    by 0x52D6E87: srunner_run_all (check_run.c:372)
==15849==    by 0x52D163C: gst_check_run_suite (gstcheck.c:663)
==15849==    by 0x401CFD: main (mpegtsmux.c:747)
==15849== 
100%: Checks: 6, Failures: 0, Errors: 0
make: *** [elements/mpegtsmux.valgrind] Error 1

(In reply to comment #2)
> (From update of attachment 229936 [details] [review])
> Thanks for the patch. While this probably fixes your problem, I don't think
> it's entirely right yet.
> 
> Instead of memsetting the programs, it should probably iterate over them and
> tsmux_program_free() those that are non-NULL, no?

No, the actual programs are freed in tsmux_free(). The programs array in the derived class only keeps references to them.

> Supported by valgrind as well:

The first leak is in the test itself and obvious to fix. The second one is some inconsistency in the state transition in the mpegtsmux element, i. e. mpegtsmux_reset() should iterate over the mux->streamheader list and release the resources that valgrind complains about. Looks like mpegtsmux_reset() is not called during one of the transitions.

I'll have a look.

And thanks for pointing these out. I don't have all good habits yet ;) (missed valgrind-devel package).

Ah, sorry, as you can tell I didn't really look too deeply at any of this.

> No, the actual programs are freed in tsmux_free(). The programs
> array in the derived class only keeps references to them

Ah, ok. I was just confused to see a tsmux_program_new() call in mpegtsmux.c, but no place where that got freed, for example.

Oh, one more niggle: please don't use C++-style comments but C comments, thanks!

Created attachment 229979 [details] [review]
Improved patch: fixed memory leaks in the test and in the element

* Fixed a memory leak in the test
* Found and fixed a memory leak in the mpegtsmux elements itself
All clean and tidy which makes valgrind happy again.

Thanks a lot, pushed:

 commit 13910f515462282e61b499c288418534d5672316
 Author: Krzysztof Konopko <krzysztof.konopko@youview.com>
 Date:   Mon Nov 26 19:21:03 2012 +0000

    mpegtsmux: crashes when trying to re-use the element
    
    A crash occured after pushing buffers and changing mpegtsmux state to
    NULL/READ and then back to PLAYING/PAUSED.
    
    The crash was caused by holding a dangling pointer in the MpegTsMux
    program table.
    
    Additionally stream headers were leaked when resetting the element:
    mux->streamheader set to NULL in mpegtsmux_reset() before it's released
    later in the same function.
    
    Added a unit test: test_multiple_state_change
    
    https://bugzilla.gnome.org/show_bug.cgi?id=689107