GNOME Bugzilla – Bug 750965
rtpjitterbuffer: 1. Fix a typing error of comment, 2. Add null check in free_item function
Last modified: 2015-06-16 14:39:58 UTC
1. Fix a typing error of comment. interpollate -> interpolate 2. Add null check in free_item function. It need to check more safety when releasing item.
Created attachment 305268 [details] [review] I attached patch for this bug. I attached patch for this bug.
Review of attachment 305268 [details] [review]: ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +900,3 @@ free_item (RTPJitterBufferItem * item) { + if (item && item->data && item->type != ITEM_TYPE_QUERY) How can it ever be called with item==NULL? Do you have a testcase for this, or an description of how it could get here? Also the g_slice_free() below would still crash then. This function here shouldn't really be called with NULL at all :)
I got this bug using prevent tool(coverity) as following. in master branch gstrtpjitterbuffer.c 2289 line : item = rtp_jitter_buffer_pop (priv->jbuf, NULL); free_item (item); If the item is null, it will happen crash(also follow code : item->data). I think 'free_item' function will be safety more for later because we have to check NULL before using the function. It is just my opinion.:)
If pop() returns NULL there, something is seriously wrong. But coverity can't know that of course :) What about just adding a g_return_if_fail(item != NULL) to free_item()?
and with seriously wrong I mean this can only happen if the internal state of the jitterbuffer code is inconsistent and things will likely crash sooner or later anyway
Created attachment 305271 [details] [review] I attached new patch. Thanks for your reply.
commit 6696bd62efde7db437c65b639d49da66b0b9795e Author: Sangkyu Park <sk1122.park@samsung.com> Date: Mon Jun 15 14:32:21 2015 +0900 rtpjitterbuffer: Minor cleanup 1. Add Null check in 'free_item' function. 2. Fix a typing error of comment. https://bugzilla.gnome.org/show_bug.cgi?id=750965
Note that this is internal API, so should not really have g_return_*(), but at best g_assert().
Created attachment 305356 [details] [review] I agree your opinion and attached new patch for it. Should I make new bug for this?
(In reply to Tim-Philipp Müller from comment #8) > Note that this is internal API, so should not really have g_return_*(), but > at best g_assert(). I disagree in this case, while it would be an internal programming error it still is something that shouldn't necessary stop the application. It might just continue working
That's not what we do elsewhere, and a g_return_*() doesn't really make anything 'safer', since people on embedded systems will typically compile them out anyway. Not that I mind mind leaving it as is.
(In reply to Tim-Philipp Müller from comment #11) > That's not what we do elsewhere, and a g_return_*() doesn't really make > anything 'safer', since people on embedded systems will typically compile > them out anyway. Not that I mind mind leaving it as is. Maybe that's what you mean, though it felt unclear to me. g_return_* will leave the if () return block when "compiling out". Unlike g_assert which will complete go away.
> Maybe that's what you mean, though it felt unclear to me. g_return_* will > leave the if () return block when "compiling out". No: https://git.gnome.org/browse/glib/tree/glib/gmessages.h#n326 > Unlike g_assert which will complete go away. This is true. I wasn't meaning to suggest that g_assert() is better or safer than g_return_if_*(), just that g_return_if_*() should generally only be used for public API to catch programmer errors, not in internal code. Anyway, sorry for the bikeshedding :)