Attachment 135639 Details for Bug 89104 – [05/12] _dbus_server_new_for_socket: Properly disconnect during error unwinding

[patch] [05/12] _dbus_server_new_for_socket: Properly disconnect during error unwinding

0005-_dbus_server_new_for_socket-Properly-disconnect-duri.patch (text/plain), 4.56 KB, created by Simon McVittie on 2017-11-21 16:29:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2017-11-21 16:29:34 UTC

Size: 4.56 KB

patch

obsolete

>From 524f79e86144b3e1b722f102f89ee070a9586278 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <smcv@collabora.com>
>Date: Tue, 21 Nov 2017 14:38:13 +0000
>Subject: [PATCH 05/12] _dbus_server_new_for_socket: Properly disconnect during
> error unwinding
>
>_dbus_server_finalize_base() asserts that the socket has been
>disconnected, but in some OOM code paths we would call it without
>officially disconnecting. Do so.
>
>This means we need to be a bit more careful about what is
>socket_disconnect()'s responsibility to clean up, what is
>_dbus_server_new_for_socket()'s responsibility, and what is the caller's
>responsibility.
>
>Signed-off-by: Simon McVittie <smcv@collabora.com>
>---
> dbus/dbus-server-protected.h |  1 +
> dbus/dbus-server-socket.c    | 27 ++++++++++++++++++++++-----
> dbus/dbus-server.c           | 24 +++++++++++++++---------
> 3 files changed, 38 insertions(+), 14 deletions(-)
>
>diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h
>index f613bf34..5664b2b4 100644
>--- a/dbus/dbus-server-protected.h
>+++ b/dbus/dbus-server-protected.h
>@@ -96,6 +96,7 @@ dbus_bool_t _dbus_server_init_base      (DBusServer             *server,
>                                          const DBusString       *address,
>                                          DBusError              *error);
> void        _dbus_server_finalize_base  (DBusServer             *server);
>+void        _dbus_server_disconnect_unlocked (DBusServer        *server);
> dbus_bool_t _dbus_server_add_watch      (DBusServer             *server,
>                                          DBusWatch              *watch);
> void        _dbus_server_remove_watch   (DBusServer             *server,
>diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c
>index b5179be6..460b535c 100644
>--- a/dbus/dbus-server-socket.c
>+++ b/dbus/dbus-server-socket.c
>@@ -241,8 +241,11 @@ socket_disconnect (DBusServer *server)
>           socket_server->watch[i] = NULL;
>         }
> 
>-      _dbus_close_socket (socket_server->fds[i], NULL);
>-      _dbus_socket_invalidate (&socket_server->fds[i]);
>+      if (_dbus_socket_is_valid (socket_server->fds[i]))
>+        {
>+          _dbus_close_socket (socket_server->fds[i], NULL);
>+          _dbus_socket_invalidate (&socket_server->fds[i]);
>+        }
>     }
> 
>   if (socket_server->socket_name != NULL)
>@@ -336,10 +339,24 @@ _dbus_server_new_for_socket (DBusSocket       *fds,
>                                    socket_server->watch[i]))
>         {
>           int j;
>-          for (j = 0 ; j < i ; j++)
>-            _dbus_server_remove_watch (server,
>-                                       socket_server->watch[j]);
> 
>+          /* The caller is still responsible for closing the fds until
>+           * we return successfully, so don't let socket_disconnect()
>+           * close them */
>+          for (j = 0; j < n_fds; j++)
>+            _dbus_socket_invalidate (&socket_server->fds[i]);
>+
>+          /* socket_disconnect() will try to remove all the watches;
>+           * make sure it doesn't see the ones that weren't even added
>+           * yet */
>+          for (j = i; j < n_fds; j++)
>+            {
>+              _dbus_watch_invalidate (socket_server->watch[i]);
>+              _dbus_watch_unref (socket_server->watch[i]);
>+              socket_server->watch[i] = NULL;
>+            }
>+
>+          _dbus_server_disconnect_unlocked (server);
>           SERVER_UNLOCK (server);
>           _dbus_server_finalize_base (&socket_server->base);
>           goto failed_2;
>diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c
>index ea9aff2d..d91df832 100644
>--- a/dbus/dbus-server.c
>+++ b/dbus/dbus-server.c
>@@ -764,6 +764,20 @@ dbus_server_unref (DBusServer *server)
>     }
> }
> 
>+void
>+_dbus_server_disconnect_unlocked (DBusServer *server)
>+{
>+  _dbus_assert (server->vtable->disconnect != NULL);
>+
>+  if (!server->disconnected)
>+    {
>+      /* this has to be first so recursive calls to disconnect don't happen */
>+      server->disconnected = TRUE;
>+
>+      (* server->vtable->disconnect) (server);
>+    }
>+}
>+
> /**
>  * Releases the server's address and stops listening for
>  * new clients. If called more than once, only the first
>@@ -780,15 +794,7 @@ dbus_server_disconnect (DBusServer *server)
>   dbus_server_ref (server);
>   SERVER_LOCK (server);
> 
>-  _dbus_assert (server->vtable->disconnect != NULL);
>-
>-  if (!server->disconnected)
>-    {
>-      /* this has to be first so recursive calls to disconnect don't happen */
>-      server->disconnected = TRUE;
>-      
>-      (* server->vtable->disconnect) (server);
>-    }
>+  _dbus_server_disconnect_unlocked (server);
> 
>   SERVER_UNLOCK (server);
>   dbus_server_unref (server);
>-- 
>2.15.0
>

Actions: View | Splinter Review

Attachments on bug 89104: 135264 | 135265 | 135634 | 135635 | 135636 | 135638 | 135639 | 135640 | 135641 | 135642 | 135643 | 135644 | 135645 | 135646 | 135738

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.