When I try to start pulseaudio, and got the error message: E: [pulseaudio] module-dbus-protocol.c: dbus_server_listen() failed: org.freedesktop.DBus.Error.BadAddress: In D-Bus address, percent character was followed by characters other than hex digits I have tried to debug this, and my understanding is that dbus_address_escape_value will fail to escape bytes with the high bit set when "char" is signed. Pulsaudio creates a path to a socket under the home directory. In my case, with an o-diaeresis in the home directory name, that path is /home/göran/.pulse/bc6e8f186e8afde79d2ddb0047756edd-runtime/dbus-socket The "ö" is UTF-i encoded, that is \303\266 in octal. Pulseaudio sends this to dbus_address_escape_value which passes it on to _dbus_address_append_escaped where it is traversed by a pointer with the type "const char *". When it reaches the first byte in the "ö", the \303, this byte is correctly considered needing escaping, so it is sent to _dbus_string_append_byte_as_hex. The argument with the byte in this function has the type "int", so the normal C promotion is made. Since "char" is signed in my case, it interpreted as -61, and this is the value used in the function. When checking for the first hex number, the index into "hexdigits" is "byte >> 4" or -4, i.e. somewhere just before the array in the memory. Apparently, there happens to be \377 there this time. Next it looks up, the low four bits using the expression "byte & 0x0f", and that gets right since it completely disregards any sign extension. The end result is this string. /home/g%\377\063%\365\066ran/.pulse/bc6e8f186e8afde79d2ddb0047756edd-runtime/dbus-socket Note that \063 is correctly the ASCII code for "3", and \066 means "6". So the low hex digit are correct. But dbus_server_listen is correct in complaining that the first byte after the percent signs are not hexadecimal digits. All this was done on a Fedora 17 system, with these package versions: dbus-libs-1.4.10-4.fc17.x86_64 pulseaudio-1.1-9.fc17.x86_64
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!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.