GNOME Bugzilla – Bug 664141
[socketclient] The socketclient does not clear the last proxy address
Last modified: 2011-11-18 17:37:53 UTC
When doing an asynchronous connect() the ProxyAddress is saved in the callback data, but never cleared. This is has the side effect of leaking a proxy address when there is more then one proxy and at least first one failed, and to try to connect to some random proxy protocols when a proxy settings is followed by direct:// Example: Configuration: socks://mysocks:12345 direct:// Socks connection failed so we skip to direct, as as there is a valid proxy_addr pointer, we try and do a socks handshake on the real destination address.
Created attachment 201479 [details] [review] Clear proxy address
Comment on attachment 201479 [details] [review] Clear proxy address >Subject: [PATCH] [gio] Clear proxy address uppon retry "upon" >to leak or wors, trying to do the proxy handshake on the final "worse" >+ g_clear_object (&data->proxy_addr); > if (G_IS_PROXY_ADDRESS (address) && > data->client->priv->enable_proxy) > data->proxy_addr = g_object_ref (G_PROXY_ADDRESS (address)); I think it would be more consistent with the rest of the code if you cleared proxy_addr from g_socket_client_proxy_connect_callback() when the connection failed, right?
(In reply to comment #2) > (From update of attachment 201479 [details] [review]) > >Subject: [PATCH] [gio] Clear proxy address uppon retry > > "upon" > > >to leak or wors, trying to do the proxy handshake on the final > > "worse" > > >+ g_clear_object (&data->proxy_addr); > > if (G_IS_PROXY_ADDRESS (address) && > > data->client->priv->enable_proxy) > > data->proxy_addr = g_object_ref (G_PROXY_ADDRESS (address)); > > I think it would be more consistent with the rest of the code if you cleared > proxy_addr from g_socket_client_proxy_connect_callback() when the connection > failed, right? I could but that will have the effect that we will free more stuff all over the place. I would suggest that maybe we should do the cleanup right at the beginning of the iterations, which is inside enumerator_next_async(). What do you think of this solution ? I just notice error in the cleanup code: g_object_unref (data->current_socket); data->current_socket = NULL; g_object_unref (data->connection); data->connection = NULL; g_object_unref (data->current_socket); data->current_socket = NULL; We unref twice the current_current socket. This code is rarely reached, most likely the reason we never notice.
(In reply to comment #3) > I could but that will have the effect that we will free more stuff all over the > place. I would suggest that maybe we should do the cleanup right at the > beginning of the iterations, which is inside enumerator_next_async(). What do > you think of this solution ? Mostly, I just think it should be consistent. I don't care which way it is as long as it's the same throughout. > I just notice error in the cleanup code: Oops. Yeah, fix that too. (Probably a merge failure?)
Created attachment 201607 [details] [review] Clear proxy address I've moved all the cleanup at the same place, this make it easier to debug/expand in the future.
Created attachment 201608 [details] [review] Terminate async connection when application proxy is used While doing this, I just notice that asynchronous connection will simply never terminate when we match an application side proxy.
Comment on attachment 201608 [details] [review] Terminate async connection when application proxy is used >+ /* Simply complete the connection, we don't want to do TLS handshake >+ * as the application proxy handling may need proxy handshake first */ can you update the g_socket_client_add_application_proxy() docs to point this out as well? ok to commit with that addition
Done.