Bug with UTF-8 string and dbus

Bug with UTF-8 string and dbus

[Julien Danjou]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Hi,

While coding notifications.el, I found an odd bug here.
When using it with some UTF-8 chars in a string (like 'title' or
'body'), Emacs raises a D-Bus error because it did not received a reply.
But reading dbus-monitor, it did NOT send a method call.

I can reproduce it easily with:

  (dbus-call-method :session
                    "org.freedesktop.Notifications"
                    "/org/freedesktop/Notifications"
                    "org.freedesktop.Notifications"
                    "Notify"
                    :string "Emacsé")

(this is not a valid call for Notify, but anyhow it should send the
call)

I've tried to break on inside dbus.c:xd_append_arg and what I got is:
492		  char *val = SDATA (Fstring_make_unibyte (object));
(gdb) print (char *) val
$6 = 0x253e830 "Emacs", <incomplete sequence \351>

Is this normal? If yes, how to fix? If no, where's the bug?

Thanks,
-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Bug with UTF-8 string and dbus

[Stefan Monnier]

> 492		  char *val = SDATA (Fstring_make_unibyte (object));

Fstring_make_unibyte is wrong here.
Most likely, if the D-Bus specification mandates UTF-8, the better thing
to do would be either to encode using utf-8, or to take advantage of the
fact that Emacs already uses utf-8 internally and pass just SDATA (object).


        Stefan

Re: Bug with UTF-8 string and dbus

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 08 Jun 2010 20:43:37 -0400
> Cc: emacs-devel@gnu.org
> 
> > 492		  char *val = SDATA (Fstring_make_unibyte (object));
> 
> Fstring_make_unibyte is wrong here.
> Most likely, if the D-Bus specification mandates UTF-8, the better thing
> to do would be either to encode using utf-8, or to take advantage of the
> fact that Emacs already uses utf-8 internally and pass just SDATA (object).

Using unencoded SDATA would be wrong with eight-bit characters (aka
raw bytes).  I'd suggest to encode, to be on the safe side.

Re: Bug with UTF-8 string and dbus

[Julien Danjou]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Wed, Jun 09 2010, Eli Zaretskii wrote:

>> > 492		  char *val = SDATA (Fstring_make_unibyte (object));
>> 
>> Fstring_make_unibyte is wrong here.
>> Most likely, if the D-Bus specification mandates UTF-8, the better thing
>> to do would be either to encode using utf-8, or to take advantage of the
>> fact that Emacs already uses utf-8 internally and pass just SDATA (object).

According to D-Bus spec[1]:

          STRING

          UTF-8 string (must be valid UTF-8). Must be nul terminated and
          contain no other nul bytes.

> Using unencoded SDATA would be wrong with eight-bit characters (aka
> raw bytes).  I'd suggest to encode, to be on the safe side.

Any hint on how to do that ?
I mean, I don't know the Emacs C API at all, but I can test some
idea/patch if pointed in the appropriate direction.

[1]  http://dbus.freedesktop.org/doc/dbus-specification.html

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Bug with UTF-8 string and dbus

[Eli Zaretskii]

> From: Julien Danjou <julien@danjou.info>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Wed, 09 Jun 2010 08:34:17 +0200
> 
> > Using unencoded SDATA would be wrong with eight-bit characters (aka
> > raw bytes).  I'd suggest to encode, to be on the safe side.
> 
> Any hint on how to do that ?

Here's one way:

   code_convert_string_norecord (SDATA (object), Qutf_8, 1);

(Make sure you include coding.h, to have Qutf_8 declared.)

You will see quite a few other places we use this in the sources.

Re: Bug with UTF-8 string and dbus

[Jan Djärv]

Julien Danjou skrev 2010-06-09 08.34:
> On Wed, Jun 09 2010, Eli Zaretskii wrote:
>
>>>> 492		  char *val = SDATA (Fstring_make_unibyte (object));
>>>
>>> Fstring_make_unibyte is wrong here.
>>> Most likely, if the D-Bus specification mandates UTF-8, the better thing
>>> to do would be either to encode using utf-8, or to take advantage of the
>>> fact that Emacs already uses utf-8 internally and pass just SDATA (object).
>
> According to D-Bus spec[1]:
>
>            STRING
>
>            UTF-8 string (must be valid UTF-8). Must be nul terminated and
>            contain no other nul bytes.
>
>> Using unencoded SDATA would be wrong with eight-bit characters (aka
>> raw bytes).  I'd suggest to encode, to be on the safe side.
>
> Any hint on how to do that ?

char *val = SDATA (ENCODE_UTF_8 (object));

should do it.

	Jan D.



> I mean, I don't know the Emacs C API at all, but I can test some
> idea/patch if pointed in the appropriate direction.
>
> [1]  http://dbus.freedesktop.org/doc/dbus-specification.html
>

Re: Bug with UTF-8 string and dbus

[Jan Djärv]

2010-06-09 09:27, Eli Zaretskii skrev:
>> From: Julien Danjou<julien@danjou.info>
>> Cc: Stefan Monnier<monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
>> Date: Wed, 09 Jun 2010 08:34:17 +0200
>>
>>> Using unencoded SDATA would be wrong with eight-bit characters (aka
>>> raw bytes).  I'd suggest to encode, to be on the safe side.
>>
>> Any hint on how to do that ?
>
> Here's one way:
>
>     code_convert_string_norecord (SDATA (object), Qutf_8, 1);
>
> (Make sure you include coding.h, to have Qutf_8 declared.)
>
> You will see quite a few other places we use this in the sources.

That is not right, code_convert_string_norecord takes a Lisp_Object as first 
argument.

	Jan D.

[PATCH] Fix D-Bus string encoding.

[Julien Danjou]

Signed-off-by: Julien Danjou <julien@danjou.info>
---

This fix the problem described in <87typdnr08.fsf@keller.adm.naquadah.org>

 src/ChangeLog  |    5 +++++
 src/dbusbind.c |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 16e1b87..daa9ea7 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2010-06-09  Julien Danjou  <julien@danjou.info>
+
+	* dbusbind.c (xd_append_arg): Fix string encoding: it has to be
+	valid UTF-8.
+
 2010-06-08  Dan Nicolaescu  <dann@ics.uci.edu>
 
 	* lread.c (X_OK): Remove, unused.
diff --git a/src/dbusbind.c b/src/dbusbind.c
index a72a955..4a17fb4 100644
--- a/src/dbusbind.c
+++ b/src/dbusbind.c
@@ -489,7 +489,7 @@ xd_append_arg (dtype, object, iter)
       case DBUS_TYPE_OBJECT_PATH:
       case DBUS_TYPE_SIGNATURE:
 	{
-	  char *val = SDATA (Fstring_make_unibyte (object));
+          char *val = SDATA (ENCODE_UTF_8 (object));
 	  XD_DEBUG_MESSAGE ("%c %s", dtype, val);
 	  if (!dbus_message_iter_append_basic (iter, dtype, &val))
 	    XD_SIGNAL2 (build_string ("Unable to append argument"), object);
-- 
1.7.1

Re: Bug with UTF-8 string and dbus

[Eli Zaretskii]

> Date: Wed, 09 Jun 2010 10:51:08 +0200
> From: Jan Djärv <jan.h.d@swipnet.se>
> CC: Julien Danjou <julien@danjou.info>, monnier@iro.umontreal.ca, 
>  emacs-devel@gnu.org
> 
> > Here's one way:
> >
> >     code_convert_string_norecord (SDATA (object), Qutf_8, 1);
> >
> > (Make sure you include coding.h, to have Qutf_8 declared.)
> >
> > You will see quite a few other places we use this in the sources.
> 
> That is not right, code_convert_string_norecord takes a Lisp_Object as first 
> argument.

You are right, sorry.  The first argument should be object itself
(assuming it is a Lisp string).  If it is a C string, then it should
be run through make_unibyte_string first.

Re: Bug with UTF-8 string and dbus

[Stefan Monnier]

> According to D-Bus spec[1]:

>           STRING
>           UTF-8 string (must be valid UTF-8). Must be nul terminated and
>           contain no other nul bytes.

>> Using unencoded SDATA would be wrong with eight-bit characters (aka
>> raw bytes).  I'd suggest to encode, to be on the safe side.

Since the spec says "must be valid UTF-8", I think that SDATA(object)
might even be better than encoding to utf-8 since encoding to utf-8 will
turn eight-bit-byte chars into raw bytes which will likely not make
a valid utf-8 output.


        Stefan

Re: Bug with UTF-8 string and dbus

[Julien Danjou]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Wed, Jun 09 2010, Stefan Monnier wrote:

> Since the spec says "must be valid UTF-8", I think that SDATA(object)
> might even be better than encoding to utf-8 since encoding to utf-8 will
> turn eight-bit-byte chars into raw bytes which will likely not make
> a valid utf-8 output.

I clearly not have enough inside myself to decide what's the best
approach. I can provide a good patch if needed, but I'm also quite sure
any of you can fix it correctly.

So as long as someone push a patch I'll be happy. :-)

-- 
Julien Danjou
// ᐰ <julien@danjou.info>   http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Bug with UTF-8 string and dbus

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Since the spec says "must be valid UTF-8", I think that SDATA(object)
> might even be better than encoding to utf-8 since encoding to utf-8 will
> turn eight-bit-byte chars into raw bytes which will likely not make
> a valid utf-8 output.

Neither does emacs' internal encoding.  If you have a non-utf8 string
you have lost already.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Bug with UTF-8 string and dbus

[Michael Albinus]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> Since the spec says "must be valid UTF-8", I think that SDATA(object)
>> might even be better than encoding to utf-8 since encoding to utf-8 will
>> turn eight-bit-byte chars into raw bytes which will likely not make
>> a valid utf-8 output.
>
> Neither does emacs' internal encoding.  If you have a non-utf8 string
> you have lost already.

The following works with Julien's patch:

  (notifications-notify :title "Emacsé")

So it might be worth to install it.

> Andreas.

Best regards, Michael.

Re: Bug with UTF-8 string and dbus

[Stefan Monnier]

>> Since the spec says "must be valid UTF-8", I think that SDATA(object)
>> might even be better than encoding to utf-8 since encoding to utf-8 will
>> turn eight-bit-byte chars into raw bytes which will likely not make
>> a valid utf-8 output.
> Neither does emacs' internal encoding.

AFAIK, Emacs's internal encoding is valid utf-8.  It uses private
characters for some things, but I don't think that makes it invalid.

> If you have a non-utf8 string
> you have lost already.

Not sure what you mean by "non-utf8 string".


        Stefan

Re: Bug with UTF-8 string and dbus

[Davis Herring]

> AFAIK, Emacs's internal encoding is valid utf-8.  It uses private
> characters for some things, but I don't think that makes it invalid.

Aren't some of those characters encoded using more bytes than the standard
allows?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Bug with UTF-8 string and dbus

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> AFAIK, Emacs's internal encoding is valid utf-8.  It uses private
> characters for some things, but I don't think that makes it invalid.

The eight-bit characters are encoded outside of the Unicode range, and a
good utf-8 decoder must treat them as invalid.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Bug with UTF-8 string and dbus

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> AFAIK, Emacs's internal encoding is valid utf-8.  It uses private
>> characters for some things, but I don't think that makes it invalid.
>
> The eight-bit characters are encoded outside of the Unicode range, and a
> good utf-8 decoder must treat them as invalid.

Yes, that's the whole point.  Indeed, Emacs own utf-8 decoder treats
them as invalid too: when Emacs considers the data to be in utf-8
instead of emacs-internal encoding, it will decode the respective codes
into its "raw byte" presentation.  Which again is not legal utf-8 (but a
rather obvious "extension" of the utf-8 encoding scheme which quite
artificially stops at 2^20+2^16 or something similar which I don't
accurately remember and that is a consequence of the range encodable
with utf-16 with surrogate codes).

-- 
David Kastrup

Re: Bug with UTF-8 string and dbus

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Since the spec says "must be valid UTF-8", I think that SDATA(object)
> might even be better than encoding to utf-8 since encoding to utf-8 will
> turn eight-bit-byte chars into raw bytes which will likely not make
> a valid utf-8 output.

IMHO unencoded data should never leave the internals of Emacs.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix D-Bus string encoding.

[Stefan Monnier]

> +	* dbusbind.c (xd_append_arg): Fix string encoding: it has to be
> +	valid UTF-8.

Thanks, I installed a slightly different fix which just uses the
string's bytes (internally stored in utf-8 already), and added checks.


        Stefan

Re: [PATCH] Fix D-Bus string encoding.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 09 Jun 2010 20:20:10 -0400
> Cc: emacs-devel@gnu.org
> 
> > +	* dbusbind.c (xd_append_arg): Fix string encoding: it has to be
> > +	valid UTF-8.
> 
> Thanks, I installed a slightly different fix which just uses the
> string's bytes (internally stored in utf-8 already)

I agree with Andreas here: we should not output unencoded internal
representation of characters.  If nothing else, it will be a
maintenance burden if and when we decide to change something in the
internal representation, or if dbus will ever accept binary data.

I don't really understand why you insist on doing this over
objections, even though functionally both approaches are currently
equivalent.

Re: [PATCH] Fix D-Bus string encoding.

[Miles Bader]

Eli Zaretskii <eliz@gnu.org> writes:
>> Thanks, I installed a slightly different fix which just uses the
>> string's bytes (internally stored in utf-8 already)
>
> I agree with Andreas here: we should not output unencoded internal
> representation of characters.  If nothing else, it will be a
> maintenance burden if and when we decide to change something in the
> internal representation, or if dbus will ever accept binary data.

Isn't there some function in Emacs which "converts", but internally
checks to see if the desired output encoding is the same as Emacs'
internal encoding, and avoids the actual conversion in that case?  That
would be both future-proof and efficient...

[If there isn't such a function, there should be!]

-Miles

-- 
Infancy, n. The period of our lives when, according to Wordsworth, 'Heaven
lies about us.' The world begins lying about us pretty soon afterward.

Re: [PATCH] Fix D-Bus string encoding.

[Eli Zaretskii]

> From: Miles Bader <miles@gnu.org>
> System-Type: x86_64-unknown-linux-gnu
> Date: Thu, 10 Jun 2010 11:48:41 +0900
> Cc: julien@danjou.info, Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel@gnu.org
> Reply-To: Miles Bader <miles@gnu.org>
> 
> Isn't there some function in Emacs which "converts", but internally
> checks to see if the desired output encoding is the same as Emacs'
> internal encoding, and avoids the actual conversion in that case?

What do you mean by "encoding is the same"?  If the internal encoding
is utf-8-emacs, while the external is utf-8, are they "the same" or
not?

Anyway, I think avoiding the conversion in this case is a classic
example of premature optimization: no one have made the case that
performance matters in this case.  If you look at encode_coding_utf_8
you will see that it's no more than a fancy copy for almost every
character (notable exception being eight-bit characters, aka raw
bytes).  What exactly are we saving here by "avoiding the conversion"?

Am I missing something?

Re: Bug with UTF-8 string and dbus

[Andreas Schwab]

"Stephen J. Turnbull" <stephen@xemacs.org> writes:

> Maybe it's just a matter of terminology, but what about David's
> leading use case of TeX (as opposed to say Omega), which treats UTF-8
> as an octet stream?

If you have an octett stream then you don't have utf-8, but an unibyte
string.  My point is that the Emacs dbus interface should _always_
(en|de)code the strings it passes to/from the outside.  The internal
representation should remain internal to Emacs.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."