[libvirt] [PATCH 1/4] virStorageVol: avoid PATH_MAX-sized array
POSIX allows implementations where PATH_MAX is undefined, leading to compilation error. Not to mention that even if it is defined, it is often wasteful in relation to the amount of data being stored. All clients of vol->key were audited, and found not to care about whether key is static or dynamic, except for these offenders: * src/datatypes.h (struct _virStorageVol): Manage key dynamically. * src/datatypes.c (virReleaseStorageVol): Free key. (virGetStorageVol): Copy key. --- I'm also adding a 'make syntax-check' rule in gnulib to catch this from being a problem in the future: http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/27259 src/datatypes.c | 8 +++++--- src/datatypes.h | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 4c4cbd0..f0b5025 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -738,10 +738,10 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, virReportOOMError(); goto error; } - if (virStrcpyStatic(ret->key, key) == NULL) { + ret->key = strdup(key); + if (ret->key == NULL) { virMutexUnlock(&conn->lock); - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("Volume key %s too large for destination"), key); + virReportOOMError(); goto error; } ret->magic = VIR_STORAGE_VOL_MAGIC; @@ -754,6 +754,7 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, error: if (ret != NULL) { + VIR_FREE(ret->key); VIR_FREE(ret->name); VIR_FREE(ret->pool); VIR_FREE(ret); @@ -780,6 +781,7 @@ virReleaseStorageVol(virStorageVolPtr vol) { VIR_DEBUG("release vol %p %s", vol, vol->name); vol->magic = -1; + VIR_FREE(vol->key); VIR_FREE(vol->name); VIR_FREE(vol->pool); VIR_FREE(vol); diff --git a/src/datatypes.h b/src/datatypes.h index ebda992..f114360 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -1,7 +1,7 @@ /* * datatypes.h: management of structs for public data types * - * Copyright (C) 2006-2008, 2010 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -254,8 +254,7 @@ struct _virStorageVol { virConnectPtr conn; /* pointer back to the connection */ char *pool; /* Pool name of owner */ char *name; /* the storage vol external name */ - /* XXX currently abusing path for this. Ought not to be so evil */ - char key[PATH_MAX]; /* unique key for storage vol */ + char *key; /* unique key for storage vol */ }; /** -- 1.7.4.4
See previous patch for why this is good... * src/test/test_driver.c (struct _testConn, testOpenFromFile) (testClose): Manage path dynamically. --- src/test/test_driver.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6c8b9cf..18f6a52 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -77,7 +77,7 @@ typedef struct _testCell *testCellPtr; struct _testConn { virMutex lock; - char path[PATH_MAX]; + char *path; int nextDomID; virCapsPtr caps; virNodeInfo nodeInfo; @@ -790,9 +790,8 @@ static int testOpenFromFile(virConnectPtr conn, privconn->nextDomID = 1; privconn->numCells = 0; - if (virStrcpyStatic(privconn->path, file) == NULL) { - testError(VIR_ERR_INTERNAL_ERROR, - _("Path %s too big for destination"), file); + if ((privconn->path = strdup(file)) == NULL) { + virReportOOMError(); goto error; } memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); @@ -1090,6 +1089,7 @@ static int testOpenFromFile(virConnectPtr conn, virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); + VIR_FREE(privconn->path); testDriverUnlock(privconn); VIR_FREE(privconn); conn->privateData = NULL; @@ -1161,6 +1161,7 @@ static int testClose(virConnectPtr conn) virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virDomainEventStateFree(privconn->domainEventState); + VIR_FREE(privconn->path); testDriverUnlock(privconn); virMutexDestroy(&privconn->lock); -- 1.7.4.4
2011/6/22 Eric Blake <eblake@redhat.com>:
See previous patch for why this is good...
* src/test/test_driver.c (struct _testConn, testOpenFromFile) (testClose): Manage path dynamically. --- src/test/test_driver.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
See previous patch for why this is good... * src/xen/xen_driver.h (xenXMConfCache): Manage filename dynamically. * src/xen/xm_internal.c (xenXMConfigCacheAddFile) (xenXMConfigFree, xenXMDomainDefineXML): Likewise. --- src/xen/xen_driver.h | 4 ++-- src/xen/xm_internal.c | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 20eca6e..6d45f7d 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -1,7 +1,7 @@ /* * xen_unified.c: Unified Xen driver. * - * Copyright (C) 2007, 2010 Red Hat, Inc. + * Copyright (C) 2007, 2010-2011 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -117,7 +117,7 @@ struct xenUnifiedDriver { typedef struct xenXMConfCache *xenXMConfCachePtr; typedef struct xenXMConfCache { time_t refreshedAt; - char filename[PATH_MAX]; + char *filename; virDomainDefPtr def; } xenXMConfCache; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 713bac3..530b0d4 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -143,6 +143,7 @@ static int xenInotifyActive(virConnectPtr conn) static void xenXMConfigFree(void *payload, const void *key ATTRIBUTE_UNUSED) { xenXMConfCachePtr entry = (xenXMConfCachePtr)payload; virDomainDefFree(entry->def); + VIR_FREE(entry->filename); VIR_FREE(entry); } @@ -281,7 +282,11 @@ xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) virReportOOMError(); return -1; } - memcpy(entry->filename, filename, PATH_MAX); + if ((entry->filename = strdup(filename)) == NULL) { + virReportOOMError(); + VIR_FREE(entry); + return -1; + } } entry->refreshedAt = now; @@ -289,6 +294,7 @@ xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) VIR_DEBUG("Failed to read %s", entry->filename); if (!newborn) virHashSteal(priv->configCache, filename); + VIR_FREE(entry->filename); VIR_FREE(entry); return -1; } @@ -298,6 +304,7 @@ xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) if (newborn) { if (virHashAddEntry(priv->configCache, entry->filename, entry) < 0) { virDomainDefFree(entry->def); + VIR_FREE(entry->filename); VIR_FREE(entry); xenXMError(VIR_ERR_INTERNAL_ERROR, "%s", _("xenXMConfigCacheRefresh: virHashAddEntry")); @@ -309,9 +316,11 @@ xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) * of the domain in question */ if (!virHashLookup(priv->nameConfigMap, entry->def->name)) { - if (virHashAddEntry(priv->nameConfigMap, entry->def->name, entry->filename) < 0) { + if (virHashAddEntry(priv->nameConfigMap, entry->def->name, + entry->filename) < 0) { virHashSteal(priv->configCache, filename); virDomainDefFree(entry->def); + VIR_FREE(entry->filename); VIR_FREE(entry); } } @@ -1171,7 +1180,10 @@ virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) goto error; } - memmove(entry->filename, filename, PATH_MAX); + if ((entry->filename = strdup(filename)) == NULL) { + virReportOOMError(); + goto error; + } entry->def = def; if (virHashAddEntry(priv->configCache, filename, entry) < 0) { @@ -1194,6 +1206,7 @@ virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) error: VIR_FREE(filename); + VIR_FREE(entry->filename); VIR_FREE(entry); virDomainDefFree(def); xenUnifiedUnlock(priv); -- 1.7.4.4
2011/6/22 Eric Blake <eblake@redhat.com>:
See previous patch for why this is good...
* src/xen/xen_driver.h (xenXMConfCache): Manage filename dynamically. * src/xen/xm_internal.c (xenXMConfigCacheAddFile) (xenXMConfigFree, xenXMDomainDefineXML): Likewise. --- src/xen/xen_driver.h | 4 ++-- src/xen/xm_internal.c | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
See previous patch for why this is good... * src/util/pci.c (struct _pciDevice, pciGetDevice, pciFreeDevice): Manage path dynamically. Report snprintf overflow. * src/util/hostusb.c (struct _usbDevice, usbGetDevice) (usbFreeDevice): Likewise. --- src/util/hostusb.c | 32 +++++++++++++++++++++++++------- src/util/pci.c | 31 +++++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/util/hostusb.c b/src/util/hostusb.c index d5b478b..1669e2f 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -48,7 +48,7 @@ struct _usbDevice { char name[USB_ADDR_LEN]; /* domain:bus:slot.function */ char id[USB_ID_LEN]; /* product vendor */ - char path[PATH_MAX]; + char *path; }; /* For virReportOOMError() and virReportSystemError() */ @@ -171,13 +171,30 @@ usbGetDevice(unsigned bus, dev->bus = bus; dev->dev = devno; - snprintf(dev->name, sizeof(dev->name), "%.3o:%.3o", - dev->bus, dev->dev); - snprintf(dev->path, sizeof(dev->path), - USB_DEVFS "%03d/%03d", dev->bus, dev->dev); + if (snprintf(dev->name, sizeof(dev->name), "%.3o:%.3o", + dev->bus, dev->dev) >= sizeof(dev->name)) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("dev->name buffer overflow: %.3o:%.3o"), + dev->bus, dev->dev); + usbFreeDevice(dev); + return NULL; + } + if (virAsprintf(&dev->path, USB_DEVFS "%03d/%03d", + dev->bus, dev->dev) < 0) { + virReportOOMError(); + usbFreeDevice(dev); + return NULL; + } /* XXX fixme. this should be product/vendor */ - snprintf(dev->id, sizeof(dev->id), "%d %d", dev->bus, dev->dev); + if (snprintf(dev->id, sizeof(dev->id), "%d %d", dev->bus, + dev->dev) >= sizeof(dev->id)) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("dev->id buffer overflow: %d %d"), + dev->bus, dev->dev); + usbFreeDevice(dev); + return NULL; + } VIR_DEBUG("%s %s: initialized", dev->id, dev->name); @@ -203,6 +220,7 @@ void usbFreeDevice(usbDevice *dev) { VIR_DEBUG("%s %s: freeing", dev->id, dev->name); + VIR_FREE(dev->path); VIR_FREE(dev); } diff --git a/src/util/pci.c b/src/util/pci.c index 8b2ca42..46a3a83 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -56,7 +56,7 @@ struct _pciDevice { char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ - char path[PATH_MAX]; + char *path; int fd; unsigned initted; @@ -1307,10 +1307,21 @@ pciGetDevice(unsigned domain, dev->slot = slot; dev->function = function; - snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", - dev->domain, dev->bus, dev->slot, dev->function); - snprintf(dev->path, sizeof(dev->path), - PCI_SYSFS "devices/%s/config", dev->name); + if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", + dev->domain, dev->bus, dev->slot, + dev->function) >= sizeof(dev->name)) { + pciReportError(VIR_ERR_INTERNAL_ERROR, + _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), + dev->domain, dev->bus, dev->slot, dev->function); + pciFreeDevice(dev); + return NULL; + } + if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", + dev->name) < 0) { + virReportOOMError(); + pciFreeDevice(dev); + return NULL; + } if (access(dev->path, F_OK) != 0) { virReportSystemError(errno, @@ -1334,7 +1345,14 @@ pciGetDevice(unsigned domain, } /* strings contain '0x' prefix */ - snprintf(dev->id, sizeof(dev->id), "%s %s", &vendor[2], &product[2]); + if (snprintf(dev->id, sizeof(dev->id), "%s %s", &vendor[2], + &product[2]) >= sizeof(dev->id)) { + pciReportError(VIR_ERR_INTERNAL_ERROR, + _("dev->id buffer overflow: %s %s"), + &vendor[2], &product[2]); + pciFreeDevice(dev); + return NULL; + } VIR_FREE(product); VIR_FREE(vendor); @@ -1351,6 +1369,7 @@ pciFreeDevice(pciDevice *dev) return; VIR_DEBUG("%s %s: freeing", dev->id, dev->name); pciCloseConfig(dev); + VIR_FREE(dev->path); VIR_FREE(dev); } -- 1.7.4.4
2011/6/22 Eric Blake <eblake@redhat.com>:
See previous patch for why this is good...
* src/util/pci.c (struct _pciDevice, pciGetDevice, pciFreeDevice): Manage path dynamically. Report snprintf overflow. * src/util/hostusb.c (struct _usbDevice, usbGetDevice) (usbFreeDevice): Likewise. --- src/util/hostusb.c | 32 +++++++++++++++++++++++++------- src/util/pci.c | 31 +++++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 13 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 06/22/2011 05:08 PM, Matthias Bolte wrote:
2011/6/22 Eric Blake <eblake@redhat.com>:
See previous patch for why this is good...
* src/util/pci.c (struct _pciDevice, pciGetDevice, pciFreeDevice): Manage path dynamically. Report snprintf overflow. * src/util/hostusb.c (struct _usbDevice, usbGetDevice) (usbFreeDevice): Likewise. --- src/util/hostusb.c | 32 +++++++++++++++++++++++++------- src/util/pci.c | 31 +++++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 13 deletions(-)
ACK.
Thanks; series pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/6/22 Eric Blake <eblake@redhat.com>:
POSIX allows implementations where PATH_MAX is undefined, leading to compilation error. Not to mention that even if it is defined, it is often wasteful in relation to the amount of data being stored.
All clients of vol->key were audited, and found not to care about whether key is static or dynamic, except for these offenders:
* src/datatypes.h (struct _virStorageVol): Manage key dynamically. * src/datatypes.c (virReleaseStorageVol): Free key. (virGetStorageVol): Copy key. ---
I'm also adding a 'make syntax-check' rule in gnulib to catch this from being a problem in the future: http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/27259
src/datatypes.c | 8 +++++--- src/datatypes.h | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-)
@@ -254,8 +254,7 @@ struct _virStorageVol { virConnectPtr conn; /* pointer back to the connection */ char *pool; /* Pool name of owner */ char *name; /* the storage vol external name */ - /* XXX currently abusing path for this. Ought not to be so evil */ - char key[PATH_MAX]; /* unique key for storage vol */ + char *key; /* unique key for storage vol */ };
I think that the comment was referring to the some storage backends using the path of a file as the key instead of some other form of key. So the comment may still be valid (but only for specific backend) and was not related to PATH_MAX here. As it's definitely wrong to have it in this general place I think it's okay to remove it. ACK. -- Matthias Bolte http://photron.blogspot.com
participants (2)
-
Eric Blake -
Matthias Bolte