[Spice-devel] [spice-gtk] sasl: Rework memory handling in spice_channel_perform_auth_sasl()

[Marc-André Lureau]

----- Original Message -----
> While looking at the SASL code, I noticed some memory leaks in error paths.
> This commit adds a cleanup: block to free some of the memory dynamically
> allocated in that function, and remove the corresponding g_free() from
> the regular code flow. This should ensure that both the regular path
> and the error paths free the same memory.
> ---
> 
> Something like this?

yes! ack (assuming it is tested :)

> 
> Christophe
> 
>  gtk/spice-channel.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 08418f7..5a94fa0 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1331,7 +1331,7 @@ static gboolean
> spice_channel_perform_auth_sasl(SpiceChannel *channel)
>      };
>      sasl_interact_t *interact = NULL;
>      guint32 len;
> -    char *mechlist;
> +    char *mechlist = NULL;
>      const char *mechname;
>      gboolean ret = FALSE;
>      GSocketAddress *addr = NULL;
> @@ -1386,8 +1386,6 @@ static gboolean
> spice_channel_perform_auth_sasl(SpiceChannel *channel)
>                            saslcb,
>                            SASL_SUCCESS_DATA,
>                            &saslconn);
> -    g_free(localAddr);
> -    g_free(remoteAddr);
>  
>      if (err != SASL_OK) {
>          g_critical("Failed to create SASL client context: %d (%s)",
> @@ -1436,8 +1434,6 @@ static gboolean
> spice_channel_perform_auth_sasl(SpiceChannel *channel)
>      spice_channel_read(channel, mechlist, len);
>      mechlist[len] = '\0';
>      if (c->has_error) {
> -        g_free(mechlist);
> -        mechlist = NULL;
>          goto error;
>      }
>  
> @@ -1453,8 +1449,6 @@ restart:
>      if (err != SASL_OK && err != SASL_CONTINUE && err != SASL_INTERACT) {
>          g_critical("Failed to start SASL negotiation: %d (%s)",
>                     err, sasl_errdetail(saslconn));
> -        g_free(mechlist);
> -        mechlist = NULL;
>          goto error;
>      }
>  
> @@ -1637,15 +1631,22 @@ complete:
>       * is defined to be sent unencrypted, and setting saslconn turns
>       * on the SSF layer encryption processing */
>      c->sasl_conn = saslconn;
> -    return ret;
> +    goto cleanup;
>  
>  error:
> -    g_clear_object(&addr);
>      if (saslconn)
>          sasl_dispose(&saslconn);
>      emit_main_context(channel, SPICE_CHANNEL_EVENT,
>      SPICE_CHANNEL_ERROR_AUTH);
>      c->has_error = TRUE; /* force disconnect */
> -    return FALSE;
> +    ret = FALSE;
> +
> +cleanup:
> +    g_free(localAddr);
> +    g_free(remoteAddr);
> +    g_free(mechlist);
> +    g_free(serverin);
> +    g_clear_object(&addr);
> +    return ret;
>  }
>  #endif /* HAVE_SASL */
>  
> --
> 1.8.3.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>