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
Reviewed-by: Will Thompson <will.thompson@collabora.co.uk>
Reviewed-by: Thiago Macieira <thiago@kde.org>

diff --git a/bus/dir-watch-inotify.c b/bus/dir-watch-inotify.c
index 461b8ee..54704a4 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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-mainloop.c b/dbus/dbus-mainloop.c
index 43159a7..03e9d1c 100644 (file)
--- a/dbus/dbus-mainloop.c
+++ b/dbus/dbus-mainloop.c
@@ -832,11 +832,14 @@ _dbus_loop_iterate (DBusLoop     *loop,
 
               if (_DBUS_UNLIKELY (fds[i].revents & _DBUS_POLLNVAL))
                 {
+                  DBusWatch *watch = _dbus_watch_ref (wcb->watch);
+
                   _dbus_warn ("invalid request, socket fd %d not open\n",
                       fds[i].fd);
-                  _dbus_watch_invalidate (wcb->watch);
-                  _dbus_loop_remove_watch (loop, wcb->watch, wcb->function,
+                  _dbus_loop_remove_watch (loop, watch, wcb->function,
                       ((Callback *)wcb)->data);
+                  _dbus_watch_invalidate (watch);
+                  _dbus_watch_unref (watch);
                 }
             }
               

diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c
index e8a24e4..02973c1 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);