GNOME Bugzilla – Bug 732789
mpegts: don't confuse slice allocator with regular allocator
Last modified: 2014-07-11 08:01:35 UTC
The first patch fixes Coverity defect id 1224131. The second patch is hopefully an uncontroversial cleanup.
Created attachment 279969 [details] [review] Proposed patch 1/2.
Created attachment 279970 [details] [review] Proposed patch 2/2.
Created attachment 279971 [details] [review] Proposed patch 3/2. Stumbled on Coverity defect id 1224130 as well in the same area with the same kind of problem.
Comment on attachment 279969 [details] [review] Proposed patch 1/2. >@@ -977,7 +977,7 @@ _gst_mpegts_extended_event_descriptor_copy (GstMpegtsExtendedEventDescriptor * > > copy = g_slice_dup (GstMpegtsExtendedEventDescriptor, source); > copy->items = g_ptr_array_ref (source->items); >- copy->text = g_slice_copy (sizeof (source->text), source->text); >+ copy->text = g_memdup (source->text, strlen (source->text)); g_strdup() please. >@@ -1870,7 +1870,7 @@ void > gst_mpegts_dvb_data_broadcast_descriptor_free (GstMpegtsDataBroadcastDescriptor > * source) > { >- g_free (source->selector_bytes); >+ g_slice_free1 (source->length, source->selector_bytes); > g_free (source->language_code); > g_free (source->text); > g_slice_free (GstMpegtsDataBroadcastDescriptor, source); >@@ -1913,7 +1913,7 @@ gst_mpegts_descriptor_parse_dvb_data_broadcast (const GstMpegtsDescriptor > res->length = *data; > data += 1; > >- res->selector_bytes = g_memdup (data, res->length); >+ res->selector_bytes = g_slice_copy (res->length, data); > data += res->length; I think here the g_slice_copy() should be changed to g_memdup() and these two should be left alone. Generally I don't think we should use GSlice for random chunks of memory, only structs or if we know that often chunks of the same size are going to be allocated.
> Generally I don't think we should use GSlice for randomchunks of memory, only > structs or if we know that often chunks of the same size are going to be > allocated. Alright, I opted to keep as much of the original code as possible. Was a bit unclear what direction to take. I'll fix the patch ASAP.
Created attachment 280014 [details] [review] Proposed patch 1/2. Fixed Tim's review comments.
Sebastian, can you rebase against current master ? I only just realized this bug was open, and pushed a fix for one of the leaks. We should get those leak fixes in before 1.4.0.
> Sebastian, can you rebase against current master ? Of course. Let me just ask -- private_data_length is after your patch allocated by the slice allocator (this is where our patches conflict). It seems to me that this datastructure is of variable size like __tim mentioned above which leads me to believe that using the normal allocator is more appropriate? If you want me to I keep that part of the change because of this reason? Let me know and I'll rebase and upload again.
Yes, it should be switched to the regular allocator.
Created attachment 280209 [details] [review] Proposed patch 1/2.
Created attachment 280210 [details] [review] Proposed patch 2/2.
> Yes, it should be switched to the regular allocator. Alright, I think I rebased everything successfully now.
commit fba6ebaae2d9adb5aa501fc81b4a7005042f71c6 Author: Sebastian Rasmussen <sebras@hotmail.com> Date: Sun Jul 6 01:55:50 2014 +0200 mpegts: No need to check for NULL before calling g_free() Fixes https://bugzilla.gnome.org/show_bug.cgi?id=732789 commit 66c38b50f3ddd911eb5abd05e8095e438cadec78 Author: Sebastian Rasmussen <sebras@hotmail.com> Date: Sun Jul 6 01:55:16 2014 +0200 mpegts: Don't confuse slice allocator with regular one Previously selector_bytes and private_data_bytes were sometimes allocated and free using the normal allocator and sometimes using the slice allocator. Additionally prefer g_strdup() to g_memdup() for strings. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=732789