commit 2b2c0940f1b7ce8a858a26ad5e246fd645a83830
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Jan 9 17:28:17 2012 +0000

    matroskamux: fix codec string leaks

Only one of the 2 reported leaks is fixed, using the same pipeline valgrind shows:

==2875== Searching for pointers to 2,727 not-freed blocks
==2875== Checked 9,179,256 bytes
==2875== 
==2875== 28 bytes in 1 blocks are definitely lost in loss record 1,081 of 2,186
==2875==    at 0x4C279F2: calloc (vg_replace_malloc.c:467)
==2875==    by 0x55BB6E9: g_malloc0 (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.0)
==2875==    by 0xA2FF7C9: gst_matroska_mux_video_pad_setcaps (matroska-mux.c:1132)
==2875==    by 0x4E89AEE: gst_pad_set_caps (gstpad.c:2730)
==2875==    by 0x4E8B51F: gst_pad_push_data (gstpad.c:4247)
==2875==    by 0x4E8F095: gst_pad_push (gstpad.c:4730)
==2875==    by 0x9190C1D: gst_base_parse_push_frame (gstbaseparse.c:1993)
==2875==    by 0x9191C35: gst_base_parse_handle_and_push_frame.isra.9 (gstbaseparse.c:1770)
==2875==    by 0x9192A3C: gst_base_parse_chain (gstbaseparse.c:2493)
==2875==    by 0x4E8F1A9: gst_pad_push (gstpad.c:4710)
==2875==    by 0x93D767E: gst_gdp_depay_chain (gstgdpdepay.c:328)
==2875==    by 0x4E8F1A9: gst_pad_push (gstpad.c:4710)
==2875==    by 0x91A64CB: gst_base_src_loop (gstbasesrc.c:2559)
==2875==    by 0x4EB66E3: gst_task_func (gsttask.c:327)
==2875==    by 0x55DC7D7: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.0)
==2875==    by 0x55DA2B5: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.0)
==2875==    by 0x586DEFB: start_thread (pthread_create.c:304)
==2875==    by 0x5B6489C: clone (clone.S:112)
==2875== 
==2875== LEAK SUMMARY:
==2875==    definitely lost: 28 bytes in 1 blocks
==2875==    indirectly lost: 240 bytes in 10 blocks
==2875==      possibly lost: 0 bytes in 0 blocks
==2875==    still reachable: 27,091 bytes in 526 blocks
==2875==         suppressed: 494,201 bytes in 2,190 blocks
==2875== Reachable blocks (those to which a pointer was found) are not shown.
==2875== To see them, rerun with: --leak-check=full --show-reachable=yes
==2875== 
==2875== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 195 from 153)

I could only reproduce one with your file and command line, which I fixed.
If I get to repro the second one when/if I try again later, I'll fix it too, but for now it doesn't appear to leak for me (modulo all the suppressions of course).

AFAICS, there are more context fields that can be leaked in the same way (as codec string), most notably probably the codec private data.

Vincent, I rechecked the suppression file and it is ok I can reproduce the leak with the given file and with the mpeg4 stream from that camera

please also note that valgrind doens't report any leak with the latest released version

Created attachment 204934 [details] [review]
fix codec_priv leak

this patch fix the leak I reported

maybe could be done a method that free codec_priv if not null and write the new codec_priv and use this method everywhere, as pointed by Mark the same issue can happen in other parts of the plugin

Nicola

I'm using git from about yesterday, so it's odd that I only saw the string leak if you see both this and codec_priv. I just tried again to double check I didn't mess something up and they don't appear. Since it fixes them on your end, then please post a patch that also does the other uses of codec_priv.

Though it seems unfortunate we get to do these allocations more than once in the first place.

Maybe stating the obvious, but the allocation happening more than once is likely due to an upstream video parser that incrementally parses more info leading to new caps leading to renegotiation in the muxer's _set_caps (which is so far not expecting this and can lead to leaking all sorts of context stuff, codec string, codec-data etc).

AFAIK most of the other code in matroskamux should already take care to free if it decides to replace some context field (?)

In particular, would be nice if one can come up with a clever trick/hack to handle it safely for all the possible fields of the context (audio and/or video) at maybe the start of _set_caps, then it does not have to be distributed all over each individual assignment ...

Created attachment 204941 [details] [review]
another try ...

please review, this fix the leak I reported and potentially other leaks

There is a duplicate call to the new free function below "/* Create avcC header */", but aside from this the patch looks fine.

Created attachment 204947 [details] [review]
removed duplicate free

please note that the original code was:

   if (context->codec_priv != NULL) {
      g_free (context->codec_priv);
      context->codec_priv = NULL;
      context->codec_priv_size = 0;
    }

    /* Create avcC header */
    if (codec_buf != NULL) {
      context->codec_priv_size = GST_BUFFER_SIZE (codec_buf);
      context->codec_priv = g_malloc0 (context->codec_priv_size);
      memcpy (context->codec_priv, GST_BUFFER_DATA (codec_buf),
          context->codec_priv_size);
    }

so the free was ever done and codec_priv was assigned only if codec_buf != NULL

in this patch the free is done only before codec_priv assignment

maybe could be some other side effect if codec_buf == NULL, in the original matroskamux codec_priv was null in this case, with the attached patch the previous value is not modified

Removing this one changes the semantics though. I don't know if it codec_buf can nornally be NULL or if that's a safety check, but it seems best to keep the free in the same place - outside the if - and avoid adding one inside. Sorry if I was unclear about this.

Created attachment 204953 [details] [review]
shoud be fine now

There are two new _free calls before g_realloc, which are wrong, as these will lose the data before the newly allocated size.

Created attachment 204968 [details] [review]
another try ...

please review again 
thanks
Nicola

Pushed, thanks.

commit d1bb060d71923e6db5c9f837232281a9b33b08cc
Author: Nicola Murino <nicola.murino@gmail.com>
Date:   Tue Jan 10 18:50:27 2012 +0100

    matroskamux: fix codec_priv leaks
    
    https://bugzilla.gnome.org/show_bug.cgi?id=667419