Attachment 42275 Details for Bug 33336 – Always remove, invalidate and free watches before closing watched sockets

[patch] Always remove, invalidate and free watches before closing watched sockets

0001-Always-remove-invalidate-and-free-watches-before-clo.patch (text/plain), 13.57 KB, created by Simon McVittie on 2011-01-21 08:59:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-01-21 08:59:37 UTC

Size: 13.57 KB

patch

obsolete

>From 30c8afd17523f48d859277525226dfda04011174 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 21 Jan 2011 16:48:37 +0000
>Subject: [PATCH 1/2] Always remove, invalidate and free watches before closing watched sockets
>
>This should mean we don't get invalid fds in the main loop.
>
>The BSD (kqueue) and Windows code paths are untested, but follow the same
>patterns as the tested Linux/generic Unix versions.
>
>DBusTransportSocket was already OK (it called free_watches() before
>_dbus_close_socket, and that did the remove, invalidate, unref dance).
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33336
>---
> bus/dir-watch-inotify.c      |    6 ++-
> bus/dir-watch-kqueue.c       |    6 ++-
> bus/main.c                   |   22 ++++++--
> dbus/dbus-server-socket.c    |    1 +
> dbus/dbus-spawn-win.c        |   36 ++++++++++---
> dbus/dbus-spawn.c            |  122 +++++++++++++++++++++---------------------
> dbus/dbus-transport-socket.c |    2 +
> 7 files changed, 119 insertions(+), 76 deletions(-)
>
>diff --git a/bus/dir-watch-inotify.c b/bus/dir-watch-inotify.c
>index 461b8ee..54704a4 100644
>--- a/bus/dir-watch-inotify.c
>+++ b/bus/dir-watch-inotify.c
>@@ -204,16 +204,18 @@ _shutdown_inotify (void *data)
> 
>   _set_watched_dirs_internal (&empty);
> 
>-  close (inotify_fd);
>-  inotify_fd = -1;
>   if (watch != NULL)
>     {
>       _dbus_loop_remove_watch (loop, watch, _inotify_watch_callback, NULL);
>+      _dbus_watch_invalidate (watch);
>       _dbus_watch_unref (watch);
>       _dbus_loop_unref (loop);
>     }
>   watch = NULL;
>   loop = NULL;
>+
>+  close (inotify_fd);
>+  inotify_fd = -1;
> }
> 
> static int
>diff --git a/bus/dir-watch-kqueue.c b/bus/dir-watch-kqueue.c
>index 4e436eb..62f7b3d 100644
>--- a/bus/dir-watch-kqueue.c
>+++ b/bus/dir-watch-kqueue.c
>@@ -81,6 +81,7 @@ _handle_kqueue_watch (DBusWatch *watch, unsigned int flags, void *data)
>       if (watch != NULL)
> 	{
> 	  _dbus_loop_remove_watch (loop, watch, _kqueue_watch_callback, NULL);
>+          _dbus_watch_invalidate (watch);
>           _dbus_watch_unref (watch);
> 	  watch = NULL;
> 	}
>@@ -124,10 +125,11 @@ _init_kqueue (BusContext *context)
>                                    NULL, NULL))
>           {
>             _dbus_warn ("Unable to add reload watch to main loop");
>-	    close (kq);
>-	    kq = -1;
>+	    _dbus_watch_invalidate (watch);
> 	    _dbus_watch_unref (watch);
> 	    watch = NULL;
>+	    close (kq);
>+	    kq = -1;
>             goto out;
> 	  }
>     }
>diff --git a/bus/main.c b/bus/main.c
>index 53038db..1af588e 100644
>--- a/bus/main.c
>+++ b/bus/main.c
>@@ -43,7 +43,8 @@ static int reload_pipe[2];
> #define RELOAD_READ_END 0
> #define RELOAD_WRITE_END 1
> 
>-static void close_reload_pipe (void);
>+static void close_reload_pipe (DBusWatch **);
>+static void close_reload_pipe_write (void);
> 
> static void
> signal_handler (int sig)
>@@ -64,7 +65,7 @@ signal_handler (int sig)
>             !_dbus_write_socket (reload_pipe[RELOAD_WRITE_END], &str, 0, 1))
>           {
>             _dbus_warn ("Unable to write to reload pipe.\n");
>-            close_reload_pipe ();
>+            close_reload_pipe_write ();
>           }
>       }
>       break;
>@@ -178,7 +179,7 @@ handle_reload_watch (DBusWatch    *watch,
>       _dbus_read_socket (reload_pipe[RELOAD_READ_END], &str, 1) != 1)
>     {
>       _dbus_warn ("Couldn't read from reload pipe.\n");
>-      close_reload_pipe ();
>+      close_reload_pipe (&watch);
>       return TRUE;
>     }
>   _dbus_string_free (&str);
>@@ -249,11 +250,24 @@ setup_reload_pipe (DBusLoop *loop)
> }
> 
> static void
>-close_reload_pipe (void)
>+close_reload_pipe (DBusWatch **watch)
> {
>+    _dbus_loop_remove_watch (bus_context_get_loop (context),
>+        *watch, reload_watch_callback, NULL);
>+    _dbus_watch_invalidate (*watch);
>+    _dbus_watch_unref (*watch);
>+    *watch = NULL;
>+
>     _dbus_close_socket (reload_pipe[RELOAD_READ_END], NULL);
>     reload_pipe[RELOAD_READ_END] = -1;
> 
>+    close_reload_pipe_write ();
>+}
>+
>+/* this is the only bit that's safe to do from an async signal handler */
>+static void
>+close_reload_pipe_write (void)
>+{
>     _dbus_close_socket (reload_pipe[RELOAD_WRITE_END], NULL);
>     reload_pipe[RELOAD_WRITE_END] = -1;
> }
>diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c
>index 758742d..057f7c2 100644
>--- a/dbus/dbus-server-socket.c
>+++ b/dbus/dbus-server-socket.c
>@@ -236,6 +236,7 @@ socket_disconnect (DBusServer *server)
>         {
>           _dbus_server_remove_watch (server,
>                                      socket_server->watch[i]);
>+          _dbus_watch_invalidate (socket_server->watch[i]);
>           _dbus_watch_unref (socket_server->watch[i]);
>           socket_server->watch[i] = NULL;
>         }
>diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c
>index 8ac837e..dfeb44f 100644
>--- a/dbus/dbus-spawn-win.c
>+++ b/dbus/dbus-spawn-win.c
>@@ -154,6 +154,27 @@ _dbus_babysitter_ref (DBusBabysitter *sitter)
>   return sitter;
> }
> 
>+static void
>+close_socket_to_babysitter (DBusBabysitter *sitter)
>+{
>+  _dbus_verbose ("Closing babysitter\n");
>+
>+  if (sitter->sitter_watch != NULL)
>+    {
>+      _dbus_assert (sitter->watches != NULL);
>+      _dbus_watch_list_remove_watch (sitter->watches,  sitter->sitter_watch);
>+      _dbus_watch_invalidate (sitter->sitter_watch);
>+      _dbus_watch_unref (sitter->sitter_watch);
>+      sitter->sitter_watch = NULL;
>+    }
>+
>+  if (sitter->socket_to_babysitter != -1)
>+    {
>+      _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>+      sitter->socket_to_babysitter = -1;
>+    }
>+}
>+
> /**
>  * Decrement the reference count on the babysitter object.
>  *
>@@ -172,11 +193,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
> 
>   if (sitter->refcount == 0)
>     {
>-      if (sitter->socket_to_babysitter != -1)
>-        {
>-          _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>-          sitter->socket_to_babysitter = -1;
>-        }
>+      close_socket_to_babysitter (sitter);
> 
>       if (sitter->socket_to_main != -1)
>         {
>@@ -372,9 +389,8 @@ handle_watch (DBusWatch       *watch,
>    */
> 
>   PING();
>-  _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>+  close_socket_to_babysitter (sitter);
>   PING();
>-  sitter->socket_to_babysitter = -1;
> 
>   return TRUE;
> }
>@@ -668,6 +684,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter           **sitter_p,
>   PING();
>   if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->sitter_watch))
>     {
>+      /* we need to free it early so the destructor won't try to remove it
>+       * without it having been added, which DBusLoop doesn't allow */
>+      _dbus_watch_invalidate (sitter->sitter_watch);
>+      _dbus_watch_unref (sitter->sitter_watch);
>+      sitter->sitter_watch = NULL;
>+
>       _DBUS_SET_OOM (error);
>       goto out0;
>     }
>diff --git a/dbus/dbus-spawn.c b/dbus/dbus-spawn.c
>index a1bab3d..5654380 100644
>--- a/dbus/dbus-spawn.c
>+++ b/dbus/dbus-spawn.c
>@@ -257,6 +257,9 @@ _dbus_babysitter_ref (DBusBabysitter *sitter)
>   return sitter;
> }
> 
>+static void close_socket_to_babysitter  (DBusBabysitter *sitter);
>+static void close_error_pipe_from_child (DBusBabysitter *sitter);
>+
> /**
>  * Decrement the reference count on the babysitter object.
>  * When the reference count of the babysitter object reaches
>@@ -273,25 +276,17 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
>   
>   sitter->refcount -= 1;
>   if (sitter->refcount == 0)
>-    {      
>-      if (sitter->socket_to_babysitter >= 0)
>-        {
>-          /* If we haven't forked other babysitters
>-           * since this babysitter and socket were
>-           * created then this close will cause the
>-           * babysitter to wake up from poll with
>-           * a hangup and then the babysitter will
>-           * quit itself.
>-           */
>-          _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>-          sitter->socket_to_babysitter = -1;
>-        }
>+    {
>+      /* If we haven't forked other babysitters
>+       * since this babysitter and socket were
>+       * created then this close will cause the
>+       * babysitter to wake up from poll with
>+       * a hangup and then the babysitter will
>+       * quit itself.
>+       */
>+      close_socket_to_babysitter (sitter);
> 
>-      if (sitter->error_pipe_from_child >= 0)
>-        {
>-          _dbus_close_socket (sitter->error_pipe_from_child, NULL);
>-          sitter->error_pipe_from_child = -1;
>-        }
>+      close_error_pipe_from_child (sitter);
> 
>       if (sitter->sitter_pid > 0)
>         {
>@@ -341,21 +336,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter)
> 
>           sitter->sitter_pid = -1;
>         }
>-      
>-      if (sitter->error_watch)
>-        {
>-          _dbus_watch_invalidate (sitter->error_watch);
>-          _dbus_watch_unref (sitter->error_watch);
>-          sitter->error_watch = NULL;
>-        }
> 
>-      if (sitter->sitter_watch)
>-        {
>-          _dbus_watch_invalidate (sitter->sitter_watch);
>-          _dbus_watch_unref (sitter->sitter_watch);
>-          sitter->sitter_watch = NULL;
>-        }
>-      
>       if (sitter->watches)
>         _dbus_watch_list_free (sitter->watches);
> 
>@@ -476,16 +457,42 @@ static void
> close_socket_to_babysitter (DBusBabysitter *sitter)
> {
>   _dbus_verbose ("Closing babysitter\n");
>-  _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>-  sitter->socket_to_babysitter = -1;
>+
>+  if (sitter->sitter_watch != NULL)
>+    {
>+      _dbus_assert (sitter->watches != NULL);
>+      _dbus_watch_list_remove_watch (sitter->watches,  sitter->sitter_watch);
>+      _dbus_watch_invalidate (sitter->sitter_watch);
>+      _dbus_watch_unref (sitter->sitter_watch);
>+      sitter->sitter_watch = NULL;
>+    }
>+
>+  if (sitter->socket_to_babysitter >= 0)
>+    {
>+      _dbus_close_socket (sitter->socket_to_babysitter, NULL);
>+      sitter->socket_to_babysitter = -1;
>+    }
> }
> 
> static void
> close_error_pipe_from_child (DBusBabysitter *sitter)
> {
>   _dbus_verbose ("Closing child error\n");
>-  _dbus_close_socket (sitter->error_pipe_from_child, NULL);
>-  sitter->error_pipe_from_child = -1;
>+
>+  if (sitter->error_watch != NULL)
>+    {
>+      _dbus_assert (sitter->watches != NULL);
>+      _dbus_watch_list_remove_watch (sitter->watches,  sitter->error_watch);
>+      _dbus_watch_invalidate (sitter->error_watch);
>+      _dbus_watch_unref (sitter->error_watch);
>+      sitter->error_watch = NULL;
>+    }
>+
>+  if (sitter->error_pipe_from_child >= 0)
>+    {
>+      _dbus_close_socket (sitter->error_pipe_from_child, NULL);
>+      sitter->error_pipe_from_child = -1;
>+    }
> }
> 
> static void
>@@ -752,7 +759,7 @@ handle_watch (DBusWatch       *watch,
>               unsigned int     condition,
>               void            *data)
> {
>-  DBusBabysitter *sitter = data;
>+  DBusBabysitter *sitter = _dbus_babysitter_ref (data);
>   int revents;
>   int fd;
>   
>@@ -775,31 +782,12 @@ handle_watch (DBusWatch       *watch,
>          babysitter_iteration (sitter, FALSE))
>     ;
> 
>-  /* Those might have closed the sockets we're watching. Before returning
>-   * to the main loop, we must sort that out. */
>-
>-  if (sitter->error_watch != NULL && sitter->error_pipe_from_child == -1)
>-    {
>-      _dbus_watch_invalidate (sitter->error_watch);
>-
>-      if (sitter->watches != NULL)
>-        _dbus_watch_list_remove_watch (sitter->watches,  sitter->error_watch);
>-
>-      _dbus_watch_unref (sitter->error_watch);
>-      sitter->error_watch = NULL;
>-    }
>-
>-  if (sitter->sitter_watch != NULL && sitter->socket_to_babysitter == -1)
>-    {
>-      _dbus_watch_invalidate (sitter->sitter_watch);
>-
>-      if (sitter->watches != NULL)
>-        _dbus_watch_list_remove_watch (sitter->watches,  sitter->sitter_watch);
>-
>-      _dbus_watch_unref (sitter->sitter_watch);
>-      sitter->sitter_watch = NULL;
>-    }
>+  /* fd.o #32992: if the handle_* methods closed their sockets, they previously
>+   * didn't always remove the watches. Check that we don't regress. */
>+  _dbus_assert (sitter->socket_to_babysitter != -1 || sitter->sitter_watch == NULL);
>+  _dbus_assert (sitter->error_pipe_from_child != -1 || sitter->error_watch == NULL);
> 
>+  _dbus_babysitter_unref (sitter);
>   return TRUE;
> }
> 
>@@ -1189,6 +1177,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter          **sitter_p,
>         
>   if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->error_watch))
>     {
>+      /* we need to free it early so the destructor won't try to remove it
>+       * without it having been added, which DBusLoop doesn't allow */
>+      _dbus_watch_invalidate (sitter->error_watch);
>+      _dbus_watch_unref (sitter->error_watch);
>+      sitter->error_watch = NULL;
>+
>       dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
>       goto cleanup_and_fail;
>     }
>@@ -1204,6 +1198,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter          **sitter_p,
>       
>   if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->sitter_watch))
>     {
>+      /* we need to free it early so the destructor won't try to remove it
>+       * without it having been added, which DBusLoop doesn't allow */
>+      _dbus_watch_invalidate (sitter->sitter_watch);
>+      _dbus_watch_unref (sitter->sitter_watch);
>+      sitter->sitter_watch = NULL;
>+
>       dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
>       goto cleanup_and_fail;
>     }
>diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
>index 1d4c2bf..1dfb140 100644
>--- a/dbus/dbus-transport-socket.c
>+++ b/dbus/dbus-transport-socket.c
>@@ -1278,8 +1278,10 @@ _dbus_transport_new_for_socket (int               fd,
>   return (DBusTransport*) socket_transport;
> 
>  failed_4:
>+  _dbus_watch_invalidate (socket_transport->read_watch);
>   _dbus_watch_unref (socket_transport->read_watch);
>  failed_3:
>+  _dbus_watch_invalidate (socket_transport->write_watch);
>   _dbus_watch_unref (socket_transport->write_watch);
>  failed_2:
>   _dbus_string_free (&socket_transport->encoded_incoming);
>-- 
>1.7.2.3
>

Actions: View | Splinter Review

Attachments on bug 33336: 42275 | 42276 | 43752 | 43753

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.