Attachment 107950 Details for Bug 85108 – DBusTransport: record abnormal disconnections

[patch] DBusTransport: record abnormal disconnections

0002-DBusTransport-record-abnormal-disconnections.patch (text/plain), 16.81 KB, created by Simon McVittie on 2014-10-16 19:32:38 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-10-16 19:32:38 UTC

Size: 16.81 KB

patch

obsolete

>From 72cc39a2062e820412b980524e18181c62870433 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Thu, 16 Oct 2014 15:55:19 +0100
>Subject: [PATCH 2/8] DBusTransport: record abnormal disconnections
>
>Normal disconnection is DBUS_ERROR_DISCONNECTED.
>---
> dbus/dbus-connection.c          |  3 +-
> dbus/dbus-transport-protected.h |  1 +
> dbus/dbus-transport-socket.c    | 50 ++++++++++++++++-------
> dbus/dbus-transport.c           | 90 ++++++++++++++++++++++++++++++++++-------
> dbus/dbus-transport.h           |  9 ++++-
> 5 files changed, 122 insertions(+), 31 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index b574207..4036819 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -2866,7 +2866,8 @@ _dbus_connection_close_possibly_shared_and_unlock (DBusConnection *connection)
>    */
>   _dbus_connection_ref_unlocked (connection);
>   
>-  _dbus_transport_disconnect (connection->transport);
>+  _dbus_transport_disconnect (connection->transport, DBUS_ERROR_DISCONNECTED,
>+                              NULL);
> 
>   /* This has the side effect of queuing the disconnect message link
>    * (unless we don't have enough memory, possibly, so don't assert it).
>diff --git a/dbus/dbus-transport-protected.h b/dbus/dbus-transport-protected.h
>index 396f0ff..5f5868c 100644
>--- a/dbus/dbus-transport-protected.h
>+++ b/dbus/dbus-transport-protected.h
>@@ -110,6 +110,7 @@ struct DBusTransport
>   
>   DBusFreeFunction free_windows_user_data;            /**< Function to free windows_user_data */
>   
>+  DBusError disconnect_error;                 /**< Set if we are disconnected. */
>   unsigned int disconnected : 1;              /**< #TRUE if we are disconnected. */
>   unsigned int authenticated : 1;             /**< Cache of auth state; use _dbus_transport_peek_is_authenticated() to query value */
>   unsigned int send_credentials_pending : 1;  /**< #TRUE if we need to send credentials */
>diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
>index c1e4701..2f8cc20 100644
>--- a/dbus/dbus-transport-socket.c
>+++ b/dbus/dbus-transport-socket.c
>@@ -232,10 +232,17 @@ check_read_watch (DBusTransport *transport)
> }
> 
> static void
>-do_io_error (DBusTransport *transport)
>+do_io_error (DBusTransport *transport,
>+             const char *error,
>+             const char *message,
>+             ...)
> {
>+  va_list args;
>+
>   _dbus_transport_ref (transport);
>-  _dbus_transport_disconnect (transport);
>+  va_start (args, message);
>+  _dbus_transport_disconnect_va (transport, error, message, args);
>+  va_end (args);
>   _dbus_transport_unref (transport);
> }
> 
>@@ -279,7 +286,9 @@ read_data_into_auth (DBusTransport *transport,
>         {
>           _dbus_verbose ("Error reading from remote app: %s\n",
>                          _dbus_strerror (saved_errno));
>-          do_io_error (transport);
>+          do_io_error (transport, _dbus_error_from_errno (saved_errno),
>+                       "Error reading from remote app: %s",
>+                       _dbus_strerror (saved_errno));
>         }
> 
>       return FALSE;
>@@ -289,7 +298,7 @@ read_data_into_auth (DBusTransport *transport,
>       _dbus_assert (bytes_read == 0);
>       
>       _dbus_verbose ("Disconnected from remote app\n");
>-      do_io_error (transport);
>+      do_io_error (transport, DBUS_ERROR_DISCONNECTED, NULL);
> 
>       return FALSE;
>     }
>@@ -328,7 +337,9 @@ write_data_from_auth (DBusTransport *transport)
>         {
>           _dbus_verbose ("Error writing to remote app: %s\n",
>                          _dbus_strerror (saved_errno));
>-          do_io_error (transport);
>+          do_io_error (transport, _dbus_error_from_errno (saved_errno),
>+                       "Error writing to remote app: %s",
>+                       _dbus_strerror (saved_errno));
>         }
>     }
> 
>@@ -357,8 +368,9 @@ exchange_credentials (DBusTransport *transport,
>       else
>         {
>           _dbus_verbose ("Failed to write credentials: %s\n", error.message);
>+          do_io_error (transport, error.name,
>+                       "Error writing credentials: %s", error.message);
>           dbus_error_free (&error);
>-          do_io_error (transport);
>         }
>     }
>   
>@@ -380,8 +392,9 @@ exchange_credentials (DBusTransport *transport,
>       else
>         {
>           _dbus_verbose ("Failed to read credentials %s\n", error.message);
>+          do_io_error (transport, error.name,
>+                       "Error reading credentials: %s", error.message);
>           dbus_error_free (&error);
>-          do_io_error (transport);
>         }
>     }
> 
>@@ -423,7 +436,7 @@ do_authentication (DBusTransport *transport,
>   _dbus_transport_ref (transport);
>   
>   while (!_dbus_transport_try_to_authenticate (transport) &&
>-         _dbus_transport_get_is_connected (transport))
>+         _dbus_transport_get_is_connected (transport, NULL))
>     {      
>       if (!exchange_credentials (transport, do_reading, do_writing))
>         {
>@@ -468,7 +481,7 @@ do_authentication (DBusTransport *transport,
>         case DBUS_AUTH_STATE_NEED_DISCONNECT:
>           _dbus_verbose (" %s auth state: need to disconnect\n",
>                          TRANSPORT_SIDE (transport));
>-          do_io_error (transport);
>+          do_io_error (transport, DBUS_ERROR_AUTH_FAILED, NULL);
>           break;
>       
>         case DBUS_AUTH_STATE_AUTHENTICATED:
>@@ -696,7 +709,9 @@ do_writing (DBusTransport *transport)
>             {
>               _dbus_verbose ("Error writing to remote app: %s\n",
>                              _dbus_strerror (saved_errno));
>-              do_io_error (transport);
>+              do_io_error (transport, _dbus_error_from_errno (saved_errno),
>+                           "Error writing to remote app: %s",
>+                           _dbus_strerror (saved_errno));
>               goto out;
>             }
>         }
>@@ -870,14 +885,16 @@ do_reading (DBusTransport *transport)
>         {
>           _dbus_verbose ("Error reading from remote app: %s\n",
>                          _dbus_strerror (saved_errno));
>-          do_io_error (transport);
>+          do_io_error (transport, _dbus_error_from_errno (saved_errno),
>+                       "Error reading from remote app: %s",
>+                       _dbus_strerror (saved_errno));
>           goto out;
>         }
>     }
>   else if (bytes_read == 0)
>     {
>       _dbus_verbose ("Disconnected from remote app\n");
>-      do_io_error (transport);
>+      do_io_error (transport, DBUS_ERROR_DISCONNECTED, NULL);
>       goto out;
>     }
>   else
>@@ -944,7 +961,8 @@ socket_handle_watch (DBusTransport *transport,
>   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);
>+      /* FIXME: should this be treated as abnormal disconnection? */
>+      _dbus_transport_disconnect (transport, DBUS_ERROR_DISCONNECTED, NULL);
>       return TRUE;
>     }
>   
>@@ -1183,7 +1201,11 @@ socket_do_iteration (DBusTransport *transport,
>                                   */
>           
>           if (poll_fd.revents & _DBUS_POLLERR)
>-            do_io_error (transport);
>+            {
>+              do_io_error (transport, _dbus_error_from_errno (saved_errno),
>+                           "Error polling stream: %s",
>+                           _dbus_strerror (saved_errno));
>+            }
>           else
>             {
>               dbus_bool_t need_read = (poll_fd.revents & _DBUS_POLLIN) > 0;
>diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c
>index f63e0ce..5aed2fa 100644
>--- a/dbus/dbus-transport.c
>+++ b/dbus/dbus-transport.c
>@@ -167,6 +167,7 @@ _dbus_transport_init_base (DBusTransport             *transport,
>   transport->live_messages = counter;
>   transport->authenticated = FALSE;
>   transport->disconnected = FALSE;
>+  dbus_error_init (&transport->disconnect_error);
>   transport->is_server = (server_guid != NULL);
>   transport->send_credentials_pending = !transport->is_server;
>   transport->receive_credentials_pending = transport->is_server;
>@@ -216,7 +217,7 @@ void
> _dbus_transport_finalize_base (DBusTransport *transport)
> {
>   if (!transport->disconnected)
>-    _dbus_transport_disconnect (transport);
>+    _dbus_transport_disconnect (transport, DBUS_ERROR_DISCONNECTED, NULL);
> 
>   if (transport->free_unix_user_data != NULL)
>     (* transport->free_unix_user_data) (transport->unix_user_data);
>@@ -233,6 +234,8 @@ _dbus_transport_finalize_base (DBusTransport *transport)
>   dbus_free (transport->expected_guid);
>   if (transport->credentials)
>     _dbus_credentials_unref (transport->credentials);
>+
>+  dbus_error_free (&transport->disconnect_error);
> }
> 
> 
>@@ -499,18 +502,36 @@ _dbus_transport_unref (DBusTransport *transport)
>  * 
>  */
> void
>-_dbus_transport_disconnect (DBusTransport *transport)
>+_dbus_transport_disconnect (DBusTransport *transport,
>+                            const char *error,
>+                            const char *message,
>+                            ...)
>+{
>+  va_list args;
>+
>+  va_start (args, message);
>+  _dbus_transport_disconnect_va (transport, error, message, args);
>+  va_end (args);
>+}
>+
>+void
>+_dbus_transport_disconnect_va (DBusTransport *transport,
>+                               const char *error,
>+                               const char *message,
>+                               va_list args)
> {
>   _dbus_verbose ("start\n");
>   
>+  _dbus_assert (error != NULL);
>   _dbus_assert (transport->vtable->disconnect != NULL);
>   
>   if (transport->disconnected)
>     return;
> 
>   (* transport->vtable->disconnect) (transport);
>-  
>+
>   transport->disconnected = TRUE;
>+  _dbus_set_error_va (&transport->disconnect_error, error, message, args);
> 
>   _dbus_verbose ("end\n");
> }
>@@ -524,9 +545,18 @@ _dbus_transport_disconnect (DBusTransport *transport)
>  * @returns whether we're connected
>  */
> dbus_bool_t
>-_dbus_transport_get_is_connected (DBusTransport *transport)
>+_dbus_transport_get_is_connected (DBusTransport *transport,
>+                                  DBusError     *error)
> {
>-  return !transport->disconnected;
>+  if (transport->disconnected)
>+    {
>+      _dbus_assert (dbus_error_is_set (&transport->disconnect_error));
>+      dbus_set_error (error, transport->disconnect_error.name,
>+                      "%s", transport->disconnect_error.message);
>+      return FALSE;
>+    }
>+
>+  return TRUE;
> }
> 
> static dbus_bool_t
>@@ -568,7 +598,9 @@ auth_via_unix_user_function (DBusTransport *transport)
>       _dbus_verbose ("Client UID "DBUS_UID_FORMAT
>                      " was rejected, disconnecting\n",
>                      _dbus_credentials_get_unix_uid (auth_identity));
>-      _dbus_transport_disconnect (transport);
>+      _dbus_transport_disconnect (transport, DBUS_ERROR_ACCESS_DENIED,
>+                                  "Client UID " DBUS_UID_FORMAT " was rejected",
>+                                  _dbus_credentials_get_unix_uid (auth_identity));
>     }
> 
>   return allow;
>@@ -618,12 +650,20 @@ auth_via_windows_user_function (DBusTransport *transport)
>     {
>       _dbus_verbose ("Client SID '%s' was rejected, disconnecting\n",
>                      _dbus_credentials_get_windows_sid (auth_identity));
>-      _dbus_transport_disconnect (transport);
>+      _dbus_transport_disconnect (transport, DBUS_ERROR_ACCESS_DENIED,
>+                                  "Client SID '%s' was rejected",
>+                                  _dbus_credentials_get_windows_sid (auth_identity));
>     }
> 
>   return allow;
> }
> 
>+static inline const char *
>+unnullify (const char *s)
>+{
>+  return s == NULL ? "<null>" : s;
>+}
>+
> static dbus_bool_t
> auth_via_default_rules (DBusTransport *transport)
> {
>@@ -666,18 +706,30 @@ auth_via_default_rules (DBusTransport *transport)
>   else
>     {
>       if (_dbus_credentials_include(our_identity,DBUS_CREDENTIAL_WINDOWS_SID))
>+        {
>           _dbus_verbose ("Client authorized as SID '%s'"
>                          " but our SID is '%s', disconnecting\n",
>-                         (_dbus_credentials_get_windows_sid(auth_identity) ?
>-                          _dbus_credentials_get_windows_sid(auth_identity) : "<null>"),
>-                         (_dbus_credentials_get_windows_sid(our_identity) ?
>-                          _dbus_credentials_get_windows_sid(our_identity) : "<null>"));
>+                         unnullify (_dbus_credentials_get_windows_sid(auth_identity)),
>+                         unnullify (_dbus_credentials_get_windows_sid(our_identity)));
>+          _dbus_transport_disconnect (transport, DBUS_ERROR_AUTH_FAILED,
>+                                      "Client authorized as SID '%s'"
>+                                      " but our SID is '%s'",
>+                                      unnullify (_dbus_credentials_get_windows_sid(auth_identity)),
>+                                      unnullify (_dbus_credentials_get_windows_sid(our_identity)));
>+        }
>       else
>+        {
>           _dbus_verbose ("Client authorized as UID "DBUS_UID_FORMAT
>                          " but our UID is "DBUS_UID_FORMAT", disconnecting\n",
>                          _dbus_credentials_get_unix_uid(auth_identity),
>                          _dbus_credentials_get_unix_uid(our_identity));
>-      _dbus_transport_disconnect (transport);
>+          _dbus_transport_disconnect (transport, DBUS_ERROR_AUTH_FAILED,
>+                                      "Client authorized as UID " DBUS_UID_FORMAT
>+                                      " but our UID is " DBUS_UID_FORMAT,
>+                                      _dbus_credentials_get_unix_uid (auth_identity),
>+                                      _dbus_credentials_get_unix_uid (our_identity));
>+        }
>+
>       allow = FALSE;
>     }  
> 
>@@ -765,7 +817,10 @@ _dbus_transport_try_to_authenticate (DBusTransport *transport)
>             {
>               _dbus_verbose ("Client expected GUID '%s' and we got '%s' from the server\n",
>                              transport->expected_guid, server_guid);
>-              _dbus_transport_disconnect (transport);
>+              _dbus_transport_disconnect (transport, DBUS_ERROR_AUTH_FAILED,
>+                                          "Expected GUID '%s' does not match server GUID '%s'",
>+                                          transport->expected_guid,
>+                                          server_guid);
>               _dbus_connection_unref_unlocked (transport->connection);
>               return FALSE;
>             }
>@@ -1140,6 +1195,7 @@ dbus_bool_t
> _dbus_transport_queue_messages (DBusTransport *transport)
> {
>   DBusDispatchStatus status;
>+  DBusValidity validity;
> 
> #if 0
>   _dbus_verbose ("_dbus_transport_queue_messages()\n");
>@@ -1180,10 +1236,14 @@ _dbus_transport_queue_messages (DBusTransport *transport)
>         }
>     }
> 
>-  if (_dbus_message_loader_get_is_corrupted (transport->loader))
>+  validity = _dbus_message_loader_get_corruption_reason (transport->loader);
>+
>+  if (validity != DBUS_VALID)
>     {
>       _dbus_verbose ("Corrupted message stream, disconnecting\n");
>-      _dbus_transport_disconnect (transport);
>+      _dbus_transport_disconnect (transport, DBUS_ERROR_INVALID_ARGS,
>+                                  "Disconnecting sender of corrupted message (%s)",
>+                                  _dbus_validity_to_error_message (validity));
>     }
> 
>   return status != DBUS_DISPATCH_NEED_MEMORY;
>diff --git a/dbus/dbus-transport.h b/dbus/dbus-transport.h
>index 39c74c4..c65a17a 100644
>--- a/dbus/dbus-transport.h
>+++ b/dbus/dbus-transport.h
>@@ -36,8 +36,15 @@ DBusTransport*     _dbus_transport_open                   (DBusAddressEntry
>                                                            DBusError                  *error);
> DBusTransport*     _dbus_transport_ref                    (DBusTransport              *transport);
> void               _dbus_transport_unref                  (DBusTransport              *transport);
>-void               _dbus_transport_disconnect             (DBusTransport              *transport);
> dbus_bool_t        _dbus_transport_get_is_connected       (DBusTransport              *transport);
>+void               _dbus_transport_disconnect             (DBusTransport              *transport,
>+                                                           const char                 *error,
>+                                                           const char                 *format,
>+                                                           ...);
>+void               _dbus_transport_disconnect_va          (DBusTransport              *transport,
>+                                                           const char                 *error,
>+                                                           const char                 *format,
>+                                                           va_list                     args);
> dbus_bool_t        _dbus_transport_peek_is_authenticated  (DBusTransport              *transport);
> dbus_bool_t        _dbus_transport_try_to_authenticate    (DBusTransport              *transport);
> dbus_bool_t        _dbus_transport_get_is_anonymous       (DBusTransport              *transport);
>-- 
>2.1.1
>

Actions: View | Splinter Review

Attachments on bug 85108: 107949 | 107950 | 107951 | 107952 | 107953 | 107955 | 107956 | 107957

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.