On Wed, Feb 12, 2014 at 12:46 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel