GNOME Bugzilla – Bug 710088
rtsp-server: Socket ownership handling broken in gst_rtsp_connection_create_from_socket()
Last modified: 2014-02-25 22:28:50 UTC
Created attachment 257240 [details] [review] unref socket after calling connection_create_from_socket() gst_rtsp_server_transfer_connection() calls gst_rtsp_connection_create_from_socket() but doesn't unref the socket afterwards when it should since the name of the function, and also its description, implies that it takes ownership of the socket.
Review of attachment 257240 [details] [review]: This looks wrong. While gst_rtsp_connection_create_from_socket() does take ownership of the socket, it doesn't advertise this via gobject-introspection annotations and also does only take ownership of the socket if everything succeeds. This is not binding friendly behaviour. So what should happen here is that gst_rtsp_connection_create_from_socket() should be fixed. And depending on how it's fixed (IMHO it shouldn't take ownership of the socket because that would now break backwards compat), this fix here needs to be adapted (and if it takes ownership of the socket still, this should be added as gobject-introspection annotation).
this code is a bit confusing, gst_rtsp_connection_create_from_socket() does not take ownership of the socket. It reffs the socket indirectly by calling g_socket_connection_factory_create_connection(socket) which returns GIOStream. The refernce is then dropped from gst_rtsp_connection_close() by unreffing the stream: g_object_unref (conn->stream0); conn->stream0 = NULL; conn->socket0 = NULL;
Comment on attachment 257240 [details] [review] unref socket after calling connection_create_from_socket() I see, makes sense then. I'd like Wim to comment on this before pushing though as it changes behaviour and might not be what he intended.
It's not really my function :) but yes the patch seems correct to me.
commit 7b34d1e9153cdd404fe9996dff4c4837a2865973 Author: Ognyan Tonchev <ognyan@axis.com> Date: Mon Oct 14 12:03:07 2013 +0200 rtsp-server: Fix socket leak https://bugzilla.gnome.org/show_bug.cgi?id=710088