GNOME Bugzilla – Bug 723655
Socket source is left in the poll after the socket is closed
Last modified: 2017-10-25 14:36:32 UTC
GSocketSource is used to poll on a socket, for instance, in GSocketConnection. When the socket is closed, the source should become ready in order for any operations waiting on the socket's sources to be reliably completed. Now the socket's former file descriptor is simply left in the poll, which may lead to it failing to report events if it has been reclaimed for another object.
Created attachment 268133 [details] [review] Remove the socket from poll if it has been closed This prevents polling on file descriptors that are no longer in use or have been reused for something else.
Created attachment 268134 [details] [review] Test /socket/connection-close-with-pending Test that closing a GSocketConnection with a pending read on the input substream causes the read to terminate with the correct error. This might not be the case without the preceding GSocketSource fix, if the file descriptor has been reclaimed by another kernel object that does not poll ready.
The bug here is that g_io_stream_close() succeeds when you have a read pending. It is supposed to fail, as you note in your test case. *** This bug has been marked as a duplicate of bug 707912 ***
Created attachment 268238 [details] [review] Test /socket/source-postmortem Checks that after a GSocket is closed, a source created off it with g_socket_create_source() will dispatch with G_IO_NVAL even if the descriptor used by the socket is reclaimed for something else.
g_io_stream_close() is one thing, but this bug is really about GSocketSource polling for a descriptor that's no longer used for the socket. I have changed the test to illustrate.
Review of attachment 268133 [details] [review]: This looks excellent, thanks! Only problem is it needs rebased; we need to bring back socket_source_prepare, which was removed on non-Windows. Are you still interested in landing this and doing the rebase? If not, I'll try to take care of it.
Review of attachment 268238 [details] [review]: Excellent!
Created attachment 361992 [details] [review] socket: Don't poll the socket fd after close This prevents polling on file descriptors that are no longer in use or have been reused for something else. Based on a patch by Mikhail Zabaluev
Created attachment 361993 [details] [review] Test /socket/source-postmortem Checks that after a GSocket is closed, a source created off it with g_socket_create_source() will dispatch with G_IO_NVAL even if the descriptor used by the socket is reclaimed for something else.
I took a stab at updating the first patch, and added a few more checks inside socket_source_dispatch() that hopefully make sense. But I don't really know what I'm doing. Philip, what do you think?
Created attachment 361994 [details] [review] Test /socket/source-postmortem Checks that after a GSocket is closed, a source created off it with g_socket_create_source() will dispatch with G_IO_NVAL even if the descriptor used by the socket is reclaimed for something else.
Review of attachment 361992 [details] [review]: Looks reasonable to me. Setting to needs-work for the question I have. ::: gio/gsocket.c @@ +3810,3 @@ { socket->priv->timed_out = TRUE; + if (!g_socket_is_closed (socket_source->socket)) Could potentially combine this condition with the (timeout >= 0 && timeout < g_source_get_time()) one above? Do we want to set timed_out but not events?
Review of attachment 361994 [details] [review]: ::: gio/tests/socket.c @@ +1402,3 @@ + +static void +test_source_postmortem (void) Would be good to have a comment above the test function explaining what the test is trying to check, and how it does that. @@ +1434,3 @@ + g_assert_cmpint (fd, !=, -1); + + g_main_context_iteration (NULL, FALSE); Using the global default main context here is a bit suspect. The test would be a bit more robust if a custom main context were used. Then you can also safely assert at the end of the test that there are no more pending sources on that main context.
(In reply to Philip Withnall from comment #12) > Review of attachment 361992 [details] [review] [review]: > > Looks reasonable to me. Setting to needs-work for the question I have. Eh, I was hoping you'd find a bug... touching this code is nerve-wracking. > ::: gio/gsocket.c > @@ +3810,3 @@ > { > socket->priv->timed_out = TRUE; > + if (!g_socket_is_closed (socket_source->socket)) > > Could potentially combine this condition with the (timeout >= 0 && timeout < > g_source_get_time()) one above? Do we want to set timed_out but not events? Good point, I guess it makes sense to set neither. (In reply to Philip Withnall from comment #13) > Would be good to have a comment above the test function explaining what the > test is trying to check, and how it does that. OK. > @@ +1434,3 @@ > + g_assert_cmpint (fd, !=, -1); > + > + g_main_context_iteration (NULL, FALSE); > > Using the global default main context here is a bit suspect. The test would > be a bit more robust if a custom main context were used. > > Then you can also safely assert at the end of the test that there are no > more pending sources on that main context. Good point.
I think the "Reuse the former socket's file descriptor" test isn't actually doing anything, because if the former fd was still in use, dup2 would silently close it. So the test can be simplified a bit more as well. What's important to test is that the callback gets visited, and that (as you noted) there is nothing more pending on the main context.
(In reply to Michael Catanzaro from comment #15) > What's important to test is that the callback gets visited, and that (as you noted) > there is nothing more pending on the main context. Well, ideally we would test that it really gets removed from the poll, which I guess was the intend behind the dup2 test... but that did not work as intended. I don't know if that's actually testable.
Created attachment 362109 [details] [review] socket: Don't poll the socket fd after close This prevents polling on file descriptors that are no longer in use or have been reused for something else. Based on a patch by Mikhail Zabaluev
Created attachment 362110 [details] [review] Add socket postmortem test Checks that after a GSocket is closed, a source created off it with g_socket_create_source() will dispatch exactly once with G_IO_NVAL. Based on a patch by Mikhail Zabaluev
Review of attachment 362110 [details] [review]: ::: gio/tests/socket.c @@ +1402,3 @@ + +static void +test_source_postmortem (void) Comment here seems to have not appeared.
Review of attachment 362109 [details] [review]:
(In reply to Michael Catanzaro from comment #14) > (In reply to Philip Withnall from comment #12) > > Review of attachment 361992 [details] [review] [review] [review]: > > > > Looks reasonable to me. Setting to needs-work for the question I have. > > Eh, I was hoping you'd find a bug... touching this code is nerve-wracking.
(In reply to Philip Withnall from comment #21) > (In reply to Michael Catanzaro from comment #14) > > (In reply to Philip Withnall from comment #12) > > > Review of attachment 361992 [details] [review] [review] [review] [review]: > > > > > > Looks reasonable to me. Setting to needs-work for the question I have. > > > > Eh, I was hoping you'd find a bug... touching this code is nerve-wracking. Bah. There was a witty emoji-based reply in there, but Bugzilla ate it. Boo. :-(
Review of attachment 362110 [details] [review]: ::: gio/tests/socket.c @@ +1425,3 @@ + g_object_unref (socket); + + /* Test that, after a socket is closed, its source callback should be called I put the comment here above the asserts, since none of the other test functions in this file have a comment on top... OK?
(In reply to Michael Catanzaro from comment #23) > Review of attachment 362110 [details] [review] [review]: > > ::: gio/tests/socket.c > @@ +1425,3 @@ > + g_object_unref (socket); > + > + /* Test that, after a socket is closed, its source callback should be > called > > I put the comment here above the asserts, since none of the other test > functions in this file have a comment on top... OK? I would prefer to start the convention that the comment is just above the test function, but it’s not worth respinning the patch for.
Review of attachment 362110 [details] [review]: Make it so.
Attachment 362109 [details] pushed as f99045f - socket: Don't poll the socket fd after close Attachment 362110 [details] pushed as c5202bc - Add socket postmortem test
Sigh, the patch is incomplete. While iterating on this patch, I accidentally dropped a call to g_source_remove_unix_fd() in a refactoring, and forgot to bring it back.
Created attachment 362233 [details] [review] socket: actually remove fd from poll when socket is closed I accidentally dropped a call to g_source_remove_unix_fd() in a refactoring, and forgot to bring it back. The test did not catch this, because the test only checks that the right source callback is dispatched properly. I don't know how to test this.
(In reply to Michael Catanzaro from comment #28) > I accidentally dropped a call to g_source_remove_unix_fd() in a > refactoring, and forgot to bring it back. Well, a bit misleading... the call was never there before, I added it for my patch and then removed it before uploading.
Created attachment 362234 [details] [review] socket: actually remove fd from poll when socket is closed In my previous patch, I failed to call g_source_remove_unix_fd() in order to actually stop polling the fd of the closed socket. The test did not catch this, because the test only checks that the right source callback is dispatched properly. I don't know how to test this.
Review of attachment 362234 [details] [review]: LGTM with this cleanup. ::: gio/gsocket.c @@ +3800,3 @@ + g_source_remove_unix_fd (source, socket_source->fd_tag); + socket_source->fd_tag = NULL; + } Code style: if (socket_source->fd_tag != NULL) g_source_remove_unix_fd (source, socket_source->fd_tag); socket_source->fd_tag = NULL;
Attachment 362234 [details] pushed as 4b07869 - socket: actually remove fd from poll when socket is closed