Attachment 51967 Details for Bug 39720 – Fix confusion between "is it authenticated?" and "try to authenticate"

[patch] Fix confusion between "is it authenticated?" and "try to authenticate"

0001-Fix-confusion-between-is-it-authenticated-and-try-to.patch (text/plain), 12.50 KB, created by Simon McVittie on 2011-10-04 09:47:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-10-04 09:47:46 UTC

Size: 12.50 KB

patch

obsolete

>From 5712d4ff36bc1b23806ca4467e38cbf398c413fa Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 4 Oct 2011 17:02:33 +0100
>Subject: [PATCH] Fix confusion between "is it authenticated?" and "try to
> authenticate"
>
>Historically, _dbus_transport_get_is_authenticated() has had the
>side-effect of trying to advance the authentication state machine (if
>there's enough buffered input to do so). This seems an inappropriate
>activity for what looks like a simple getter.
>
>Split it into _dbus_transport_try_to_authenticate (which does what it
>always used to do) and _dbus_transport_peek_is_authenticated (which
>is the simple getter version).
>
>To minimize the difference in behaviour for the stable branch of D-Bus,
>I've only used _dbus_transport_peek_is_authenticated where it was used
>in an assertion, which should clearly not have side effects (and I've
>checked that the asserting function cannot be called until both
>authentication and authorization have completed). Replacing most of the
>calls to get_is_authenticated with try_to_authenticate is a possible
>piece of future work.
>
>Based on patches from Cosimo Alfarano, who noticed this
>assertion-with-side-effects.
>
>Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
>---
> dbus/dbus-connection.c          |   41 ++++++++++++++++++++++++++------------
> dbus/dbus-transport-protected.h |    2 +-
> dbus/dbus-transport-socket.c    |   16 +++++++-------
> dbus/dbus-transport.c           |   33 ++++++++++++++++++++++++++----
> dbus/dbus-transport.h           |    3 +-
> 5 files changed, 67 insertions(+), 28 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index 8b8310d..c993234 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -476,9 +476,9 @@ _dbus_connection_queue_received_message_link (DBusConnection  *connection,
>   DBusPendingCall *pending;
>   dbus_uint32_t reply_serial;
>   DBusMessage *message;
>-  
>-  _dbus_assert (_dbus_transport_get_is_authenticated (connection->transport));
>-  
>+
>+  _dbus_assert (_dbus_transport_peek_is_authenticated (connection->transport));
>+
>   _dbus_list_append_link (&connection->incoming_messages,
>                           link);
>   message = link->data;
>@@ -2939,9 +2939,12 @@ dbus_connection_get_is_authenticated (DBusConnection *connection)
>   dbus_bool_t res;
> 
>   _dbus_return_val_if_fail (connection != NULL, FALSE);
>-  
>+
>+  /* Note that despite its name, this function has side-effects: it tries
>+   * to advance the authentication process. */
>+
>   CONNECTION_LOCK (connection);
>-  res = _dbus_transport_get_is_authenticated (connection->transport);
>+  res = _dbus_transport_try_to_authenticate (connection->transport);
>   CONNECTION_UNLOCK (connection);
>   
>   return res;
>@@ -5090,10 +5093,13 @@ dbus_connection_get_unix_user (DBusConnection *connection,
> 
>   _dbus_return_val_if_fail (connection != NULL, FALSE);
>   _dbus_return_val_if_fail (uid != NULL, FALSE);
>-  
>+
>+  /* Note that despite its name, this function has side-effects: it tries
>+   * to advance the authentication process. */
>+
>   CONNECTION_LOCK (connection);
> 
>-  if (!_dbus_transport_get_is_authenticated (connection->transport))
>+  if (!_dbus_transport_try_to_authenticate (connection->transport))
>     result = FALSE;
>   else
>     result = _dbus_transport_get_unix_user (connection->transport,
>@@ -5126,10 +5132,13 @@ dbus_connection_get_unix_process_id (DBusConnection *connection,
> 
>   _dbus_return_val_if_fail (connection != NULL, FALSE);
>   _dbus_return_val_if_fail (pid != NULL, FALSE);
>-  
>+
>+  /* Note that despite its name, this function has side-effects: it tries
>+   * to advance the authentication process. */
>+
>   CONNECTION_LOCK (connection);
> 
>-  if (!_dbus_transport_get_is_authenticated (connection->transport))
>+  if (!_dbus_transport_try_to_authenticate (connection->transport))
>     result = FALSE;
>   else
>     result = _dbus_transport_get_unix_process_id (connection->transport,
>@@ -5160,10 +5169,13 @@ dbus_connection_get_adt_audit_session_data (DBusConnection *connection,
>   _dbus_return_val_if_fail (connection != NULL, FALSE);
>   _dbus_return_val_if_fail (data != NULL, FALSE);
>   _dbus_return_val_if_fail (data_size != NULL, FALSE);
>-  
>+
>+  /* Note that despite its name, this function has side-effects: it tries
>+   * to advance the authentication process. */
>+
>   CONNECTION_LOCK (connection);
> 
>-  if (!_dbus_transport_get_is_authenticated (connection->transport))
>+  if (!_dbus_transport_try_to_authenticate (connection->transport))
>     result = FALSE;
>   else
>     result = _dbus_transport_get_adt_audit_session_data (connection->transport,
>@@ -5256,10 +5268,13 @@ dbus_connection_get_windows_user (DBusConnection             *connection,
> 
>   _dbus_return_val_if_fail (connection != NULL, FALSE);
>   _dbus_return_val_if_fail (windows_sid_p != NULL, FALSE);
>-  
>+
>+  /* Note that despite its name, this function has side-effects: it tries
>+   * to advance the authentication process. */
>+
>   CONNECTION_LOCK (connection);
> 
>-  if (!_dbus_transport_get_is_authenticated (connection->transport))
>+  if (!_dbus_transport_try_to_authenticate (connection->transport))
>     result = FALSE;
>   else
>     result = _dbus_transport_get_windows_user (connection->transport,
>diff --git a/dbus/dbus-transport-protected.h b/dbus/dbus-transport-protected.h
>index 44b9d78..396f0ff 100644
>--- a/dbus/dbus-transport-protected.h
>+++ b/dbus/dbus-transport-protected.h
>@@ -111,7 +111,7 @@ struct DBusTransport
>   DBusFreeFunction free_windows_user_data;            /**< Function to free windows_user_data */
>   
>   unsigned int disconnected : 1;              /**< #TRUE if we are disconnected. */
>-  unsigned int authenticated : 1;             /**< Cache of auth state; use _dbus_transport_get_is_authenticated() to query value */
>+  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 */
>   unsigned int receive_credentials_pending : 1; /**< #TRUE if we need to receive credentials */
>   unsigned int is_server : 1;                 /**< #TRUE if on the server side */
>diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
>index 1d4c2bf..956fe7c 100644
>--- a/dbus/dbus-transport-socket.c
>+++ b/dbus/dbus-transport-socket.c
>@@ -135,7 +135,7 @@ check_write_watch (DBusTransport *transport)
>   
>   _dbus_transport_ref (transport);
> 
>-  if (_dbus_transport_get_is_authenticated (transport))
>+  if (_dbus_transport_try_to_authenticate (transport))
>     needed = _dbus_connection_has_messages_to_send_unlocked (transport->connection);
>   else
>     {
>@@ -190,7 +190,7 @@ check_read_watch (DBusTransport *transport)
>   
>   _dbus_transport_ref (transport);
> 
>-  if (_dbus_transport_get_is_authenticated (transport))
>+  if (_dbus_transport_try_to_authenticate (transport))
>     need_read_watch =
>       (_dbus_counter_get_size_value (transport->live_messages) < transport->max_live_messages_size) &&
>       (_dbus_counter_get_unix_fd_value (transport->live_messages) < transport->max_live_messages_unix_fds);
>@@ -404,7 +404,7 @@ do_authentication (DBusTransport *transport,
> 
>   oom = FALSE;
>   
>-  orig_auth_state = _dbus_transport_get_is_authenticated (transport);
>+  orig_auth_state = _dbus_transport_try_to_authenticate (transport);
> 
>   /* This is essential to avoid the check_write_watch() at the end,
>    * we don't want to add a write watch in do_iteration before
>@@ -419,7 +419,7 @@ do_authentication (DBusTransport *transport,
>   
>   _dbus_transport_ref (transport);
>   
>-  while (!_dbus_transport_get_is_authenticated (transport) &&
>+  while (!_dbus_transport_try_to_authenticate (transport) &&
>          _dbus_transport_get_is_connected (transport))
>     {      
>       if (!exchange_credentials (transport, do_reading, do_writing))
>@@ -477,7 +477,7 @@ do_authentication (DBusTransport *transport,
> 
>  out:
>   if (auth_completed)
>-    *auth_completed = (orig_auth_state != _dbus_transport_get_is_authenticated (transport));
>+    *auth_completed = (orig_auth_state != _dbus_transport_try_to_authenticate (transport));
>   
>   check_read_watch (transport);
>   check_write_watch (transport);
>@@ -498,7 +498,7 @@ do_writing (DBusTransport *transport)
>   dbus_bool_t oom;
>   
>   /* No messages without authentication! */
>-  if (!_dbus_transport_get_is_authenticated (transport))
>+  if (!_dbus_transport_try_to_authenticate (transport))
>     {
>       _dbus_verbose ("Not authenticated, not writing anything\n");
>       return TRUE;
>@@ -703,7 +703,7 @@ do_reading (DBusTransport *transport)
>   _dbus_verbose ("fd = %d\n",socket_transport->fd);
>   
>   /* No messages without authentication! */
>-  if (!_dbus_transport_get_is_authenticated (transport))
>+  if (!_dbus_transport_try_to_authenticate (transport))
>     return TRUE;
> 
>   oom = FALSE;
>@@ -1055,7 +1055,7 @@ socket_do_iteration (DBusTransport *transport,
>   poll_fd.fd = socket_transport->fd;
>   poll_fd.events = 0;
>   
>-  if (_dbus_transport_get_is_authenticated (transport))
>+  if (_dbus_transport_try_to_authenticate (transport))
>     {
>       /* This is kind of a hack; if we have stuff to write, then try
>        * to avoid the poll. This is probably about a 5% speedup on an
>diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c
>index ba6bd4b..9e51ec7 100644
>--- a/dbus/dbus-transport.c
>+++ b/dbus/dbus-transport.c
>@@ -682,10 +682,33 @@ auth_via_default_rules (DBusTransport *transport)
>   return allow;
> }
> 
>+/**
>+ * Returns #TRUE if we have been authenticated. It will return #TRUE even if
>+ * the transport is now disconnected, but was ever authenticated before
>+ * disconnecting.
>+ *
>+ * This replaces the older _dbus_transport_get_is_authenticated() which
>+ * had side-effects.
>+ *
>+ * @param transport the transport
>+ * @returns whether we're authenticated
>+ */
>+dbus_bool_t
>+_dbus_transport_peek_is_authenticated (DBusTransport *transport)
>+{
>+  return transport->authenticated;
>+}
> 
> /**
>- * Returns #TRUE if we have been authenticated.  Will return #TRUE
>- * even if the transport is disconnected.
>+ * Returns #TRUE if we have been authenticated. It will return #TRUE even if
>+ * the transport is now disconnected, but was ever authenticated before
>+ * disconnecting.
>+ *
>+ * If we have not finished authenticating, but we have enough buffered input
>+ * to finish the job, then this function will do so before it returns.
>+ *
>+ * This used to be called _dbus_transport_get_is_authenticated(), but that
>+ * name seems inappropriate for a function with side-effects.
>  *
>  * @todo we drop connection->mutex when calling the unix_user_function,
>  * and windows_user_function, which may not be safe really.
>@@ -694,7 +717,7 @@ auth_via_default_rules (DBusTransport *transport)
>  * @returns whether we're authenticated
>  */
> dbus_bool_t
>-_dbus_transport_get_is_authenticated (DBusTransport *transport)
>+_dbus_transport_try_to_authenticate (DBusTransport *transport)
> {  
>   if (transport->authenticated)
>     return TRUE;
>@@ -1081,12 +1104,12 @@ _dbus_transport_get_dispatch_status (DBusTransport *transport)
>       _dbus_counter_get_unix_fd_value (transport->live_messages) >= transport->max_live_messages_unix_fds)
>     return DBUS_DISPATCH_COMPLETE; /* complete for now */
> 
>-  if (!_dbus_transport_get_is_authenticated (transport))
>+  if (!_dbus_transport_try_to_authenticate (transport))
>     {
>       if (_dbus_auth_do_work (transport->auth) ==
>           DBUS_AUTH_STATE_WAITING_FOR_MEMORY)
>         return DBUS_DISPATCH_NEED_MEMORY;
>-      else if (!_dbus_transport_get_is_authenticated (transport))
>+      else if (!_dbus_transport_try_to_authenticate (transport))
>         return DBUS_DISPATCH_COMPLETE;
>     }
> 
>diff --git a/dbus/dbus-transport.h b/dbus/dbus-transport.h
>index 0db048a..7a3a5e3 100644
>--- a/dbus/dbus-transport.h
>+++ b/dbus/dbus-transport.h
>@@ -38,7 +38,8 @@ DBusTransport*     _dbus_transport_ref                    (DBusTransport
> void               _dbus_transport_unref                  (DBusTransport              *transport);
> void               _dbus_transport_disconnect             (DBusTransport              *transport);
> dbus_bool_t        _dbus_transport_get_is_connected       (DBusTransport              *transport);
>-dbus_bool_t        _dbus_transport_get_is_authenticated   (DBusTransport              *transport);
>+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);
> dbus_bool_t        _dbus_transport_can_pass_unix_fd       (DBusTransport              *transport);
> 
>-- 
>1.7.6.3
>

Actions: View | Splinter Review

Attachments on bug 39720: 51903 | 51904 | 51905 | 51906 | 51967 | 52219 | 52220 | 52221 | 52222 | 52223 | 52224 | 52225 | 52469 | 57713 | 58314 | 58315 | 58316 | 84480 | 84481

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.