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

[Marc-André Lureau]

On Wed, Feb 12, 2014 at 12:46 PM, Christophe Fergeau
<cfergeau at redhat.com> wrote:
> 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.

ack

>
>
>>
>> > 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 ;)

I'll check for return value instead.

>
>>
>> >> +          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



-- 
Marc-André Lureau