GNOME Bugzilla – Bug 748382
GSocketListener memory corruption
Last modified: 2018-05-24 17:48:05 UTC
Created attachment 302261 [details] [review] gsocketlistener: Fix memory corruption The implementation of g_socket_listener_accept_socket_async() assumes that the task will get destroyed before returning from accept_ready(), and hence it can just tie the life-time of the sources to the life- time of the task, and drop the task reference before returning from accept_ready(). However, if an idle source has to be scheduled in order to deliver the task's result, this means that the task, and consequently also its sources, will stay alive a bit longer. By doing so, additional callbacks to accept_ready() may happen in the meantime, i.e. more than one socket might be ready for dispatching, and it will start dropping references it doesn't own.
My test-case that accidentally triggered this bug: https://github.com/frida/frida-gum/blob/9bcd4e5523b553e28584ca8dc2eafdb8320a1981/tests/core/script.c#L421-L477
(In reply to Ole André Vadla Ravnås from comment #0) > However, if an idle source has to be scheduled in order > to deliver the task's result, this means that the task, and consequently > also its sources, will stay alive a bit longer. While that statement is true, I don't see any way that it should be possible to hit that case; accept_ready() should always be called in the same GMainContext as the task was created from, and always in a later iteration of that GMainContext. So it always ought to be possible to run the callback immediately. Or at least, that's how it looks. Can you figure out why that's not happening in your code? It would be awesome if you could extract a simple test case out of your code to add to gio/tests/socket-listener.c... Beyond that, assuming that the code is correct/necessary, it seems weird to use g_task_set_task_data() solely for its side effect of calling the GDestroyNotify. I think I'd just pass NULL for the destroy_notify when initially setting the task_data, and then call free_sources() explicitly in accept_ready().
(In reply to Dan Winship from comment #2) > (In reply to Ole André Vadla Ravnås from comment #0) > > However, if an idle source has to be scheduled in order > > to deliver the task's result, this means that the task, and consequently > > also its sources, will stay alive a bit longer. > > While that statement is true, I don't see any way that it should be possible > to hit that case; accept_ready() should always be called in the same > GMainContext as the task was created from, and always in a later iteration > of that GMainContext. So it always ought to be possible to run the callback > immediately. Or at least, that's how it looks. Can you figure out why that's > not happening in your code? That didn't happen in my code because I had two sources, and I observed that the callback sometimes happened twice as they were both signalled in the same iteration. My understanding is that GTask's idle source will then get processed only after these existing sources have been processed. What seemed to happen in my case was that I often got "lucky" as I had just accepted a short-lived connection on one source, right before shutting down everything, which resulted in both sources being ready in the same context iteration. > It would be awesome if you could extract a simple test case out of your code > to add to gio/tests/socket-listener.c... I'm currently swamped with work so I won't be able to do a big enough context-switch back to this in the near-term, but I'll give it a try as soon as I find a big enough chunk of time to start submitting some of the other patches as well. (Keeping them here: https://github.com/frida/glib) > Beyond that, assuming that the code is correct/necessary, it seems weird to > use g_task_set_task_data() solely for its side effect of calling the > GDestroyNotify. I think I'd just pass NULL for the destroy_notify when > initially setting the task_data, and then call free_sources() explicitly in > accept_ready(). Ahh yes, that seems really weird in retrospect, wonder what I was smoking there! :) I'll update the patch.
Created attachment 302299 [details] [review] gsocketlistener: Fix memory corruption Updated patch.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1030.