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

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