From 5712d4ff36bc1b23806ca4467e38cbf398c413fa Mon Sep 17 00:00:00 2001 From: Simon McVittie 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 --- 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