[Spice-devel] [PATCH spice-server 02/10] reds: Fix leaks if reds_init_client_ssl_connection fails

[Christophe Fergeau]

On Mon, Sep 11, 2017 at 11:47:10AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 11, 2017 at 11:15:39AM +0100, Frediano Ziglio wrote:
> > > If a client is able to complete the TLS handshake phase
> > > reds_init_client_ssl_connection leaked some memory
> > > as the stream is not correctly freed.
> > > This also cause the stream to send the SPICE_CHANNEL_EVENT_DISCONNECTED
> > 
> > "causes"
> > 
> > > event. Otherwise only SPICE_CHANNEL_EVENT_CONNECTED was sent.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/reds.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 24ec2bdde..5da716fc2 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -2503,7 +2503,10 @@ static RedLinkInfo
> > > *reds_init_client_ssl_connection(RedsState *reds, int socket)
> > >      return link;
> > >  
> > >  error:
> > > -    free(link->stream);
> > > +    /* close the stream but do not close the socket, this API is
> > > +     * supposed to not close it if it fails */
> > > +    link->stream->socket = -1;
> > > +    reds_stream_free(link->stream);
> > >      BN_free(link->tiTicketing.bn);
> > >      free(link);
> > 
> > This is getting fairly similar to reds_link_free()?
> > 
> > Christophe
> > 
> 
> yes... wondering why is done this way... maybe to avoid the socket close?

I would guess just we never paid attention to that.

> Maybe in a follow up? This patch try just to avoid the leak.

I think it would be fine for this patch to change error: to
link->stream->socket = -1; reds_link_free(...);

Christophe


> 
> Frediano
> 
> > >      return NULL;