GNOME Bugzilla – Bug 755326
souphttpclientsink: Fix memory leaks and segfault crash
Last modified: 2015-10-12 13:55:30 UTC
This bug covers 2 memory leaks, 1 segfault issue and improvement in removing redundant function. 1) Memory leaks. When launching the sample pipeline present in the file, which has a invalid http uri, there are 2 memory leaks. ==17141== 24 (12 direct, 12 indirect) bytes in 1 blocks are definitely lost in loss record 2,337 of 3,970 ==17141== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==17141== by 0x4212BE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x4229281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x42093E0: g_list_append (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x62E7E58: gst_soup_http_client_sink_set_caps (gstsouphttpclientsink.c:463) ==17141== by 0x6149906: gst_base_sink_default_event (gstbasesink.c:3137) ==17141== by 0x62E7984: gst_soup_http_client_sink_event (gstsouphttpclientsink.c:618) ==17141== by 0x6141FB2: gst_base_sink_event (gstbasesink.c:3243) ==17141== by 0x409E34C: gst_pad_send_event_unchecked (gstpad.c:5452) ==17141== by 0x409EAD6: gst_pad_push_event_unchecked (gstpad.c:5123) ==17141== by 0x409F127: push_sticky (gstpad.c:3690) ==17141== by 0x409CEF6: events_foreach (gstpad.c:597) ==17141== 17,585 (24 direct, 17,561 indirect) bytes in 2 blocks are definitely lost in loss record 3,968 of 3,970 ==17141== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==17141== by 0x4212BE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x4229281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x42093E0: g_list_append (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==17141== by 0x62E76FF: gst_soup_http_client_sink_render (gstsouphttpclientsink.c:793) ==17141== by 0x6150D93: gst_base_sink_chain_unlocked.isra.12 (gstbasesink.c:3524) ==17141== by 0x6152CB7: gst_base_sink_chain_main (gstbasesink.c:3647) ==17141== by 0x409FC7D: gst_pad_push_data (gstpad.c:4085) ==17141== by 0x6242439: gst_ogg_mux_push_buffer.isra.10 (gstoggmux.c:594) ==17141== by 0x624611B: gst_ogg_mux_collected (gstoggmux.c:1618) ==17141== by 0x616B618: gst_collect_pads_check_collected (gstcollectpads.c:1364) ==17141== by 0x616D12E: gst_collect_pads_chain (gstcollectpads.c:2217) Adding code to free streamheader_buffers and sent_buffers in reset. 2) Segfault. In case we pass a location without 'http://', then soup_message_new ("PUT", souphttpsink->location); will return NULL. There is no check for this and results in segfault. Hence checking the same and returning. 3) Redundant function. A new function free_buffer_list is written, which does the same as g_list_free_full. Hence removing the function and replacing it with g_list_free_full in all places. Adding all changes into a single patch for now.. Maybe we can split the patch for each change to avoid confusion.
Created attachment 311730 [details] [review] fix memory leaks, segfault and remove redundant function
Review of attachment 311730 [details] [review]: ::: ext/soup/gstsouphttpclientsink.c @@ +653,3 @@ + if (souphttpsink->message == NULL) { + GST_DEBUG_OBJECT (souphttpsink, + "URI could not be parsed while creating message."); Ideally we should also catch that in set_uri / set_property already, but this check would still be a good idea on top of that. You could use GstUri for the validation there
Created attachment 311810 [details] [review] fix memory leaks, segfault and remove redundant function as per review comments checking if the location uri is valid in set_property and freeing the location variable if not valid.
Created attachment 311811 [details] [review] fix memory leaks, segfault and remove redundant function remove extra line added by mistake :)
Created attachment 311814 [details] [review] fix memory leaks, segfault and remove redundant function add an extra check for location to see if it is NULL. Prevents assertion error when location is set as NULL
ping :)
Review of attachment 311814 [details] [review]: Can you make this 3 patches? ::: ext/soup/gstsouphttpclientsink.c @@ +255,3 @@ + (GDestroyNotify) gst_buffer_unref); + g_list_free_full (souphttpsink->sent_buffers, + (GDestroyNotify) gst_buffer_unref); These changes in 1) @@ +297,3 @@ souphttpsink->offset = 0; + if ((souphttpsink->location == NULL) + || !gst_uri_is_valid (souphttpsink->location)) { This 2) @@ +660,2 @@ souphttpsink->message = soup_message_new ("PUT", souphttpsink->location); + if (souphttpsink->message == NULL) { This 3)
Created attachment 313093 [details] [review] patch 1: replace redundant free_buffer_list Making it into four patches. 1) Replace redundant free_buffer_list with g_list_free_full. 2) Free streamheader_buffers and sent_buffers during failure cases. 3) Check if location being set is valid. 4) Check if soup message is created.
Created attachment 313094 [details] [review] patch 2: fix memory leaks during failures
Created attachment 313095 [details] [review] patch 3: Check if location being set is valid
Created attachment 313096 [details] [review] patch 4: check if soup message is created
commit 6eb8db8afd1a8186be5b155fd945e679b56ccc77 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Oct 12 11:18:51 2015 +0900 souphttpclientsink: Check if soup message is created If soup message is not created then the same should not be passed on, which is resulting in segfault. Hence throwing a warning message and returning https://bugzilla.gnome.org/show_bug.cgi?id=755326 commit 2ee7b2b94de4ae638fef65596e3c7da92eff831f Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Oct 12 11:15:15 2015 +0900 souphttpclientsink: Check if location being set is valid Adding a check in set_property to find if the location uri is valid and printing warning if not valid. https://bugzilla.gnome.org/show_bug.cgi?id=755326 commit cece69003c2d0cc535fb1df1e3579cce68df3dcc Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Oct 12 11:09:30 2015 +0900 souphttpclientsink: Fix memory leaks during failures freeing streamheader_buffers and sent_buffers during failure cases. https://bugzilla.gnome.org/show_bug.cgi?id=755326 commit b1d4e7e222faadfd60f2a86b31ed697a9689a9c5 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Oct 12 11:03:17 2015 +0900 souphttpclientsink: Replace redundant free_buffer_list function Removing free_buffer_list and replacing it with already available function g_list_free_full https://bugzilla.gnome.org/show_bug.cgi?id=755326