[libvirt] [libvirt-glib 2/2] Fix potential crashes in error cases

[Martin Kletzander]

On 12/14/2012 05:03 PM, Christophe Fergeau wrote:
> fetch_list implementations in libvirt-gobject-connection.c and
> libvirt-gobject-storage-pool.c can misbehave in error situations
> or when the call is cancelled:
> - when the call is cancelled, 'lst' will be NULL and 'n' non-0 so
> we'll try to iterate over 'lst', which will cause a crash
> - when list_func fails, 'lst' is likely to be uninitialized, which
> will lead to invalid frees in the memory cleanup in the error: branch.
> We can avoid this issue by making sure 'lst' is initialized to 0
> when it's created.
> ---
>  libvirt-gobject/libvirt-gobject-connection.c   | 10 ++++++----
>  libvirt-gobject/libvirt-gobject-storage-pool.c | 10 ++++++----
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c
> index 0525323..6888482 100644
> --- a/libvirt-gobject/libvirt-gobject-connection.c
> +++ b/libvirt-gobject/libvirt-gobject-connection.c
> @@ -613,7 +613,7 @@ static gchar ** fetch_list(virConnectPtr vconn,
>          if (g_cancellable_set_error_if_cancelled(cancellable, err))
>              goto error;
>  
> -        lst = g_new(gchar *, n);
> +        lst = g_new0(gchar *, n);
>          if ((n = list_func(vconn, lst, n)) < 0) {
>              gvir_set_error(err, GVIR_CONNECTION_ERROR,
>                             0,
> @@ -626,9 +626,11 @@ static gchar ** fetch_list(virConnectPtr vconn,
>      return lst;
>  
>  error:
> -    for (i = 0 ; i < n; i++)
> -        g_free(lst[i]);
> -    g_free(lst);
> +    if (lst != NULL) {
> +        for (i = 0 ; i < n; i++)
> +            g_free(lst[i]);
> +        g_free(lst);
> +    }
>      return NULL;
>  }
>  
> diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c
> index 8f579a1..16b39d4 100644
> --- a/libvirt-gobject/libvirt-gobject-storage-pool.c
> +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c
> @@ -333,7 +333,7 @@ static gchar ** fetch_list(virStoragePoolPtr vpool,
>          if (g_cancellable_set_error_if_cancelled(cancellable, err))
>              goto error;
>  
> -        lst = g_new(gchar *, n);
> +        lst = g_new0(gchar *, n);
>          if ((n = list_func(vpool, lst, n)) < 0) {
>              gvir_set_error(err, GVIR_STORAGE_POOL_ERROR,
>                             0,
> @@ -346,9 +346,11 @@ static gchar ** fetch_list(virStoragePoolPtr vpool,
>      return lst;
>  
>  error:
> -    for (i = 0 ; i < n; i++)
> -        g_free(lst[i]);
> -    g_free(lst);
> +    if (lst != NULL) {
> +        for (i = 0 ; i < n; i++)
> +            g_free(lst[i]);
> +        g_free(lst);
> +    }
>      return NULL;
>  }
>  
> 

I see you fixed it both ways (initialization to 0 as well as the check),
so this should be definitely enough.  ACK,

Martin