Bug 16338 - sending non-UTF-8 strings is not diagnosed as a programming error
Summary: sending non-UTF-8 strings is not diagnosed as a programming error
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: high major
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard: NB#223152
Keywords: patch
Depends on:
Reported: 2008-06-13 04:05 UTC by Martyn Russell
Modified: 2011-03-10 11:12 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:

dbus_message_iter_append_basic: check string-like arguments for validity (3.50 KB, patch)
2011-02-25 08:49 UTC, Simon McVittie
Details | Splinter Review
dbus-marshal-validate: expose _dbus_validate_utf8 as API (1.64 KB, patch)
2011-03-03 09:28 UTC, Simon McVittie
Details | Splinter Review
dbus_message_iter_append_basic: validate booleans too (1.17 KB, patch)
2011-03-03 09:29 UTC, Simon McVittie
Details | Splinter Review
dbus_message_iter_append_fixed_array: add a check for valid booleans (1.18 KB, patch)
2011-03-03 09:30 UTC, Simon McVittie
Details | Splinter Review

Description Martyn Russell 2008-06-13 04:05:38 UTC
We are trying to send file paths across DBus between two Tracker processes. We found that the trackerd (which sends the messages to tracker-indexer) was exiting. 

It turns out that DBus calls _exit() for us from inside a library. That is incredible. No error message, no warning, just _exit(). No library should ever do this. What if you use DBus for some critical application/system?


So here is the back trace:

(gdb) break _exit
Breakpoint 2 at 0xb7b9f044
(gdb) cont
[Thread 0xb58feb90 (LWP 21904) exited]
[Thread 0xb7173b90 (LWP 21900) exited]
[Thread 0xb60ffb90 (LWP 21903) exited]
[Thread 0xb34ffb90 (LWP 21924) exited]
[Thread 0xb46ffb90 (LWP 21912) exited]
[Thread 0xb3efeb90 (LWP 21917) exited]
[Thread 0xb2cfeb90 (LWP 21927) exited]

Breakpoint 2, 0xb7b9f044 in _exit () from /lib/tls/i686/cmov/libc.so.6

Breakpoint 2, 0xb7b9f044 in _exit () from /lib/tls/i686/cmov/libc.so.6
(gdb) bt
#0  0xb7b9f044 in _exit () from /lib/tls/i686/cmov/libc.so.6
#1  0xb7e5f3dd in _dbus_exit (code=1) at dbus-sysdeps-unix.c:2480
#2  0xb7e41a31 in _dbus_connection_update_dispatch_status_and_unlock (connection=0x8100048, new_status=DBUS_DISPATCH_COMPLETE) at dbus-connection.c:4032
#3  0xb7e44777 in _dbus_connection_send_and_unlock (connection=0x8100048, message=0x8105cc8, client_serial=0x0) at dbus-connection.c:2015
#4  0xb7e447fd in dbus_connection_send (connection=0x8100048, message=0x8105cc8, serial=0x0) at dbus-connection.c:3086
#5  0xb7e3d2a7 in send_no_return_values (connection=0x8100048, msg=0x8105cc8, error=<value optimized out>) at dbus-bus.c:1372
#6  0xb7e3d326 in dbus_bus_remove_match (connection=0x8100048, 
    rule=0x8101b90 "type='signal',sender='org.freedesktop.Tracker.Indexer',path='/org/freedesktop/Tracker/Indexer',interface='org.freedesktop.Tracker.Indexer'", error=0x0) at dbus-bus.c:1530
#7  0xb7e79589 in ?? () from /usr/lib/libdbus-glib-1.so.2
#8  0xb7e7a5b5 in ?? () from /usr/lib/libdbus-glib-1.so.2
#9  0xb7d66d70 in g_object_run_dispose () from /usr/lib/libgobject-2.0.so.0


After futher conversations on IRC between walters and pvanhoof. Phillip kindly looked into this and all these IRC logs/back traces come from him:

pvanhoof What makes check_disconnected_message_arrived_unlocked happen in dbus-connection.c ?
pvanhoof And why does this take place? connection->disconnected_message_arrived = TRUE;
pvanhoof Which will result in _dbus_connection_update_dispatch_status_and_unlock calling _dbus_exit, which will unsolicited exit my process
pvanhoof Even in such a way that it's hardly debuggable too, abort() would have been ... nicer
walters by default dbus will exit when the bus dies, just like how xlib will when the X server connection is lost
pvanhoof So the bus dies
pvanhoof What can make it die?  :) 
walters you can use _set_exit_on_disconnect (), but you should try to avoid that
* abock_ (n=abock@nat/novell/x-0c2477506c243d42) has joined #dbus
walters when the session ends
pvanhoof walters, it looks like dbus_g_bus_get does that
walters what kind of connection is this?  to the session bus?  system bus?  private connection?
pvanhoof I don't seem to have control over that with dbus-glib
pvanhoof         connection = dbus_g_bus_get (DBUS_BUS_SESSION, &error);
pvanhoof http://pastebin.com/m68e7f2cb
walters yeah, you'd have to get the DBusConnection using dbus_g_connection_get_connection
pvanhoof Is that better than dbus_g_bus_get ?
walters hmm
pvanhoof I also wonder what is making my session-bus die
walters no you call it after you call dbus_g_bus_get
pvanhoof We are being quite aggressive with what we do with poor little dbus
pvanhoof but dying  :) ?
pvanhoof We're also not seeing this on all versions of DBus, only on Ubuntu Hardy's
walters is the session bus crashing?
pvanhoof If there's a way how I can debug this (if it's indeed a session-bus bug), let me know
pvanhoof I can reproduce it easily
walters a stack trace would be useful...does hardy have apport?  it should be in /var/crash i think then
pvanhoof I did in /root: DEB_BUILD_OPTIONS=nostrip apt-get source dbus -b
pvanhoof And I installed those packages
pvanhoof Just FYI about my environment
* KaL is now known as KaL_out
walters you could attach gdb to the session bus too
pvanhoof apport is already the newest version.
pvanhoof 108        360  0.0  0.1   2876  1320 ?        Ss   20:24   0:00 /usr/bin/dbus-daemon --system
pvanhoof that one?
walters no, that's the system bus
pvanhoof pvanhoof  1134  0.0  0.1   2776  1196 ?        Ss   20:26   0:00 dbus-daemon --fork --print-address 24 --print-pid 26 --session
pvanhoof that one?
walters presumably
pvanhoof (gdb) cont
pvanhoof Continuing.
pvanhoof ok, let's see
pvanhoof it didn't crash, yet my application had the _exit() problem
* danpb_ltop has quit (Read error: 104 (Connection reset by peer))
* danpb_ltop (n=berrange@79-78-85-191.dynamic.dsl.as9105.com) has joined #dbus
walters another possibility is the session bus is kicking your application off because it sent something invalid
pvanhoof Detaching from program: /usr/bin/dbus-daemon, process 1134
pvanhoof pvanhoof@nerts:/root/dbus-1.1.20/dbus$ 
walters highly probable is invalid UTF-8
pvanhoof oh, yeah. martyn is sending filenames in a GStrv over the dbus
pvanhoof Can that trigger this?
walters you might try dbus-monitor --session, i think you would see the error return from that
walters certainly if the filenames are not UTF-8
pvanhoof http://pastebin.com/mfb9795a
pvanhoof that's dbus-monitor --session, and making it happen
* bergie has quit ()
walters hm, actually you wouldn't see an invalid message in the monitor - i think - because the bus would drop it before it sent it to the monitor handler

pvanhoof But the cause is quite likely an invalid UTF-8 filename on my filesystem, that's being collected into the GStrv and being sent over the bus
pvanhoof Sounds likely, because I'm the only guy who's having these problems
pvanhoof (trackerd:28849): Tracker-DEBUG: Found  :'/home/pvanhoof/Documents/Presentations/jokela/zami.pp.fi/stuff/Kuvat/2007/Jyv\xe4skyl\xe4ss\xe4 31.10 - 4.11.2007/index.html' (16)
* thiago_home (n=thiago@kde/thiago) has joined #dbus
pvanhoof like that one  :) 
walters yeah, whichever app is doing that should probably use g_utf8_validate and if that fails try reencoding it using the locale
pvanhoof okay
pvanhoof thanks
walters np


This is where the _exit() takes place:
 3997  static void
 3998  _dbus_connection_update_dispatch_status_and_unlock (DBusConnection    *connection,
 3999                                                      DBusDispatchStatus new_status)
 4000  {
   dbus_bool_t changed;
   DBusDispatchStatusFunction function;
   void *data;
   HAVE_LOCK_CHECK (connection);
   _dbus_connection_ref_unlocked (connection);
   changed = new_status != connection->last_dispatch_status;
   connection->last_dispatch_status = new_status;
   function = connection->dispatch_status_function;
   data = connection->dispatch_status_data;
   if (connection->disconnected_message_arrived &&
       connection->disconnected_message_processed = TRUE;
       /* this does an unref, but we have a ref
        * so we should not run the finalizer here
        * inside the lock.
       connection_forget_shared_unlocked (connection);
       if (connection->exit_on_disconnect)
           CONNECTION_UNLOCK (connection);            
 3031            _dbus_verbose ("Exiting on Disconnected signal\n");
 4032            _dbus_exit (1);
 4033            _dbus_assert_not_reached ("Call to exit() returned");



So from the superb advice from walters, we tried checking every string for UTF-8 validity and it stopped DBus from calling _exit().

Can we please change this implementation so we don't call _exit() EVER in DBus? It would be much better for DBus to just drop the message or return a GError in such cases so people know what is going on.
Comment 1 Havoc Pennington 2008-07-11 19:33:56 UTC
Read the docs for dbus_connection_set_exit_on_disconnect(). Exit is the default on disconnect for the same reason it is in Xlib: nearly all apps should exit on disconnect from dbus, because the session bus only exits on session end, and it's a bug for an app to persist after the session ends.

It is _allowed_ by the API to persist after session end, or exit in a custom way, but you have to take explicit action to turn off the default behavior. The reason this makes sense is that if you turn off the default behavior you MUST write a handler for the Disconnected signal from your DBusConnection - if you don't exit on this signal, you do at least need to unref and free the connection object. So ignoring it is not OK. If you're going to know to write that handler, you'll also know to call dbus_connection_set_exit_on_disconnect() to turn off the default handler which exits. But if you don't think about it, exiting is the right default, just as Xlib exits when you lose the X server connection.

Xlib doesn't have a way (short of longjmp()) to even turn this off, which I think is wrong, but Xlib does have the proper default. Without this Xlib default we'd be plagued by apps that don't die when the session ends. Same for libdbus. 

With libdbus, you must do something on disconnect, there's a default handler, and you can replace it if you like.

Now, the real issue you have is that you're getting disconnected. The reason you're disconnected is that you sent not-well-formed protocol. DBUS_TYPE_STRING is required to be UTF-8. The bus daemon strictly validates well-formedness for security reasons and robustness-of-session reasons, so the whole desktop does not crash. If you don't want UTF-8 validation, then use an array of bytes, not a string.

Unfortunately, it's quite challenging to modify dbus-daemon to try sending back an error reply before it closes the socket; there may be another open bug about that, or at least a thread in list archives, and I think a patch makes sense, but, it isn't an easy patch to write iirc. (I don't remember the details.)
The daemon would need to write out an error reply to the not-well-formed message, flush the socket as possible without blocking, then close the socket, all synchronously. 99% of the time the client would get the error, for debugging purposes, though it would not be guaranteed.

That would be a good patch, in the meantime there's a simple fix that would have saved you: add a _dbus_return_if_fail(validate_utf8(arg)) to whatever public API function allowed you to marshal an invalid UTF-8 string. (In fact there's a general rule that libdbus should never send not-well-formed protocol: it should trap any application mistakes in this respect with return_if_fail() statements. But as you see, some of the return_if_fail are missing; patches to add them are welcome.)

And your app when loading untrusted data should be fixed to validate it before passing it in to APIs that require valid UTF-8 (including dbus and GTK). Both libdbus and GTK have undefined behavior if you give invalid UTF-8 to APIs that require valid strings.

Comment 2 Havoc Pennington 2008-08-19 11:42:16 UTC
Relevant archive link on how to fix this:
Comment 3 Stian Skjelstad 2009-01-29 04:25:17 UTC
I saw simular happen in one project of mine.. My program just exited. After some debugging I found out that dbus called exit(). More debugging showed that my client was disconnected from the dbus server due to communcation error (I had some garbage strings sent over).

But I do agree, would be better if dbus_* functions in the client returns error on disconnected stated instead of calling exit()
Comment 4 Colin Walters 2009-01-29 04:57:59 UTC
One possibility is to have an environment variable like LIBDBUS_VALIDATE=1 or something which would cause the library to check UTF-8 client side, and if there are any other checks that would make sense, we could do those as well.
Comment 5 Havoc Pennington 2009-03-06 16:43:06 UTC
I don't think any fancy env variables are needed; most things should already be checked by _dbus_return_if_fail

Why not just add _dbus_return_if_fail(validate_utf8(arg)) to the public API entry points that take a utf-8 string? That's the simple fix...

For actual non-checks validation, I'd stick to having just the recipient not client do it, and fix the bug to send an error back before dropping connection. But that case should basically never happen if we add a couple return_if_fail(valid_utf8) in key places; invalid utf8 is one of the few things not checked already.

Comment 6 Simon McVittie 2011-02-25 08:48:26 UTC
I have a branch to validate UTF-8 strings, and the syntax of signatures and object paths, before sending. I'll attach the patch in a moment.

(The validation can be turned off with --disable-checks, at the cost of turning programming errors from a noisy assertion failure into a silent crash. As with GLib g_return_if_fail checks, I don't recommend doing this.)

(In reply to comment #5)
> For actual non-checks validation, I'd stick to having just the recipient not
> client do it, and fix the bug to send an error back before dropping connection.
> But that case should basically never happen if we add a couple
> return_if_fail(valid_utf8) in key places

I've cloned Bug #34726 to represent the request to send back an error when invalid messages are received.
Comment 7 Simon McVittie 2011-02-25 08:49:36 UTC
Created attachment 43809 [details] [review]
dbus_message_iter_append_basic: check string-like arguments for validity

Strings: UTF-8 with no embedded NULs, by adding a new internal function,

Object paths, signatures: the obvious syntactic checks

This moves some of the burden of validation to the sender.

When sending <http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-demo.txt>
10240 times with up to 1024 parallel calls pending, on a single-core ARM
Linux device, I found that user CPU time in dbus-spam increased by up to 80%
as a result of the validation. However, when sending messages to dbus-daemon,
overall throughput only reduced by 15%, and when sending messages to an echo
service, overall throughput actually improved by around 14% (presumably
because making the sender CPU-bound influenced kernel scheduling).
Comment 8 Simon McVittie 2011-02-25 08:51:43 UTC
(In reply to comment #7)
> When sending ... in dbus-spam
> to an echo service

These are provided by Bug #34140 (to which I'll attach an updated patch).
Comment 9 Cosimo Alfarano 2011-02-28 09:20:02 UTC
Non a d-bus developer, but the patch seems OK to me.
Comment 10 Cosimo Alfarano 2011-02-28 09:27:10 UTC
Review of attachment 43809 [details] [review]:

Just one observation, actually.

I'd move 

1225	/* this validation function doesn't follow the same naming pattern :-( */
1226	#define _dbus_validate_utf8(s,b,e) _dbus_string_validate_utf8 (s, b, e)

to the .h file, hiding the implementation of the macro.
It makes the reading easier for who does not want to know how the checking actually works.

my 2p.
Comment 11 Simon McVittie 2011-02-28 10:25:35 UTC
(In reply to comment #10)
> Review of attachment 43809 [details] [review]:
> Just one observation, actually.
> I'd move 
> 1225    /* this validation function doesn't follow the same naming pattern :-(
> */
> 1226    #define _dbus_validate_utf8(s,b,e) _dbus_string_validate_utf8 (s, b, e)
> to the .h file, hiding the implementation of the macro.
> It makes the reading easier for who does not want to know how the checking
> actually works.

Oh, I'd forgotten that hack was still there. :-(

What I'm working around there is:

* there are two sets of API for validating things:
  the _dbus_validate_THING family takes a byte-range inside a DBusString,
  whereas the _dbus_check_is_valid_THING family are wrappers, only
  defined when --disable-checks is not given, which take a const char *
  and assume it's \0-terminated
* for this check, we need to do the same for UTF-8 validation
* DEFINE_DBUS_NAME_CHECK expands to definition of a function
  _dbus_check_is_valid_THING that calls _dbus_validate_THING
* ... but UTF-8 validation doesn't follow the same naming pattern:
  the DBusString version is called _dbus_string_validate_utf8,
  not just _dbus_validate_utf8
* ... so I "rename" it locally

I'm not sure why you think it'd be better to move this hack to the header file and make it API? If we keep it at all, I think its scope should be as narrow as possible, so, only in the implementation.

However, a better fix would probably be to either rename _dbus_string_validate_utf8 to _dbus_validate_utf8, or rename the rest of the _dbus_validate_THING family to _dbus_string_validate_THING. Colin/whoever: any opinions on which direction would be preferred?

(Or, I could write _dbus_check_is_valid_utf8 out without using the weird macros, at the cost of a bit more boilerplate.)
Comment 12 Simon McVittie 2011-03-03 09:28:32 UTC
Created attachment 44082 [details] [review]
dbus-marshal-validate: expose _dbus_validate_utf8 as API

On second thoughts, giving it an alias consistent with
_dbus_validate_signature etc. (as Cosimo suggests) seems reasonable.

(This could be squashed into Attachment #43809 [details] if desired.)
Comment 13 Simon McVittie 2011-03-03 09:29:48 UTC
Created attachment 44083 [details] [review]
dbus_message_iter_append_basic: validate booleans too

Sending, for instance, ((dbus_bool_t) 666) is a programming error and
should be diagnosed as such.

(Again, this could be squashed into Attachment #43809 [details] if desired. The validation done in message recipients will reject these badly-valued booleans, so it has the same problems as a non-UTF-8 string.)
Comment 14 Simon McVittie 2011-03-03 09:30:14 UTC
Created attachment 44084 [details] [review]
dbus_message_iter_append_fixed_array: add a check for valid booleans

The reasoning is the same as for dbus_message_iter_append_basic.
Comment 15 Cosimo Alfarano 2011-03-03 10:16:08 UTC
Review of attachment 44083 [details] [review]:

seems OK
Comment 16 Cosimo Alfarano 2011-03-03 10:16:16 UTC
Review of attachment 44084 [details] [review]:

seems OK
Comment 17 Cosimo Alfarano 2011-03-03 10:17:24 UTC
Review of attachment 44082 [details] [review]:

I was thinking the same place :) OK, again no hat.
Comment 18 Simon McVittie 2011-03-10 05:07:40 UTC
Anyone interested in reviewing this, or opining whether it should be in 1.4 or 1.5? Colin? Will? Lennart?
Comment 19 Will Thompson 2011-03-10 10:38:31 UTC
(In reply to comment #18)
> Anyone interested in reviewing this, or opining whether it should be in 1.4 or
> 1.5? Colin? Will? Lennart?

These patches look reasonable. Given that this stuff is validated by the receiver, who'll drop the connection if it's invalid, I think applying these to 1.4 is acceptable: it will give better error messages than the current behaviour of unexplained disconnection from the bus.
Comment 20 Simon McVittie 2011-03-10 11:12:25 UTC
Fixed in dbus-1.4 for 1.4.8 and in master for 1.5.0.

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.