bus: When failing to reload client policy, continue iteration

If we have a large number of connections to the bus, and we fail to
reload the policy for one of them (perhaps because its uid no longer
exists in the system user database), previously we would crash, which
is obviously unintended. After the previous commit, we would stop
iteration through the list of client connections, which doesn't seem
great either: one bad connection shouldn't prevent us from reloading
the rest of our state.

Instead, let's distinguish between new connections (where we want
failure to establish a security policy to be fatal), and pre-existing
connections (where the current security policy is presumably good
enough to keep using if we have nothing better). If we're unable to
reload the policy for a pre-existing connection, log a warning and
carry on iterating.

Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343
Signed-off-by: Simon McVittie <smcv@collabora.com>

3 files changed, 36 insertions, 2 deletions

diff --git a/bus/bus.c b/bus/bus.c
index 2ad8e789..f5100739 100644
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -1285,11 +1285,42 @@ bus_context_get_policy (BusContext *context)
 BusClientPolicy*
 bus_context_create_client_policy (BusContext      *context,
                                   DBusConnection  *connection,
+                                  BusClientPolicy *previous,
                                   DBusError       *error)
 {
+  BusClientPolicy *client;
+  DBusError local_error = DBUS_ERROR_INIT;
+  const char *conn;
+  const char *loginfo;
+
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-  return bus_policy_create_client_policy (context->policy, connection,
-                                          error);
+
+  client = bus_policy_create_client_policy (context->policy, connection,
+                                            &local_error);
+
+  /* On success, use new policy */
+  if (client != NULL)
+    return client;
+
+  /* On failure while setting up a new connection, fail */
+  if (previous == NULL)
+    {
+      dbus_move_error (&local_error, error);
+      return NULL;
+    }
+
+  /* On failure while reloading, keep the previous policy */
+  conn = bus_connection_get_name (connection);
+  loginfo = bus_connection_get_loginfo (connection);
+
+  if (conn == NULL)
+    conn = "(inactive)";
+
+  bus_context_log (context, DBUS_SYSTEM_LOG_WARNING,
+                   "Unable to reload policy for connection \"%s\" (%s), "
+                   "keeping current policy: %s",
+                   conn, loginfo, local_error.message);
+  return bus_client_policy_ref (previous);
 }
 
 int
diff --git a/bus/bus.h b/bus/bus.h
index 2e0de825..6ebea2c8 100644
--- a/bus/bus.h
+++ b/bus/bus.h
@@ -109,6 +109,7 @@ BusPolicy*        bus_context_get_policy                         (BusContext
 
 BusClientPolicy*  bus_context_create_client_policy               (BusContext       *context,
                                                                   DBusConnection   *connection,
+                                                                  BusClientPolicy  *previous,
                                                                   DBusError        *error);
 int               bus_context_get_activation_timeout             (BusContext       *context);
 int               bus_context_get_auth_timeout                   (BusContext       *context);
diff --git a/bus/connection.c b/bus/connection.c
index 8a8ce5f4..b7439296 100644
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -1560,6 +1560,7 @@ bus_connection_complete (DBusConnection   *connection,
 
   d->policy = bus_context_create_client_policy (d->connections->context,
                                                 connection,
+                                                NULL,
                                                 error);
 
   /* we may have a NULL policy on OOM or error getting list of
@@ -1645,6 +1646,7 @@ bus_connections_reload_policy (BusConnections *connections,
 
       policy = bus_context_create_client_policy (connections->context,
                                                  connection,
+                                                 d->policy,
                                                  error);
       if (policy == NULL)
         {