GNOME Bugzilla – Bug 763762
crash due to gst_iterator_fold which lacks error handling.
Last modified: 2016-05-23 17:11:54 UTC
Hello, I have seen a crash when do pipeline destruction which caused by our own problem. However, as far as I see, this reveals the lack of handling in GST since gst_iterator_fold() unset the uninitialized value in case of errors and which is wrong. Hence a crash. GstIteratorResult gst_iterator_fold (GstIterator * it, GstIteratorFoldFunction func, GValue * ret, gpointer user_data) { GValue item = { 0, }; GstIteratorResult result; while (1) { // ... case GST_ITERATOR_RESYNC: case GST_ITERATOR_ERROR: goto fold_done; case GST_ITERATOR_DONE: goto fold_done; } } fold_done: g_value_unset (&item); return result; }
+ Trace 236089
Hope this is clear to fix that. Many thanks
Yes, it should not unset if gst_iterator_next returned GST_ITERATOR_ERROR without initialising the g_value - which has happened in this case because the iterator is NULL for some reason. How did it end up in that situation?
With some luck! That happened when tries to access a pipeline which has lots of elements in it while changing state to NULL.
AFAICS, any path that leads to that situation would first throw a CRITICAL that the bin is invalid, which implies a ref-counting error somewhere.
Thanks. So you mean that ref-counting error somewhere in user code which is our code in dtor in this case?
An extra unref somewhere in your code is the most likely. It's not impossible that it could be inside GStreamer somewhere, but it's more likely we'd have already seen that if so.
Thanks for a comment and will look into our side. BTW, this bug should be fixed or is it rather good to let the user crash?
I'm not sure I understand the crash still, actually. Did you capture any output from the app when it crashed? Is it hard to get it to happen again?
What output did you mean? GST log? Not that hard to reproduce since narrowed down how to make it happen on purpose. The question is: the app might get wrong about keeping ref-count to GST elements and which may cause to take this path leading to this. Need to look into the app side further. But separately, wondered whether this NULL iterator handling in this function is rather well-intended. May be okay to let the app crash but the app is not calling that explicitly with null iterator in this case. Hope this is clearer.
Yes, I meant any stderr output preceding the fault. It also should not crash in g_value_unset unless you have G_DEBUG=fatal-warnings on. The GValue doesn't hold a value, but it's not uninitialised - it's been initialised to 0 and should just warn on g_value_unset.... so if it's actually crashing there's something here I don't understand yet.
No log on stderr and no G_DEBUG setting. I assume that you're taking about `g_return_if_fail()` in the code? void g_value_unset (GValue *value) { GTypeValueTable *value_table; g_return_if_fail (G_IS_VALUE (value)); value_table = g_type_value_table_peek (G_VALUE_TYPE (value)); if (value_table->value_free) value_table->value_free (value); memset (value, 0, sizeof (*value)); }
We can't assume that the g_return_if_fail() checks are active, GLib might have built with -DG_DISABLE_CHECKS or whatever the define is. Pushed this now: commit e0de5ed915f779eaefac7267a54f3aa49837a0bc Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon May 23 18:00:30 2016 +0100 iterator: only unset GValue if it was inited And add some function guards. From GLib 2.48 on it is allowed to pass an uninitialised GValue to g_value_unset(). https://bugzilla.gnome.org/show_bug.cgi?id=763762 But really, if you call _fold() with a NULL iterator, all bets are off already, things went wrong before already then.