[libvirt] [PATCH 0/4] Resolve UNINIT errors found by Coverity
This set of patches resolves "Error: UNINIT (CWE-457)" errors found by Coverity John Ferlan (4): parallels: Resolve issues with uninitialized 'ret' value openvz: Need to initialize 'ret' for kb_per_pages error path lxc: Initialize dst due to potential cleanup usage before setting interface: Need to initialize 'add_to_list' src/interface/interface_backend_udev.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 19 ++++++++++++++----- 4 files changed, 17 insertions(+), 8 deletions(-) -- 1.7.11.7
Added some messaging to indicate possible failure from virXPathULongLong() as well --- src/parallels/parallels_storage.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 2908bee..10206bf 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -258,7 +258,7 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, virStorageVolDefPtr def) { xmlXPathContextPtr ctxt = NULL; - int ret; + int ret = -1; if (STRNEQ((const char *)root->name, "Parallels_disk_image")) { virReportError(VIR_ERR_XML_ERROR, @@ -274,12 +274,17 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, ctxt->node = root; - if (virXPathULongLong("string(./Disk_Parameters/Disk_size)", - ctxt, &def->capacity)) - ret = -1; + if ((ret = virXPathULongLong("string(./Disk_Parameters/Disk_size)", + ctxt, &def->capacity)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("failed to get disk size from " + "the disk descriptor xml")); + goto cleanup; + } def->capacity <<= 9; def->allocation = def->capacity; + ret = 0; cleanup: xmlXPathFreeContext(ctxt); return ret; @@ -315,7 +320,8 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool, def->type = VIR_STORAGE_VOL_FILE; - parallelsDiskDescParse(diskDescPath, def); + if (parallelsDiskDescParse(diskDescPath, def) < 0) + goto no_parse; if (!(def->target.path = realpath(diskPath, NULL))) goto no_memory; @@ -333,6 +339,9 @@ no_memory: virStorageVolDefFree(def); virReportOOMError(); return -1; +no_parse: + virStorageVolDefFree(def); + return -1; } static int parallelsFindVmVolumes(virStoragePoolObjPtr pool, -- 1.7.11.7
On Tue, Jan 08, 2013 at 10:40:58AM -0500, John Ferlan wrote:
Added some messaging to indicate possible failure from virXPathULongLong() as well --- src/parallels/parallels_storage.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 2908bee..10206bf 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -258,7 +258,7 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, virStorageVolDefPtr def) { xmlXPathContextPtr ctxt = NULL; - int ret; + int ret = -1;
if (STRNEQ((const char *)root->name, "Parallels_disk_image")) { virReportError(VIR_ERR_XML_ERROR, @@ -274,12 +274,17 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml,
ctxt->node = root;
- if (virXPathULongLong("string(./Disk_Parameters/Disk_size)", - ctxt, &def->capacity)) - ret = -1; + if ((ret = virXPathULongLong("string(./Disk_Parameters/Disk_size)", + ctxt, &def->capacity)) < 0) {
No need to assign to 'ret' here. It is initialized to -1 right at the top, and set to 0 on success later. So this intermediate assingment is a latent bug waiting to happen.
+ virReportError(VIR_ERR_XML_ERROR, + "%s", _("failed to get disk size from " + "the disk descriptor xml")); + goto cleanup; + }
def->capacity <<= 9; def->allocation = def->capacity; + ret = 0; cleanup: xmlXPathFreeContext(ctxt); return ret; @@ -315,7 +320,8 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool,
def->type = VIR_STORAGE_VOL_FILE;
- parallelsDiskDescParse(diskDescPath, def); + if (parallelsDiskDescParse(diskDescPath, def) < 0) + goto no_parse;
if (!(def->target.path = realpath(diskPath, NULL))) goto no_memory; @@ -333,6 +339,9 @@ no_memory: virStorageVolDefFree(def); virReportOOMError(); return -1; +no_parse: }
Generally we aim to only have 3 label names 'error' or 'cleanup' or 'no_memory'. Inthis case you want 'error'. I'd refactor it to allow fallthrough from 'no_memory' to 'error' thus: no_memory: virReportOOMError(); error: virStorageVolDefFree(def); return -1; which is our normal coding pattern for this scenario. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Added some messaging to indicate possible failure from virXPathULongLong() as well --- src/parallels/parallels_storage.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 2908bee..34911a4 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -258,7 +258,7 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, virStorageVolDefPtr def) { xmlXPathContextPtr ctxt = NULL; - int ret; + int ret = -1; if (STRNEQ((const char *)root->name, "Parallels_disk_image")) { virReportError(VIR_ERR_XML_ERROR, @@ -275,11 +275,16 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, ctxt->node = root; if (virXPathULongLong("string(./Disk_Parameters/Disk_size)", - ctxt, &def->capacity)) - ret = -1; + ctxt, &def->capacity) < 0) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("failed to get disk size from " + "the disk descriptor xml")); + goto cleanup; + } def->capacity <<= 9; def->allocation = def->capacity; + ret = 0; cleanup: xmlXPathFreeContext(ctxt); return ret; @@ -315,7 +320,8 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool, def->type = VIR_STORAGE_VOL_FILE; - parallelsDiskDescParse(diskDescPath, def); + if (parallelsDiskDescParse(diskDescPath, def) < 0) + goto error; if (!(def->target.path = realpath(diskPath, NULL))) goto no_memory; @@ -330,8 +336,9 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool, return 0; no_memory: - virStorageVolDefFree(def); virReportOOMError(); +error: + virStorageVolDefFree(def); return -1; } -- 1.7.11.7
On 01/09/2013 01:34 AM, John Ferlan wrote:
Added some messaging to indicate possible failure from virXPathULongLong() as well --- src/parallels/parallels_storage.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 2908bee..34911a4 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -258,7 +258,7 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, virStorageVolDefPtr def) { xmlXPathContextPtr ctxt = NULL; - int ret; + int ret = -1;
if (STRNEQ((const char *)root->name, "Parallels_disk_image")) { virReportError(VIR_ERR_XML_ERROR, @@ -275,11 +275,16 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, ctxt->node = root;
if (virXPathULongLong("string(./Disk_Parameters/Disk_size)", - ctxt, &def->capacity)) - ret = -1; + ctxt, &def->capacity) < 0) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("failed to get disk size from " + "the disk descriptor xml")); + goto cleanup; + }
def->capacity <<= 9; def->allocation = def->capacity; + ret = 0; cleanup: xmlXPathFreeContext(ctxt); return ret; @@ -315,7 +320,8 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool,
def->type = VIR_STORAGE_VOL_FILE;
- parallelsDiskDescParse(diskDescPath, def); + if (parallelsDiskDescParse(diskDescPath, def) < 0) + goto error;
if (!(def->target.path = realpath(diskPath, NULL))) goto no_memory; @@ -330,8 +336,9 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool,
return 0; no_memory: - virStorageVolDefFree(def); virReportOOMError(); +error: + virStorageVolDefFree(def); return -1; }
ACK.
--- src/openvz/openvz_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 09518d5..1ad3feb 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -485,7 +485,7 @@ error: static int openvzReadMemConf(virDomainDefPtr def, int veid) { - int ret; + int ret = -1; char *temp = NULL; unsigned long long barrier, limit; const char *param; -- 1.7.11.7
On 01/08/2013 11:40 PM, John Ferlan wrote:
--- src/openvz/openvz_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 09518d5..1ad3feb 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -485,7 +485,7 @@ error: static int openvzReadMemConf(virDomainDefPtr def, int veid) { - int ret; + int ret = -1; char *temp = NULL; unsigned long long barrier, limit; const char *param;
Is this necessary to do this, ret will be initialized to a value any way before any condition clause. There is no harm to set it initially. ACK
On 01/14/2013 11:08 AM, Guannan Ren wrote:
On 01/08/2013 11:40 PM, John Ferlan wrote:
--- src/openvz/openvz_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 09518d5..1ad3feb 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -485,7 +485,7 @@ error: static int openvzReadMemConf(virDomainDefPtr def, int veid) { - int ret; + int ret = -1; char *temp = NULL; unsigned long long barrier, limit; const char *param;
Is this necessary to do this, ret will be initialized to a value any way before any condition clause.
From Coverity:
(1) Event var_decl: Declaring variable "ret" without initializer. Also see events: [uninit_use] 488 int ret; ... 494 kb_per_pages = openvzKBPerPages(); (2) Event cond_true: Condition "kb_per_pages < 0", taking true branch 495 if (kb_per_pages < 0) (3) Event goto: Jumping to label "error" 496 goto error; 497 ... (4) Event label: Reached label "error" 548 error: (5) Event cond_true: Condition "1", taking true branch 549 VIR_FREE(temp); (6) Event uninit_use: Using uninitialized value "ret". Also see events: [var_decl] 550 return ret;
There is no harm to set it initially. ACK
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
--- src/lxc/lxc_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8050ce6..8457ae9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3102,7 +3102,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, virDomainDiskDefPtr def = dev->data.disk; virCgroupPtr group = NULL; int ret = -1; - char *dst; + char *dst = NULL; struct stat sb; bool created = false; mode_t mode = 0; -- 1.7.11.7
On 01/08/2013 11:41 PM, John Ferlan wrote:
--- src/lxc/lxc_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8050ce6..8457ae9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3102,7 +3102,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, virDomainDiskDefPtr def = dev->data.disk; virCgroupPtr group = NULL; int ret = -1; - char *dst; + char *dst = NULL; struct stat sb; bool created = false; mode_t mode = 0;
ACK
--- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 3231b73..10b8a5d 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -352,7 +352,7 @@ udevIfaceListAllInterfaces(virConnectPtr conn, const char *path; const char *name; const char *macaddr; - int add_to_list; + int add_to_list = 0; path = udev_list_entry_get_name(dev_entry); dev = udev_device_new_from_syspath(udev, path); -- 1.7.11.7
On 01/08/2013 11:41 PM, John Ferlan wrote:
--- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 3231b73..10b8a5d 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -352,7 +352,7 @@ udevIfaceListAllInterfaces(virConnectPtr conn, const char *path; const char *name; const char *macaddr; - int add_to_list; + int add_to_list = 0;
path = udev_list_entry_get_name(dev_entry); dev = udev_device_new_from_syspath(udev, path);
ACK
ping On 01/08/2013 10:40 AM, John Ferlan wrote:
This set of patches resolves "Error: UNINIT (CWE-457)" errors found by Coverity
John Ferlan (4): parallels: Resolve issues with uninitialized 'ret' value openvz: Need to initialize 'ret' for kb_per_pages error path lxc: Initialize dst due to potential cleanup usage before setting interface: Need to initialize 'add_to_list'
src/interface/interface_backend_udev.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 19 ++++++++++++++----- 4 files changed, 17 insertions(+), 8 deletions(-)
On 01/08/13 16:40, John Ferlan wrote:
This set of patches resolves "Error: UNINIT (CWE-457)" errors found by Coverity
John Ferlan (4): parallels: Resolve issues with uninitialized 'ret' value openvz: Need to initialize 'ret' for kb_per_pages error path lxc: Initialize dst due to potential cleanup usage before setting interface: Need to initialize 'add_to_list'
src/interface/interface_backend_udev.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 19 ++++++++++++++----- 4 files changed, 17 insertions(+), 8 deletions(-)
Pushed now.
participants (4)
-
Daniel P. Berrange -
Guannan Ren -
John Ferlan -
Ján Tomko