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 :|