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 783787 - gsturi: Fixed incorrect escaping of path in gst_uri_construct
gsturi: Fixed incorrect escaping of path in gst_uri_construct
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.12.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-06-14 14:16 UTC by Dimitrios Katsaros
Modified: 2017-06-15 08:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Relevant patch (1.60 KB, patch)
2017-06-14 14:19 UTC, Dimitrios Katsaros
needs-work Details | Review
Updated verison of patch witch suggestions from slomo (3.49 KB, patch)
2017-06-14 15:41 UTC, Dimitrios Katsaros
none Details | Review
Patch with added deprecation macros (5.36 KB, patch)
2017-06-15 08:24 UTC, Dimitrios Katsaros
committed Details | Review

Description Dimitrios Katsaros 2017-06-14 14:16:40 UTC
gst_uri_construct was handling the 'location' component of the uri as a generic URI string to be encoded. This is incorrect since the string is actually a path and based on RFC 3986, 'A path consists of a sequence of path segments separated by a slash ("/") character.'. This patch changes the 'escape_string' function to use 'escape_string_internal' with the "UNSAFE_PATH" option. A side effect is that the escape_string function is unused so I have flagged it as such so that gcc can compile gsturl with the -Werror=unused-function flag.
Comment 1 Dimitrios Katsaros 2017-06-14 14:19:45 UTC
Created attachment 353743 [details] [review]
Relevant patch
Comment 2 Dimitrios Katsaros 2017-06-14 15:41:38 UTC
Created attachment 353749 [details] [review]
Updated verison of patch witch suggestions from slomo
Comment 3 Sebastian Dröge (slomo) 2017-06-15 07:21:35 UTC
Review of attachment 353743 [details] [review]:

Makes sense to me, but let's at the same time deprecate this function in favour of the GstUri API. Can you add the GST_DISABLE_DEPRECATED/GST_REMOVE_DEPRECATED markers here like in other places?

::: gst/gsturi.c
@@ +259,3 @@
  * but with all special characters escaped
  **/
+G_GNUC_UNUSED static gchar *

Please just remove this function :)
Comment 4 Dimitrios Katsaros 2017-06-15 08:24:48 UTC
Created attachment 353802 [details] [review]
Patch with added deprecation macros
Comment 5 Sebastian Dröge (slomo) 2017-06-15 08:39:25 UTC
commit 688d79033f5e212f64d6f81060bf275f8a319ec2
Author: Dimitrios Katsaros <patcherwork@gmail.com>
Date:   Wed Jun 14 17:36:57 2017 +0200

    gsturi: Fixed incorrect escaping of path as a generic string
    
    The gst_uri_construct function was escaping the location string
    as a generic uri string. This is incorrect since the slash('/')
    characters are reserved for use in this exact case. The patch
    changes the escape_string function mode to handle the path correctly.
    
    I have deleted the escape_string function since it is no longer being
    used and have created a unit test for the function. I have also
    deprecated this function in favour of the GstUri API.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783787
Comment 6 Sebastian Dröge (slomo) 2017-06-15 08:41:55 UTC
And 1.12.1 without the deprecation