After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 744520 - gstutils: check uri before using it in gst_pad_create_stream_id_internal
gstutils: check uri before using it in gst_pad_create_stream_id_internal
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-14 11:24 UTC by Matthieu Bouron
Modified: 2015-02-15 17:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstutils: check uri before using it in gst_pad_create_stream_id_internal (1.44 KB, patch)
2015-02-14 11:26 UTC, Matthieu Bouron
needs-work Details | Review
gstutils: check uri before using it in gst_pad_create_stream_id_internal (1.36 KB, patch)
2015-02-15 16:53 UTC, Matthieu Bouron
committed Details | Review

Description Matthieu Bouron 2015-02-14 11:24:41 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.
Comment 1 Matthieu Bouron 2015-02-14 11:26:47 UTC
Created attachment 296818 [details] [review]
gstutils: check uri before using it in gst_pad_create_stream_id_internal
Comment 2 Tim-Philipp Müller 2015-02-15 15:03:55 UTC
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.
Comment 3 Matthieu Bouron 2015-02-15 15:25:44 UTC
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.
Comment 4 Tim-Philipp Müller 2015-02-15 15:46:16 UTC
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 :)
Comment 5 Matthieu Bouron 2015-02-15 16:53:39 UTC
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.
Comment 6 Tim-Philipp Müller 2015-02-15 17:37:56 UTC
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