GNOME Bugzilla – Bug 744520
gstutils: check uri before using it in gst_pad_create_stream_id_internal
Last modified: 2015-02-15 17:38:40 UTC
If an element implements the URI query and set the uri to NULL and if the element calls gst_pad_create_stream_id at some point, it will lead to crash as the uri is not supposed to be NULL in the gst_pad_create_stream_id_internal function.
Created attachment 296818 [details] [review] gstutils: check uri before using it in gst_pad_create_stream_id_internal
This doesn't look like it should be required. It's not allowed to call gst_query_set_uri() with a NULL uri, that would be a programming mistake in the element that does that. If an element can't answer an URI query successfully (i.e. return a valid URI), it should return FALSE from the query function. It seems to me that the only legitimate way gst_query_parse_uri() could return a NULL URI is if an element didn't call gst_query_set_uri() but still returns TRUE from the query function. Which would clearly be a bug IMHO. It "doesn't hurt" to check of course, but I don't know if we want to work around bugs like that.
I agree with you that it's not allowed to call gst_query_set_uri with a NULL uri (it outputs a bunch of critical warnings) and that the element which implements the handler should not do that and returns FALSE in the query handler instead. But I don't see the fix as a work-around for the original issue but a counter measure to avoid a crash (segmentation fault). Faulty implementations will still receives the critical warnings and thus, still have a chance to get fixed. Receiving a segfault to fix this kind of errors is a bit too extreme IMHO. What do you think ? I guess the related documentation of gst_query_set_uri might also be updated to warn users about it.
You should be already receiving a critical from the guards in gst_query_set_uri() (the gst_uri_is_valid() check) in that case. After such a critical, all bets are off anyway. If you can reduce your patch to a simple if (uri != NULL), that might be acceptable, but we don't do GST_FIXME log messages for things like this. Feel free to improve the docs for gst_query_set_uri(). The way the docs work is that if NULL is allowed as a parameter for strings or other pointers, that is explicitly marked (by means of an allow-none g-i annotation and an "or NULL" in the text). It seems intuitive to me that setting NULL URIs onto a query does not make much sense, but feel free to clarify :)
Created attachment 296882 [details] [review] gstutils: check uri before using it in gst_pad_create_stream_id_internal Patch updated: the FIXME log message has been removed.
Alright, let's do that then. Thanks for the patch! commit ee9ca5d48b9c0f0e8b5714a0d08b8915e85e8572 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Sat Feb 14 12:15:03 2015 +0100 gstutils: check uri before using it in gst_pad_create_stream_id_internal If an element implements wrongly the URI query and set the uri to NULL and if the element calls gst_pad_create_stream_id at some point, it will lead to crash as the uri is not supposed to be NULL in the gst_pad_create_stream_id_internal function. https://bugzilla.gnome.org/show_bug.cgi?id=744520