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>
https://bugs.freedesktop.org/show_bug.cgi?id=39720
Reviewed-by: Cosimo Alfarano <cosimo.alfarano@collabora.com>

diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 51c0a25..a6caaea 100644 (file)
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -490,9 +490,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;
@@ -2977,9 +2977,9 @@ dbus_connection_get_is_authenticated (DBusConnection *connection)
   dbus_bool_t res;
 
   _dbus_return_val_if_fail (connection != NULL, FALSE);
-  
+
   CONNECTION_LOCK (connection);
-  res = _dbus_transport_get_is_authenticated (connection->transport);
+  res = _dbus_transport_try_to_authenticate (connection->transport);
   CONNECTION_UNLOCK (connection);
   
   return res;
@@ -5174,10 +5174,10 @@ dbus_connection_get_unix_user (DBusConnection *connection,
 
   _dbus_return_val_if_fail (connection != NULL, FALSE);
   _dbus_return_val_if_fail (uid != NULL, FALSE);
-  
+
   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,
@@ -5210,10 +5210,10 @@ dbus_connection_get_unix_process_id (DBusConnection *connection,
 
   _dbus_return_val_if_fail (connection != NULL, FALSE);
   _dbus_return_val_if_fail (pid != NULL, FALSE);
-  
+
   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,
@@ -5245,10 +5245,10 @@ 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);
-  
+
   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,
@@ -5341,10 +5341,10 @@ 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);
-  
+
   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 (file)
--- 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 acb91cf..c7c6291 100644 (file)
--- 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 47437f2..e68a9f0 100644 (file)
--- a/dbus/dbus-transport.c
+++ b/dbus/dbus-transport.c
@@ -686,10 +686,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.
@@ -698,7 +721,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;
@@ -1085,12 +1108,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 4b82151..80fa24e 100644 (file)
--- 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);