After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 664141 - [socketclient] The socketclient does not clear the last proxy address
[socketclient] The socketclient does not clear the last proxy address
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
2.28.x
Other Linux
: Normal critical
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-11-15 20:47 UTC by Nicolas Dufresne (ndufresne)
Modified: 2011-11-18 17:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Clear proxy address (955 bytes, patch)
2011-11-15 20:50 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review
Clear proxy address (3.74 KB, patch)
2011-11-17 18:09 UTC, Nicolas Dufresne (ndufresne)
accepted-commit_now Details | Review
Terminate async connection when application proxy is used (1.06 KB, patch)
2011-11-17 18:10 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review

Description Nicolas Dufresne (ndufresne) 2011-11-15 20:47:46 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.
Comment 1 Nicolas Dufresne (ndufresne) 2011-11-15 20:50:18 UTC
Created attachment 201479 [details] [review]
Clear proxy address
Comment 2 Dan Winship 2011-11-17 13:17:42 UTC
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?
Comment 3 Nicolas Dufresne (ndufresne) 2011-11-17 14:19:54 UTC
(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.
Comment 4 Dan Winship 2011-11-17 14:44:47 UTC
(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?)
Comment 5 Nicolas Dufresne (ndufresne) 2011-11-17 18:09:25 UTC
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.
Comment 6 Nicolas Dufresne (ndufresne) 2011-11-17 18:10:54 UTC
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 7 Dan Winship 2011-11-18 00:13:05 UTC
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
Comment 8 Nicolas Dufresne (ndufresne) 2011-11-18 17:37:53 UTC
Done.