GNOME Bugzilla – Bug 741156
gdbus: add g_dbus_object_path_{un,}escape
Last modified: 2018-05-24 17:19:12 UTC
These functions are useful when putting unknown strings into D-Bus object paths. They use the same escaping mechanism as systemd (src/shared/bus-label.c).
Created attachment 292179 [details] [review] gdbus: add g_dbus_object_path_{un,}escape
I agree this is useful, but I want this to be an official part of the D-Bus spec (as a "recommended best practice" or so) before we have an API that does it.
there have been multiple discussions on dbus-list for shared escaping rules: * http://lists.freedesktop.org/archives/dbus/2008-January/009148.html * http://lists.freedesktop.org/archives/dbus/2011-June/014475.html * http://lists.freedesktop.org/archives/dbus/2013-November/015848.html
Review of attachment 292179 [details] [review]: ::: gio/gdbusutils.c @@ +693,3 @@ +/** + * g_dbus_object_path_escape: + * @s: the string to escape What kind of string? ASCII? UTF-8? A-bunch-of-bytes? @@ +731,3 @@ + * g_dbus_object_path_escape(). + * + * Returns: (transfer full): an unescaped version of @s Currently this can return non-UTF-8 data, so it's probably worth saying that this is a guint8[] instead (ie: bytestring). @@ +756,3 @@ + + if (((hi = g_ascii_xdigit_value (p[1])) > 0) && + ((lo = g_ascii_xdigit_value (p[2])) > 0)) If I read this check correctly then we'd reject _20 as invalid. Why? If I read it incorrectly then please explain why (and add a comment). You also have a security problem here: p[2] could be past the end of the string. When fixing this, I assume you still want to reject nul. What about if we find escapes for characters that needn't have been escaped? It seems weird that we can have two valid escaped encodings for the same string -- we're going to be using these as identifiers, after all. Do we want to prevent that? @@ +763,3 @@ + else + { + g_critical ("%s: invalid escape sequence in '%s'", G_STRFUNC, s); g_critical() on bad input from a function designed to be used with potentially hostile peers? Either you need to very very clearly document that this is only ever intended to be used with trusted strings or you need to make it much more robust. That involves thinking through some of the weird edge cases and (ideally) writing down what you decided in the D-Bus spec. ::: gio/tests/gdbus-names.c @@ +792,3 @@ + assert_cmp_escaped_object_path ("", "_"); + assert_cmp_escaped_object_path (":1.42", "_3a1_2e42"); + assert_cmp_escaped_object_path ("a/b", "a_2fb"); Would be nice to test "broken" cases as well...
Created attachment 292193 [details] [review] gdbus: add g_dbus_object_path_{un,}escape Thanks for the feedback. Updated the patch:o - annotate that we work on byte strings - unescape: accept 0 as a hex digit - unescape: return NULL on invalid input - add some more test cases
Review of attachment 292193 [details] [review]: Looking a lot nicer. Only one minor issue left, plus some comment stuff. ::: gio/gdbusutils.c @@ +732,3 @@ + * g_dbus_object_path_escape(). + * + * Returns: (transfer full) (array zero-terminated=1) (element-type guint8): an this is nullable now. it's probably worth mentioning in the doc why that is the case (i'd try to be quite precise with the language here like "If the string is in a format that could not have been returned by g_dbus_object_path_escape() then this function returns %NULL.") i'd also avoid the language about "containing only characters that are valid in object paths" without further clarification about what you mean. i'd read that as a (strict) precondition, and it's not. @@ +751,3 @@ + gint hi, lo; + + if (g_ascii_isalnum (*p)) This approach is actually a good deal easier to read... @@ +759,3 @@ + ((lo = g_ascii_xdigit_value (p[2])) >= 0)) + { + g_string_append_c (unescaped, (hi << 4) | lo); You should check that the resulting character is !g_ascii_isalnum() in order to avoid non-canonical encodings (eg: someone who escapes 'A' even though it is not required). @@ +764,3 @@ + else + { + /* the string contains invalid characters */ pedantic overload: strictly speaking 'characters' is wrong in this comment because we could be here if the string was '_q' for example (and both of those are valid characters) ::: gio/tests/gdbus-names.c @@ +796,3 @@ + + g_assert_cmpstr (g_dbus_object_path_unescape ("_ii"), ==, NULL); + g_assert_cmpstr (g_dbus_object_path_unescape ("döner"), ==, NULL); your not-so-subtle attempts to convince me to come to Berlin have been noted
Review of attachment 292193 [details] [review]: ::: gio/gdbusutils.c @@ +757,3 @@ + else if (*p == '_' && + ((hi = g_ascii_xdigit_value (p[1])) >= 0) && + ((lo = g_ascii_xdigit_value (p[2])) >= 0)) One more thing: this also allows hi == lo == 0 (ie: nul). We definitely don't want that.
Created attachment 292210 [details] [review] gdbus: add g_dbus_object_path_{un,}escape Thanks again. Updated as per your comments.
Review of attachment 292210 [details] [review]: Just missing the addition to the docs -sections.txt file. Looks good except for (trivial) further annotations trouble. I'd still prefer for this to get into the D-Bus spec..... ::: gio/gdbusutils.c @@ +693,3 @@ +/** + * g_dbus_object_path_escape: + * @s (array zero-terminated=1) (element-type guint8): the string to escape Sorry for not catching it earlier, but proper format is: @s: (annotations) (go) (here): description here (you left out the first ':') @@ +699,3 @@ + * This can be reversed with g_dbus_object_path_unescape(). + * + * Returns: (transfer full): an escaped version of @s We also normally leave off (transfer full) when it's the default from the type. I wouldn't even mention it if it wasn't for the other error that needs to be fixed anyway... @@ +733,3 @@ + * returns %NULL. + * + * Returns: (transfer full) (array zero-terminated=1) (element-type guint8) (nullable): an Ditto, as above, but it may be worth verifying that (array) or (element-type) doesn't modify the default behaviour in this case. Or you could just leave it as-if if you're too lazy to check :)
Created attachment 292289 [details] [review] gdbus: add g_dbus_object_path_{un,}escape Fixed annotations and removed (transfer full), which isn't necessary for gchar*. Overriding array and element-type doesn't change that default. I checked the resulting .gir, and it moves those functions into the Gio.DBusObject (and marks their original location with moved-to="DBusObject.path_escape"). Do you know how to turn this automatic namespacing off for these functions? Would it be better to rename them?
Review of attachment 292289 [details] [review]: This needs rebasing now at least for the version exports, but seems good to me.
Marking as needs-work as per comment #10 and comment #11.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/968.