Bug 896 - Avoid race conditions reading message from exited process

Patch based on extensive work from Michael Meeks <michael.meeks@novell.com>,
thanks to Dafydd Harries <dafydd.harries@collabora.co.uk>,
Kimmo Hämäläinen <kimmo.hamalainen@nokia.com> 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.
(cherry picked from commit 0e36cdd54964c4012acec2bb8e598b85e82d2846)

diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c
index e0fe888..ccd80cc 100644 (file)
--- 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 2fd5421..8ce6566 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
 
 /** @} */