From e4677cd87ff9e4e1a4e2716eacd30016d848bd08 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 10 Jul 2009 21:33:02 -0400 Subject: [PATCH] Bug 896 - Avoid race conditions reading message from exited process MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Patch based on extensive work from Michael Meeks , thanks to Dafydd Harries , Kimmo Hämäläinen and others. The basic idea with this bug is that we effectively ignore errors on write. Only when we're done reading from a connection do we close down a connection. This avoids a race condition where if a process (such as dbus-send) exited while we still had data to read in the buffer, we'd miss that data. --- dbus/dbus-sysdeps.c | 10 ++++++++++ dbus/dbus-sysdeps.h | 1 + dbus/dbus-transport-socket.c | 36 ++++++++++++++++++++++++++++-------- dbus/dbus-watch.c | 6 ++++++ dbus/dbus-watch.h | 1 + 5 files changed, 46 insertions(+), 8 deletions(-) diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c index e0fe888..ccd80cc 100644 --- a/dbus/dbus-sysdeps.c +++ b/dbus/dbus-sysdeps.c @@ -1078,6 +1078,16 @@ _dbus_get_is_errno_eintr (void) } /** + * See if errno is EPIPE + * @returns #TRUE if errno == EPIPE + */ +dbus_bool_t +_dbus_get_is_errno_epipe (void) +{ + return errno == EPIPE; +} + +/** * Get error message from errno * @returns _dbus_strerror(errno) */ diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index 7af4287..7ef2df3 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -362,6 +362,7 @@ dbus_bool_t _dbus_get_is_errno_nonzero (void); dbus_bool_t _dbus_get_is_errno_eagain_or_ewouldblock (void); dbus_bool_t _dbus_get_is_errno_enomem (void); dbus_bool_t _dbus_get_is_errno_eintr (void); +dbus_bool_t _dbus_get_is_errno_epipe (void); const char* _dbus_strerror_from_errno (void); void _dbus_disable_sigpipe (void); diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c index 46cbed9..8be4d13 100644 --- a/dbus/dbus-transport-socket.c +++ b/dbus/dbus-transport-socket.c @@ -616,7 +616,11 @@ do_writing (DBusTransport *transport) { /* EINTR already handled for us */ - if (_dbus_get_is_errno_eagain_or_ewouldblock ()) + /* For some discussion of why we also ignore EPIPE here, see + * http://lists.freedesktop.org/archives/dbus/2008-March/009526.html + */ + + if (_dbus_get_is_errno_eagain_or_ewouldblock () || _dbus_get_is_errno_epipe ()) goto out; else { @@ -807,6 +811,25 @@ do_reading (DBusTransport *transport) } static dbus_bool_t +unix_error_with_read_to_come (DBusTransport *itransport, + DBusWatch *watch, + unsigned int flags) +{ + DBusTransportSocket *transport = (DBusTransportSocket *) itransport; + + if (!(flags & DBUS_WATCH_HANGUP || flags & DBUS_WATCH_ERROR)) + return FALSE; + + /* If we have a read watch enabled ... + we -might have data incoming ... => handle the HANGUP there */ + if (watch != transport->read_watch && + _dbus_watch_get_enabled (transport->read_watch)) + return FALSE; + + return TRUE; +} + +static dbus_bool_t socket_handle_watch (DBusTransport *transport, DBusWatch *watch, unsigned int flags) @@ -817,14 +840,11 @@ socket_handle_watch (DBusTransport *transport, watch == socket_transport->write_watch); _dbus_assert (watch != NULL); - /* Disconnect in case of an error. In case of hangup do not - * disconnect the transport because data can still be in the buffer - * and do_reading may need several iteration to read it all (because - * of its max_bytes_read_per_iteration limit). The condition where - * flags == HANGUP (without READABLE) probably never happen in fact. + /* If we hit an error here on a write watch, don't disconnect the transport yet because data can + * still be in the buffer and do_reading may need several iteration to read + * it all (because of its max_bytes_read_per_iteration limit). */ - if ((flags & DBUS_WATCH_ERROR) || - ((flags & DBUS_WATCH_HANGUP) && !(flags & DBUS_WATCH_READABLE))) + if (!(flags & DBUS_WATCH_READABLE) && unix_error_with_read_to_come (transport, watch, flags)) { _dbus_verbose ("Hang up or error on watch\n"); _dbus_transport_disconnect (transport); diff --git a/dbus/dbus-watch.c b/dbus/dbus-watch.c index 7ef27bf..bca699f 100644 --- a/dbus/dbus-watch.c +++ b/dbus/dbus-watch.c @@ -51,6 +51,12 @@ struct DBusWatch unsigned int enabled : 1; /**< Whether it's enabled. */ }; +dbus_bool_t +_dbus_watch_get_enabled (DBusWatch *watch) +{ + return watch->enabled; +} + /** * Creates a new DBusWatch. Used to add a file descriptor to be polled * by a main loop. diff --git a/dbus/dbus-watch.h b/dbus/dbus-watch.h index fd65ae3..fa953ec 100644 --- a/dbus/dbus-watch.h +++ b/dbus/dbus-watch.h @@ -74,6 +74,7 @@ void _dbus_watch_list_remove_watch (DBusWatchList *watch_li void _dbus_watch_list_toggle_watch (DBusWatchList *watch_list, DBusWatch *watch, dbus_bool_t enabled); +dbus_bool_t _dbus_watch_get_enabled (DBusWatch *watch); /** @} */ -- 1.6.2.5