diff --git a/bus/dir-watch-inotify.c b/bus/dir-watch-inotify.c index 461b8ee3..54704a4f 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 4e436eb1..62f7b3dc 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 53038dbd..1af588e3 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-mainloop.c b/dbus/dbus-mainloop.c index 43159a70..03e9d1c1 100644 --- 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 e8a24e48..02973c19 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 8ac837ed..dfeb44f6 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 a1bab3df..5654380d 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 1d4c2bf7..1dfb1408 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);