GNOME Bugzilla – Bug 787306
rtsp-sink : Fix Memory leak in gstrtspclientsink.c
Last modified: 2018-04-12 18:03:11 UTC
Created attachment 359174 [details] [review] Patch file is attached. Hi , There is memory leak in gstrtspclientsink.c. Below is observation Function gst_rtsp_client_sink_setup_streams Line : 3731 There is memory leak in gstrtspclientsink.c file as "transport->destination = g_strdup (sink->server_ip);" but did not free the "detination ptr" Solution : g_free (transport->destination); ++patch is attched for possible solution. Kindly review the patch and provide your feedback.
Review of attachment 359174 [details] [review]: ::: gst/rtsp-sink/gstrtspclientsink.c @@ +3765,1 @@ gst_rtsp_transport_free (transport); gst_rtsp_transport_free() should free all memory related to the transport, so also that string
Yes .. I am agree with it. as per below doc https://sourcecodebrowser.com/gst-plugins-base0.10/0.10.18/gstrtsptransport_8c_source.html
That's docs for a 10 year old version of the code. In any case, please fix the free() function instead :)
Does it means I need to write my free() function ..instead of of using gst_rtsp_transport_free (transport);
No, you fix gst_rtsp_transport_free(). It's supposed to free everything
Created attachment 359254 [details] [review] Updated patch is attached.
Review of attachment 359254 [details] [review]: ::: gst/rtsp-sink/gstrtspclientsink.c @@ -3760,1 @@ gst_rtsp_stream_transport_set_active (context->stream_transport, TRUE); This suggests that there is a bug in the stream_transport then. If it is suppsed to own the transport, it should also free it later when needed
Created attachment 359255 [details] [review] Updated patch is attached. Please review the attached patch and provide your feedback.
Review of attachment 359255 [details] [review]: ::: gst/rtsp-sink/gstrtspclientsink.c @@ -3758,3 @@ - transport = NULL; - - gst_rtsp_stream_transport_set_active (context->stream_transport, TRUE); Why do you think removing this code is correct?
Created attachment 359256 [details] [review] Patch file is updated. As per doc "void gst_rtsp_stream_transport_set_transport (GstRTSPStreamTransport *trans, GstRTSPTransport *tr);" Set tr as the client transport. This function takes ownership of the passed tr . But gst_rtsp_stream_transport_set_active does not take any ownership to transport . So I added updated code for it. Please review once and provide your valuable comments.
Review of attachment 359256 [details] [review]: ::: gst/rtsp-sink/gstrtspclientsink.c @@ +3762,3 @@ + TRUE); + gst_object_unref (context->stream_transport); + context->stream_transport = NULL; This makes no sense: a) context->stream_transport is always not NULL here b) You first activate the stream transport, then unref and set it to NULL. Basically destroying the stream transport right after activating it Do you have a testcase to reproduce this memory leak? I would expect that test case to fail in worse ways than a memory leak with that patch. Also you said that the transport (not stream_transport) is leaked, or rather the string inside it. That would have to be freed as part of the transport then.
Can't do more here without more detailed information, sorry. Please also re-test with the latest release, a lot of code was refactored, thanks.