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 51c0a25cffbc4c679bb23e4e86d56db097edb357..a6caaea6298f3bad4650d66f7cf2fa8ca138bce2 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 44b9d7857b0d55cea64ca8d32da5bf6a107832a2..396f0ffde7e38212ec5c97b8c5cfb71838eafaa0 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 acb91cf841a1094ce3eec258a0068ecd6bf22bbd..c7c62918725ffdaa99c7b3655f4f725579a07c67 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 47437f21942e9b77ab923a908afb29d2c5e34944..e68a9f039db6b26ecc92321f539bf76d3f70b750 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 4b8215176269267ccec395d3ed637b7bbccf9e5a..80fa24efebfa51ce9d6bde88e5997ca9849b7f7c 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);