[libvirt] Power Hypervisor: Fix potential segfault and memleak in phypOpen

[Daniel P. Berrange]

On Fri, Aug 07, 2009 at 02:50:19PM +0200, Matthias Bolte wrote:
> Hi,
> 
> I came across this line in the phypOpen function:
> 
> char string[strlen(conn->uri->path)];
> 
> Here the path part of the given URI is used without checking it for
> NULL, this can cause a segfault as strlen expects a string != NULL.
> Beside that uuid_db and connection_data leak in case of an error.
> 
> In this line
> 
> conn->uri->path = string;
> 
> the original path of the URI leaks. The patch adds a VIR_FREE call
> before setting the new path.
> 
> The attached patch is compile-tested but I don't have a Power
> Hypervisor installation at hand to test it for real.
> 
> Matthias

> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index cbfd31b..f21ae64 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -61,25 +61,17 @@ static virDrvOpenStatus
>  phypOpen(virConnectPtr conn,
>           virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
>  {
> -    SSH_SESSION *session;
> -    ConnectionData *connection_data;
> -    char string[strlen(conn->uri->path)];
> -
> +    SSH_SESSION *session = NULL;
> +    ConnectionData *connection_data = NULL;
> +    char *string = NULL;
>      uuid_dbPtr uuid_db = NULL;
>  
> -    if (VIR_ALLOC(uuid_db) < 0)
> -        virReportOOMError(conn);
> -
> -    if (VIR_ALLOC(connection_data) < 0)
> -        virReportOOMError(conn);
> -
>      if (!conn || !conn->uri)
>          return VIR_DRV_OPEN_DECLINED;
>  
>      if (conn->uri->scheme == NULL || STRNEQ(conn->uri->scheme, "phyp"))
>          return VIR_DRV_OPEN_DECLINED;
>  
> -
>      if (conn->uri->server == NULL) {
>          virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
>                        VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
> @@ -94,20 +86,36 @@ phypOpen(virConnectPtr conn,
>          return VIR_DRV_OPEN_ERROR;
>      }
>  
> +    if (VIR_ALLOC(uuid_db) < 0) {
> +        virReportOOMError(conn);
> +        goto failure;
> +    }
> +
> +    if (VIR_ALLOC(connection_data) < 0) {
> +        virReportOOMError(conn);
> +        goto failure;
> +    }
> +
> +    if (VIR_ALLOC_N(string, strlen(conn->uri->path) + 1) < 0) {
> +        virReportOOMError(conn);
> +        goto failure;
> +    }
> +
>      if (escape_specialcharacters(conn->uri->path, string) == -1) {
>          virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
>                        VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
>                        _("Error parsing 'path'. Invalid characters."));
> -        return VIR_DRV_OPEN_ERROR;
> +        goto failure;
>      }
>  
>      if ((session = openSSHSession(conn, auth)) == NULL) {
>          virRaiseError(conn, NULL, NULL, 0, VIR_FROM_PHYP,
>                        VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s",
>                        _("Error while opening SSH session."));
> -        return VIR_DRV_OPEN_ERROR;
> +        goto failure;
>      }
>  
> +    VIR_FREE(conn->uri->path);
>      conn->uri->path = string;
>      connection_data->session = session;
>      connection_data->auth = auth;
> @@ -120,6 +128,13 @@ phypOpen(virConnectPtr conn,
>      init_uuid_db(conn);
>  
>      return VIR_DRV_OPEN_SUCCESS;
> +
> +  failure:
> +    VIR_FREE(uuid_db);
> +    VIR_FREE(connection_data);
> +    VIR_FREE(string);
> +
> +    return VIR_DRV_OPEN_ERROR;
>  }
>  
>  static int

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|