[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-dbus
Subject: 2 race conditions in dbus-glib in multi-thread environment.
From: "Wu, Jackie" <jackie.wu () intel ! com>
Date: 2009-06-28 8:23:55
Message-ID: 706158FABBBA044BAD4FE898A02E4BC201BD8847DA () pdsmsx503 ! ccr ! corp ! intel ! com
[Download RAW message or body]
Hi, Everyone:
I recently played with dbus-glib in multi-thread environment and found following 2 \
race conditions in dbus-glib. I didn't find them in dbus mailing list archives so put \
here for you review and hope get some suggestions to solve them. I'm new in dbus, \
glib and dbus-glib programming so please correct me in case any misunderstanding. \
Thanks!
1. See code at dbus-gproxy.c:dbus_g_proxy_end_call_internal:
pending = g_hash_table_lookup (priv->pending_calls, GUINT_TO_POINTER (call_id));
dbus_pending_call_block (pending);
The pending returned can be NULL and the function dbus_pending_call_block will \
trigger assert and then abort the program.
The race condition exists at below scenario:
a. Thread A create a proxy through dbus_g_proxy_new_for_name and create a pending \
call "GetNameOwner". That's done in dbus-glib internal. b. Then Thread A uses the \
dbus_g_proxy_call and finishes a call. When it unref the proxy, it will call \
dbus_g_proxy_cancel_call to remove the "GetNameOwner" pending call. c. But before the \
Thread A really cancel the call, Thread B, which has a mainloop has received the \
reply for the "GetNameOwner" call and then call got_name_owner_cb. The \
got_name_owner_cb calls dbus_g_proxy_end_call_internal and the pending can not be \
found. Then the code above will trigger assert(pending!=NULL) in \
dbus_pending_call_block;
I did hack to return FALSE when pending returned is NULL could bypass that bug. But I \
don't what negative impacts will be.
2. See code at dbus-proxy.c:dbus_g_proxy_manager_get
manager = dbus_connection_get_data (connection, g_proxy_manager_slot);
if (manager != NULL)
{
dbus_connection_free_data_slot (&g_proxy_manager_slot);
dbus_g_proxy_manager_ref (manager);
g_static_mutex_unlock (&connection_g_proxy_lock);
return manager;
}
The manager returned from dbus_connection_get_data doesn't have manager->refcount \
increased before it calls dbus_g_proxy_manager_ref. Between these two calls, other \
thread is able to decrease manager->refcount to 0 and free the manager. Then the \
manager returned becomes an invalid pointer.
The race condition exists at below scenario:
a. Thread A wants to create a proxy, it will calls dbus_g_proxy_manager_register to \
register the proxy to the proxy manager. My understanding is there is only one proxy \
manager instance across all threads in the same process. Thread A will call \
dbus_g_proxy_manager_ref (in dbus_g_proxy_manager_get) to increase the reference \
count to the manager. b. In function dbus_g_proxy_manager_ref, it will wait for \
manager->lock. But the manager->lock is hold by Thread B and manager->refcount is 1 \
now so the reference count will not be increased till Thread B release the lock. c. \
Thread B just finished all calls and then call dbus_g_proxy_manager_dispose, which \
will call dbus_g_proxy_manager_unref. Then the manager->refcount becomes 0. Then the \
Thread B calls dbus_g_proxy_manager_unref and directly frees the manager. d. After \
Thread B release the manager->lock. Thread A was scheduled but the manager pointer is \
an invalid pointer.
I have not found good way to solve that problem. Personally I think probably we need \
some big changes.
Thanks
Jackie (Weihua) Wu
Intel Opensource Technology Center
Intel China Research Center
(inet)8-758-1963
(o) 86-10-82171963
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from rtf -->
<style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 \
2px solid; } --></style> </head>
<body>
<font face="Arial, sans-serif" size="2">
<div>Hi, Everyone:</div>
<div> </div>
<div>I recently played with dbus-glib in multi-thread environment and found following \
2 race conditions in dbus-glib. I didn't find them in dbus mailing list archives so \
put here for you review and hope get some suggestions to solve them. </div> <div>I'm \
new in dbus, glib and dbus-glib programming so please correct me in case any \
misunderstanding. Thanks! </div> <div> </div>
<div>1. See code at dbus-gproxy.c:dbus_g_proxy_end_call_internal: </div>
<div> pending = g_hash_table_lookup (priv->pending_calls, GUINT_TO_POINTER \
(call_id));</div> <div> dbus_pending_call_block (pending);</div>
<div>The pending returned can be NULL and the function dbus_pending_call_block will \
trigger assert and then abort the program. </div> <div> </div>
<div>The race condition exists at below scenario: </div>
<div>a. Thread A create a proxy through dbus_g_proxy_new_for_name and create a \
pending call "GetNameOwner". That's done in dbus-glib internal. </div> \
<div>b. Then Thread A uses the dbus_g_proxy_call and finishes a call. When it unref \
the proxy, it will call dbus_g_proxy_cancel_call to remove the \
"GetNameOwner" pending call. </div> <div>c. But before the Thread A really \
cancel the call, Thread B, which has a mainloop has received the reply for the \
"GetNameOwner" call and then call got_name_owner_cb. The got_name_owner_cb \
calls dbus_g_proxy_end_call_internal and the pending can not be found. Then the code \
above will trigger assert(pending!=NULL) in dbus_pending_call_block;</div> \
<div> </div> <div>I did hack to return FALSE when pending returned is NULL could \
bypass that bug. But I don't what negative impacts will be. </div> <div> </div>
<div>2. See code at dbus-proxy.c:dbus_g_proxy_manager_get</div>
<div> manager = dbus_connection_get_data (connection, \
g_proxy_manager_slot);</div> <div> if (manager != NULL)</div>
<div> {</div>
<div> dbus_connection_free_data_slot \
(&g_proxy_manager_slot);</div> <div> \
dbus_g_proxy_manager_ref (manager);</div> <div> \
g_static_mutex_unlock (&connection_g_proxy_lock);</div> \
<div> return manager;</div> <div> \
}</div> <div>The manager returned from dbus_connection_get_data doesn't have \
manager->refcount increased before it calls dbus_g_proxy_manager_ref. Between \
these two calls, other thread is able to decrease manager->refcount to 0 and free \
the manager. Then the manager returned becomes an invalid pointer. </div>
<div> </div>
<div>The race condition exists at below scenario:</div>
<div>a. Thread A wants to create a proxy, it will calls dbus_g_proxy_manager_register \
to register the proxy to the proxy manager. My understanding is there is only one \
proxy manager instance across all threads in the same process. Thread A will call \
dbus_g_proxy_manager_ref (in dbus_g_proxy_manager_get) to increase the reference \
count to the manager. </div> <div>b. In function dbus_g_proxy_manager_ref, it will \
wait for manager->lock. But the manager->lock is hold by Thread B and \
manager->refcount is 1 now so the reference count will not be increased till \
Thread B release the lock. </div> <div>c. Thread B just finished all calls and \
then call dbus_g_proxy_manager_dispose, which will call dbus_g_proxy_manager_unref. \
Then the manager->refcount becomes 0. Then the Thread B calls \
dbus_g_proxy_manager_unref and directly frees the manager. </div> <div>d. After \
Thread B release the manager->lock. Thread A was scheduled but the manager pointer \
is an invalid pointer. </div> <div> </div>
<div>I have not found good way to solve that problem. Personally I think probably we \
need some big changes. </div> <div> </div>
<div>Thanks</div>
<div>Jackie (Weihua) Wu</div>
<div>Intel Opensource Technology Center</div>
<div>Intel China Research Center</div>
<div>(inet)8-758-1963 </div>
<div>(o) 86-10-82171963</div>
<div> </div>
<div> </div>
</font>
</body>
</html>
_______________________________________________
dbus mailing list
dbus@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dbus
--===============1707757784==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic