9:40 a.m.
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
9:40 a.m.
New subject: [libvirt] [PATCH 1/4] parallels: Resolve issues with uninitialized 'ret' value
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
9:54 a.m.
New subject: [libvirt] [PATCH 1/4] parallels: Resolve issues with uninitialized 'ret' value
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 :|
11:34 a.m.
New subject: [libvirt] [PATCH v2 1/4] parallels: Resolve issues with uninitialized 'ret' value
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
10:18 a.m.
New subject: [libvirt] [PATCH v2 1/4] parallels: Resolve issues with uninitialized 'ret' value
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.
9:40 a.m.
New subject: [libvirt] [PATCH 2/4] openvz: Need to initialize 'ret' for kb_per_pages error path
---
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
10:08 a.m.
New subject: [libvirt] [PATCH 2/4] openvz: Need to initialize 'ret' for kb_per_pages error path
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
10:13 a.m.
New subject: [libvirt] [PATCH 2/4] openvz: Need to initialize 'ret' for kb_per_pages error path
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:41 a.m.
New subject: [libvirt] [PATCH 3/4] lxc: Initialize dst due to potential cleanup usage before setting
---
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
10 a.m.
New subject: [libvirt] [PATCH 3/4] lxc: Initialize dst due to potential cleanup usage before setting
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
9:41 a.m.
New subject: [libvirt] [PATCH 4/4] interface: Need to initialize 'add_to_list'
---
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
9:53 a.m.
New subject: [libvirt] [PATCH 4/4] interface: Need to initialize 'add_to_list'
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
9:45 a.m.
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(-)
5:15 a.m.
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.
4330
days inactive
4337
days old
13 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Guannan Ren -
John Ferlan -
Ján Tomko