Re: [libvirt] [PATCH 1/5] phyp: Remove stack allocating a 4kb volume key and fix related memory leaks

Don't pre-allocate 4kb per key, make phypVolumeGetKey allocate the memory. Make phypBuildVolume return the volume key instead of using pre-allocated memory to store it. Also fix a memory leak in phypVolumeLookupByName when phypVolumeGetKey fails. Fix another memory leak in phypVolumeLookupByPath in the success path. Fix phypVolumeGetXMLDesc leaking voldef.key. --- src/phyp/phyp_driver.c | 98 ++++++++++++++++++++++++++--------------------- src/phyp/phyp_driver.h | 1 - 2 files changed, 54 insertions(+), 45 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index ddbc103..bd508fb 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2153,8 +2153,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml) return -1; } -static int -phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) +static char * +phypVolumeGetKey(virConnectPtr conn, const char *name) { ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; @@ -2182,10 +2182,10 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + return NULL; } - cmd = virBufferContentAndReset(&buf); + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) @@ -2196,17 +2196,13 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name) if (char_ptr) *char_ptr = '\0'; - if (memcpy(key, ret, MAX_KEY_SIZE) == NULL) - goto err; - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + return ret; err: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + return NULL; } static char * @@ -2313,9 +2309,9 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) return -1; } -static int +static char * phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, - unsigned int capacity, char *key) + unsigned int capacity) { ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; @@ -2327,6 +2323,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, char *ret = NULL; int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + char *key; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2340,10 +2337,10 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + return NULL; } - cmd = virBufferContentAndReset(&buf); + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { @@ -2351,29 +2348,37 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, goto err; } - if (phypVolumeGetKey(conn, key, lvname) == -1) - goto err;; + key = phypVolumeGetKey(conn, lvname); + + if (key == NULL) + goto err; VIR_FREE(cmd); VIR_FREE(ret); - return 0; + return key; err: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + return NULL; } static virStorageVolPtr phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname) { + char *key; + virStorageVolPtr vol; - char key[MAX_KEY_SIZE]; + key = phypVolumeGetKey(pool->conn, volname); - if (phypVolumeGetKey(pool->conn, key, volname) == -1) + if (key == NULL) return NULL; - return virGetStorageVol(pool->conn, pool->name, volname, key); + vol = virGetStorageVol(pool->conn, pool->name, volname, key); + + VIR_FREE(key); + + return vol; } static virStorageVolPtr @@ -2392,11 +2397,6 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, return NULL; } - if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { - virReportOOMError(); - return NULL; - } - /* Filling spdef manually * */ if (pool->name != NULL) { @@ -2454,9 +2454,10 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, goto err; } - if (phypBuildVolume - (pool->conn, voldef->name, spdef->name, voldef->capacity, - key) == -1) + key = phypBuildVolume(pool->conn, voldef->name, spdef->name, + voldef->capacity); + + if (key == NULL) goto err; if ((vol = @@ -2464,9 +2465,12 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, key)) == NULL) goto err; + VIR_FREE(key); + return vol; err: + VIR_FREE(key); virStorageVolDefFree(voldef); virStoragePoolDefFree(spdef); if (vol) @@ -2540,8 +2544,10 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) int exit_status = 0; char *cmd = NULL; char *spname = NULL; + char *char_ptr; char *key = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + virStorageVolPtr vol; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2566,20 +2572,21 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) if (exit_status < 0 || spname == NULL) return NULL; - char *char_ptr = strchr(spname, '\n'); + char_ptr = strchr(spname, '\n'); if (char_ptr) *char_ptr = '\0'; - if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) { - virReportOOMError(); - return NULL; - } + key = phypVolumeGetKey(conn, volname); - if (phypVolumeGetKey(conn, key, volname) == -1) + if (key == NULL) return NULL; - return virGetStorageVol(conn, spname, volname, key); + vol = virGetStorageVol(conn, spname, volname, key); + + VIR_FREE(key); + + return vol; } static int @@ -2647,6 +2654,8 @@ phypStoragePoolLookupByName(virConnectPtr conn, const char *name) static char * phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) { + char *xml; + virCheckFlags(0, NULL); virStorageVolDef voldef; @@ -2661,11 +2670,6 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) virStoragePoolDef pool; memset(&pool, 0, sizeof(virStoragePoolDef)); - if (VIR_ALLOC_N(voldef.key, MAX_KEY_SIZE) < 0) { - virReportOOMError(); - return NULL; - } - if (sp->name != NULL) { pool.name = sp->name; } else { @@ -2702,14 +2706,20 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) goto err; } - if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) { - VIR_ERROR0(_("Unable to determine volume's key.")); + voldef.key = strdup(vol->key); + + if (voldef.key == NULL) { + virReportOOMError(); goto err; } voldef.type = VIR_STORAGE_POOL_LOGICAL; - return virStorageVolDefFormat(&pool, &voldef); + xml = virStorageVolDefFormat(&pool, &voldef); + + VIR_FREE(voldef.key); + + return xml; err: return NULL; diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h index bc8e003..a22156c 100644 --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h @@ -30,7 +30,6 @@ # include <config.h> # include <libssh2.h> -# define MAX_KEY_SIZE (1024*4) # define LPAR_EXEC_ERR -1 # define SSH_CONN_ERR -2 /* error while trying to connect to remote host */ # define SSH_CMD_ERR -3 /* error while trying to execute the remote cmd */