[Spice-devel] [spice-gtk 06/13] http-proxy: add https proxy

[Christophe Fergeau]

On Wed, Feb 12, 2014 at 12:04:25PM +0100, Marc-André Lureau wrote:
> On Wed, Feb 12, 2014 at 11:59 AM, Christophe Fergeau
> <cfergeau at redhat.com> wrote:
> > Missed that initially, but tlsconn is leaked in error cases, and I think we
> > will leak a reference to it in success case too. My understanding is that
> 
> if error, function returns NULL

tlsconn = g_tls_client_connection_new (io_stream,
                                       G_SOCKET_CONNECTABLE(proxy_address),
                                       error);

if (!g_tls_connection_handshake (G_TLS_CONNECTION (tlsconn), cancellable, error))
    goto error;

error:
  if (data_in != NULL)
    g_object_unref (data_in);

  g_free (buffer);
  return NULL;
}

tlsconn will be leaked.


> 
> > we don't own the io_stream passed as an argument, so we need to ref it in
> > the http case, but in the https case, we already own a ref on the io_stream
> > (which is tlsconn), so we ref it one too many.
> 
> Where do you see we ref something?
> I remember I did check with valgrind and debugging than all references
> where correct. But I could be wrong. Please give further details.
> 

  tlsconn = g_tls_client_connection_new (io_stream,
                                         G_SOCKET_CONNECTABLE(proxy_address),
                                         error);
  io_stream = tlsconn;

  return g_object_ref (io_stream);

I think we are taking an extra ref on tlsconn here (ref which is probably
needed in the !tls case).
> > tlsconn is leaked here (dunno if it's guaranteed to be NULL when error is
> > set).
> >
> Yes it is guaratee, I can add a warning and  g_clear_object if you want.

a g_warn_if_fail() cannot hurt as it's not explicit in the API doc (or
fixing the API doc would work too ;)

> 
> >> +          return;
> >> +        }
> >> +
> >> +      g_return_if_fail (tlsconn != NULL);
> >> +
> >> +      GTlsCertificateFlags tls_validation_flags = G_TLS_CERTIFICATE_VALIDATE_ALL;
> >> +#ifdef DEBUG
> >> +      tls_validation_flags -= G_TLS_CERTIFICATE_UNKNOWN_CA + G_TLS_CERTIFICATE_BAD_IDENTITY;
> >> +#endif
> >> +      g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn),
> >> +                                                    tls_validation_flags);
> >> +      g_tls_connection_handshake_async (G_TLS_CONNECTION (tlsconn),
> >> +                                        G_PRIORITY_DEFAULT, cancellable,
> >> +                                        handshake_completed, data);
> >
> > A g_object_unref(tlsconn) is probably needed here.
> >
> 
> no, the async is running and we need to keep the ref.

Ah, my bad, I thought glib async functions were ref'ing their arg as
needed, but I'm wrong from a quick glance at gio/glib-networking.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140212/abfadfd8/attachment-0001.pgp>