Bug 53499

Copy from #bug66300, to continue discuss here.

Comment on attachment 81616 [details] [review] [review]
[PATCH 4/4] DBusString: fix a typo and check byte value strictly

Review of attachment 81616 [details] [review] [review]:
-----------------------------------------------------------------

::: dbus/dbus-string.c
@@ +977,4 @@
>  dbus_bool_t
>  _dbus_string_insert_2_aligned (DBusString         *str,
>                                 int                 insert_at,
> +                               const unsigned char octets[2])

OK, but this shouldn't have been the same patch as the other change - they're completely orthogonal (and the other one is incorrect).

@@ +2236,5 @@
>    };
>  
> +  if (byte < 0 || byte > 0xff)
> +    return FALSE;
> +

" * @returns #FALSE if no memory"

This is not "no memory": if anything, this is a programming error (which would mean _dbus_assert, rather than returning).

However, looking at the callers of this function, on platforms where char is signed, _dbus_address_append_escaped() will pass a value between -128 and +127 to this function. So, if this was an assertion failure, I suspect it might actually be possible to trigger it.

I think the right solution here would be to change the type of the parameter 'byte' to unsigned char, and change _dbus_address_append_escaped() so p is of type "unsigned char *" (which would mean using a cast when initially assigning to p).

I think this also deserves a regression test, preferably calling dbus_address_escape_value() on a parameter containing bytes > 127, in a test-case similar to test/syntax.c. (For instance, you could check that dbus_address_escape_value ("\xab") is "%ab".)

(In reply to comment #1)
> Copy from #bug66300, to continue discuss here.
> 
> Comment on attachment 81616 [details] [review] [review] [review]
> [PATCH 4/4] DBusString: fix a typo and check byte value strictly
> 
> Review of attachment 81616 [details] [review] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-string.c
> @@ +977,4 @@
> >  dbus_bool_t
> >  _dbus_string_insert_2_aligned (DBusString         *str,
> >                                 int                 insert_at,
> > +                               const unsigned char octets[2])
> 
> OK, but this shouldn't have been the same patch as the other change -
> they're completely orthogonal (and the other one is incorrect).
> 
> @@ +2236,5 @@
> >    };
> >  
> > +  if (byte < 0 || byte > 0xff)
> > +    return FALSE;
> > +
> 
> " * @returns #FALSE if no memory"
> 
> This is not "no memory": if anything, this is a programming error (which
> would mean _dbus_assert, rather than returning).
> 
> However, looking at the callers of this function, on platforms where char is
> signed, _dbus_address_append_escaped() will pass a value between -128 and
> +127 to this function. So, if this was an assertion failure, I suspect it

Yes, I think char is signed char is a more common case than where char is unsigned char.

> might actually be possible to trigger it.
> 
> I think the right solution here would be to change the type of the parameter
> 'byte' to unsigned char, and change _dbus_address_append_escaped() so p is

Should we change the API? Since _dbus_string_append_byte_as_hex() isn't a internal symble, I think just add a _dbus_assertion() is enough.

> of type "unsigned char *" (which would mean using a cast when initially
> assigning to p).

Yes, that's the point, just like _dbus_string_hex_encode() did.

> 
> I think this also deserves a regression test, preferably calling
> dbus_address_escape_value() on a parameter containing bytes > 127, in a
> test-case similar to test/syntax.c. (For instance, you could check that
> dbus_address_escape_value ("\xab") is "%ab".)

Yes, need another commit to do that.

(In reply to comment #2)
> (In reply to comment #1)
> > Copy from #bug66300, to continue discuss here.
> > 
> > Comment on attachment 81616 [details] [review] [review] [review] [review]
> > [PATCH 4/4] DBusString: fix a typo and check byte value strictly
> > 
> > Review of attachment 81616 [details] [review] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dbus/dbus-string.c
> > @@ +977,4 @@
> > >  dbus_bool_t
> > >  _dbus_string_insert_2_aligned (DBusString         *str,
> > >                                 int                 insert_at,
> > > +                               const unsigned char octets[2])
> > 
> > OK, but this shouldn't have been the same patch as the other change -
> > they're completely orthogonal (and the other one is incorrect).
> > 
> > @@ +2236,5 @@
> > >    };
> > >  
> > > +  if (byte < 0 || byte > 0xff)
> > > +    return FALSE;
> > > +
> > 
> > " * @returns #FALSE if no memory"
> > 
> > This is not "no memory": if anything, this is a programming error (which
> > would mean _dbus_assert, rather than returning).
> > 
> > However, looking at the callers of this function, on platforms where char is
> > signed, _dbus_address_append_escaped() will pass a value between -128 and
> > +127 to this function. So, if this was an assertion failure, I suspect it
> 
> Yes, I think char is signed char is a more common case than where char is
> unsigned char.
> 
> > might actually be possible to trigger it.
> > 
> > I think the right solution here would be to change the type of the parameter
> > 'byte' to unsigned char, and change _dbus_address_append_escaped() so p is
> 
> Should we change the API? Since _dbus_string_append_byte_as_hex() isn't a
> internal symble, I think just add a _dbus_assertion() is enough.

OK, I found that dbus-string.h isn't exported by libdbus.

> 
> > of type "unsigned char *" (which would mean using a cast when initially
> > assigning to p).
> 
> Yes, that's the point, just like _dbus_string_hex_encode() did.
> 
> > 
> > I think this also deserves a regression test, preferably calling
> > dbus_address_escape_value() on a parameter containing bytes > 127, in a
> > test-case similar to test/syntax.c. (For instance, you could check that
> > dbus_address_escape_value ("\xab") is "%ab".)
> 
> Yes, need another commit to do that.

Created attachment 81669 [details] [review]
[PATCH 1/2] Fix: a non ascii byte will trigger BadAddress error

Created attachment 81670 [details] [review]
[PATCH 2/2] Test: add a test case for escaping byte > 127

(In reply to comment #3)
> > Should we change the API? Since _dbus_string_append_byte_as_hex() isn't a
> > internal symble [...]
> 
> OK, I found that dbus-string.h isn't exported by libdbus.

dbus_anything() is public, _dbus_anything() is internal.

Comment on attachment 81669 [details] [review]
[PATCH 1/2] Fix: a non ascii byte will trigger BadAddress error

Review of attachment 81669 [details] [review]:
-----------------------------------------------------------------

Great, I'll apply this.

::: dbus/dbus-address.c
@@ +111,5 @@
>  
>    ret = FALSE;
>  
>    orig_len = _dbus_string_get_length (escaped);
> +  p = (const unsigned char*) _dbus_string_get_const_data (unescaped);

Our coding style is (const unsigned char *), with the space before "*".

Comment on attachment 81670 [details] [review]
[PATCH 2/2] Test: add a test case for escaping byte > 127

Review of attachment 81670 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-address.c
@@ +679,5 @@
>    { "a", "a" },
>    { "i", "i" },
> +  { "z", "z" },
> +  /* Bug: https://bugs.freedesktop.org/show_bug.cgi?id=53499 */
> +  { "%c3%b6", "\303\266" }

This would have been more obvious if it used the "\xXX" hex form for the right-hand side, instead of the \XXX octal form: "\xc3\xb6".

This is fine, though.

Fixed in git for 1.6.14, 1.7.6. Thanks!