On (15/05/14 20:32), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 06:57:28PM +0200, Stef Walter wrote:
> On 12.05.2014 17:09, Jakub Hrozek wrote:
> > On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
> >> On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
> >>> On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
> >>>> Hi,
> >>>>
> >>>> the attached patches add utility functions that allow the InfoPipe
> >>>> responder to reply with an object path, escaped if needed.
> >>>>
> >>>> The ifp_reply_objpath() function was initially in my tree, but
Pavel
> >>>> improved it quite a bit and fixed some bugs, so the patch should be
> >>>> attributed to him. I'm also fine with his code...Pavel, can you
> >>>> ack^Wreview the other two?
>
> This is a really ugly part of DBus :S
>
> I wish object paths weren't so restrictive. There's no real reason for
> it, IMO. But oh well, this is what we have to live with.
>
> > + /* D-Bus spec says:
> > + * *
> > + * * Each element must only contain the ASCII characters
> > + * "[A-Z][a-z][0-9]_"
> > + * */
> > + if ((c >= 'A' && c <= 'Z')
> > + || (c >= 'a' && c <= 'z')
> > + || (c >= '0' && c <= '9')
> > + || c == '_') {
> > + safe_path = talloc_asprintf_append_buffer(safe_path,
"%c", c);
> > + if (safe_path == NULL) {
> > + goto done;
> > + }
>
> If you're expecting to unescape these paths, then you have to escape the
> underscore as well, and not let it through verbatim.
Ah, correct, thanks!
>
> > + /* Special case for the empty string */
> > + if (strcmp(path, "_") == 0) {
>
> I may be confused, but I don't see similar logic for empty strings in
> ifp_bus_path_escape().
That's also a bug.
I added unit checks for both these scenarios to the
test_path_escape_unescape() unit test.
>
> Stef
Thanks for the review!
new patches are attached.
You forgot to fix coverity issue
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS