Re: Issue 130984 in chromium: libdbus can easily be used in thread-unsafe ways

[chro...@googlecode.com]

Updates:
Summary: libdbus can easily be used in thread-unsafe ways
Status: Assigned
Owner: sato...@chromium.org
Cc: -sato...@chromium.org key...@chromium.org davemo...@chromium.org
sain...@chromium.org
Labels: -Pri-3 Pri-1 MStone-22

Comment #1 on issue 130984 by rby...@chromium.org: libdbus can easily be
used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Ack. We NEED dbus to be reliable or we'll keep getting more subtle
reliability and security issues like issue 126217. It's far to easy to
make this sort of mistake and then burn a LOT of someone's time trying to
find it (again issue 126217 has probably cost us several hundred engineer
hours).

Could we just move all calls into DBus onto the DBus thread and then add
some low-level CHECKs to ensure that no call into DBus on another thread
can happen (even in Release builds)? Satorux, can you look into this in M22
as a follow-up to issue 126217?

[chro...@googlecode.com]

Comment #2 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Came up with an idea. What about adding something like:

Message::Message() : thread_id_(CurrentThreadId()) {
...
}

Message::~Message() {
DCHECK_EQ(thread_id_, CurrentThreadId());
...
}

This way, we can ensure that the message is deleted on the thread where the
message is created.

[chro...@googlecode.com]

Comment #3 on issue 130984 by hashim...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Solution in comment #2 seems good with current libdbus implementation, but
i'm concerned about libdbus changes in future.

libdbus docs guarantees 'thread safety' of a D-Bus connection in its
documentation.
(http://dbus.freedesktop.org/doc/api/html/group__DBusThreads.html#details)
But, as you know, this 'thread safety' was not enough for us. (even if it
is guaranteed that there is only one thread accessing a D-Bus connection,
executing ToggleWatch on a thread other than D-Bus one is fatal in our
code.)

I think the solution in comment #2 is enough if we can depend on the
current non-documented behavior of libdbus, but, if we cannot, we should
introduce more aggressive measures.

[chro...@googlecode.com]

Comment #4 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Let's do #2 for now. It's not perfect but better than nothing.

As for more aggressive measures, I have two ideas:

1) Ensure that *all* libdbus functions are called only on D-Bus thread.

As of now most libdbus functions are only called on D-Bus thread, but
dbus_message* are not. I thought using dbus_message* on UI thread was safe,
but we learned that dbus_message_unref() was not safe (crrev.com/140165),
hence we moved the call to D-Bus thread. We could go further and move
dbus_message* calls to D-Bus thread. To do this, we should copy incoming
D-Bus messages before passing these to callbacks which run on the UI thread.

2) Write our own d-bus implementation in Chrome (i.e. stop using libdbus).
This is a major effort, we'll have full control of the code. My guess is
that we'll need to write 10,000 lines of code even if we only need a subset
of the features provided by libdbus. Not sure if it's worth the investment,
though.

I think 1) is doable in a week.

[chro...@googlecode.com]

Comment #5 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Turned out #2 was not trivial. For Response (method reply), we create the
message object on UI thread in a user callback (see TestService::Echo() for
example), and delete it on D-Bus thread (deleting it on D-Bus thread is
desirable due to crrev.com/140165).

We should probably change ExportedObject API so that clients don't have to
create Response* on their side. I'll file a separate bug for this task.

[chro...@googlecode.com]

Comment #6 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Changed my mind. Recording the thread id in dbus::Message() (our wrapper)
is probably not a good idea. What it matters more is on what thread the
DBusMessage (libdbus's object) was created, which we cannot tell.

[chro...@googlecode.com]

Updates:
Labels: -Mstone-23

Comment #8 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

(No comment was entered for this change.)

[chro...@googlecode.com]

Updates:
Owner: har...@chromium.org
Cc: sato...@chromium.org

Comment #10 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

Came up with an idea.

1) Implement what the comment #2 says -- Check if the destructor of
dbus::Message is called on the same thread where the constructor was
called. As mentioned in the comment #5, this is not always true.
2) Add Message::ShouldBeDeletedOn(scoped_refptr<base::MessageLoopProxy>
message_loop_proxy). This modifies a member variable |
delete_message_loop_proxy_|
3) Annotate Message objects which should be deleted on D-Bus thread with
ShouldBeDeletedOn().
4) In the destructor, Check if the message is deleted on the thread
associated with |delete_message_loop_proxy_| if this member is set.

I think this annotation would be helpful. haruki@, could you look into this?

BTW, I rethought about the idea 1) in the comment 4, but I think this is
not feasible as there are many places where MethodCall objects are created
on UI thread. Moving them to D-Bus thread would be very difficult.

[chro...@googlecode.com]

Updates:
Owner: sato...@chromium.org
Cc: -sato...@chromium.org
Labels: -Pri-1 Pri-2

Comment #13 on issue 130984 by sato...@chromium.org: libdbus can easily be

used in thread-unsafe ways
http://code.google.com/p/chromium/issues/detail?id=130984

(No comment was entered for this change.)

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings