It may happen, that this function would dereference a NULL pointer: util/virnetdevbridge.c: In function 'virNetDevBridgeGetVlanFiltering': util/virnetdevbridge.c:199:23: error: null pointer dereference [-Werror=null-dereference] ifr->ifr_data = (char*)&args; ~~~~~~~~~~~~~~^~~~~~~~~~~~~~ Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevbridge.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index ef1f4cc..f694863 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -193,7 +193,7 @@ static int virNetDevBridgeGet(const char *brname, goto cleanup; } VIR_FREE(valuestr); - } else { + } else if (ifr) { struct __bridge_info info; unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long)&info, 0, 0 }; ifr->ifr_data = (char*)&args; @@ -212,6 +212,10 @@ static int virNetDevBridgeGet(const char *brname, _("Unable to get bridge %s %s"), brname, paramname); goto cleanup; } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to get bridge %s %s"), brname, paramname); + goto cleanup; } ret = 0; -- 2.8.3

As unlikely as it might seem, func passed to this function can return NULL. And in some cases it indeed does so: virDomainDefGetVcpuSched and virDomainDefGetIOThreadSched. However, the function I'm fixing blindly dereference value func returned thus leading to SIGSEGV. conf/domain_conf.c: In function 'virDomainDefFormatInternal': conf/domain_conf.c:22155:22: error: potential null pointer dereference [-Werror=null-dereference] if (sched->policy == i) ~~~~~^~~~~~~~ conf/domain_conf.c:22184:26: error: potential null pointer dereference [-Werror=null-dereference] priority = sched->priority; ~~~~~~~~~^~~~~~~~~~~~~~~~~ conf/domain_conf.c:22190:30: error: potential null pointer dereference [-Werror=null-dereference] if (sched->priority == priority) ~~~~~^~~~~~~~~~ conf/domain_conf.c:22155:22: error: potential null pointer dereference [-Werror=null-dereference] if (sched->policy == i) ~~~~~^~~~~~~~ conf/domain_conf.c:22184:26: error: potential null pointer dereference [-Werror=null-dereference] priority = sched->priority; ~~~~~~~~~^~~~~~~~~~~~~~~~~ conf/domain_conf.c:22190:30: error: potential null pointer dereference [-Werror=null-dereference] if (sched->priority == priority) ~~~~~^~~~~~~~~~ Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 568c699..3daeb1e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22152,7 +22152,7 @@ virDomainFormatSchedDef(virDomainDefPtr def, while ((next = virBitmapNextSetBit(resourceMap, next)) > -1) { sched = func(def, next); - if (sched->policy == i) + if (sched && sched->policy == i) ignore_value(virBitmapSetBit(schedMap, next)); } @@ -22180,14 +22180,17 @@ virDomainFormatSchedDef(virDomainDefPtr def, /* we need to find a subset of vCPUs with the given scheduler * that share the priority */ nextprio = virBitmapNextSetBit(schedMap, -1); - sched = func(def, nextprio); + + if (!(sched = func(def, nextprio))) + goto cleanup; + priority = sched->priority; ignore_value(virBitmapSetBit(prioMap, nextprio)); while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { sched = func(def, nextprio); - if (sched->priority == priority) + if (sched && sched->priority == priority) ignore_value(virBitmapSetBit(prioMap, nextprio)); } -- 2.8.3

On Tue, May 31, 2016 at 12:33:28 +0200, Michal Privoznik wrote: Drop this comment. This is dead code since everything before checked that model was set. If you want to add this check here, then remove all the checks above and make it just jump to cleanup without reporting error. Since it's impossible to happen.

There's a bug in the function. We expect the following format for the data we are parsing here: key: value So we use strchr() to find ':' and then see if it is followed by space. Well, the condition which checks that - we got it wrong. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_monitor_text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 28a6e1b..9295219 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -892,7 +892,7 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, /* extract device name and make sure that it's followed by * a colon and space */ dev_name = line; - if (!(line = strchr(line, ':')) && line[1] != ' ') { + if (!(line = strchr(line, ':')) || line[1] != ' ') { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("info blockstats reply was malformed")); goto cleanup; -- 2.8.3

So the idea is as follows: firstly we obtain a list of all the luns, then iterate over it trying to find the one we want to work with and after all the iterations we detect whether we have found something. Now, the last check is broken, because it compares a value form previous iteration, not the one we've just been through. Then, when computing md5 sum of lun's UUID, we use wrong variable again. Well, @hostScsiDisk which is type of esxVI_HostScsiDisk extends esxVI_ScsiLun type so they both have the uuid member, but it just doesn't feel right to access the data via two different variables in one function call. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/esx/esx_storage_backend_iscsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_storage_backend_iscsi.c b/src/esx/esx_storage_backend_iscsi.c index a9a19cf..96b472e 100644 --- a/src/esx/esx_storage_backend_iscsi.c +++ b/src/esx/esx_storage_backend_iscsi.c @@ -687,7 +687,7 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume, } } - if (!hostScsiDisk) { + if (!scsiLun) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could find volume with name: %s"), volume->name); goto cleanup; @@ -697,7 +697,7 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume, def.name = volume->name; - md5_buffer(scsiLun->uuid, strlen(hostScsiDisk->uuid), md5); + md5_buffer(scsiLun->uuid, strlen(scsiLun->uuid), md5); virUUIDFormat(md5, uuid_string); -- 2.8.3

Gcc fails to see that virDomainChrGetDomainPtrsInternal always sets pointers to a non NULL value (except for chr device type VIR_DOMAIN_CHR_DEVICE_TYPE_LAST which if occurs, we are in way bigger problem than NULL deref). In file included from conf/domain_conf.c:37:0: conf/domain_conf.c: In function 'virDomainChrPreAlloc': conf/domain_conf.c:14109:35: error: potential null pointer dereference [-Werror=null-dereference] return VIR_REALLOC_N(*arrPtr, *cntPtr + 1); ^~ ./util/viralloc.h:158:73: note: in definition of macro 'VIR_REALLOC_N' # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count), \ ^~~~~ conf/domain_conf.c: In function 'virDomainChrRemove': conf/domain_conf.c:14133:21: error: potential null pointer dereference [-Werror=null-dereference] for (i = 0; i < *cntPtr; i++) { ^~~~~~~ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3daeb1e..67ed4d7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14106,6 +14106,12 @@ virDomainChrPreAlloc(virDomainDefPtr vmdef, virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (!arrPtr || !cntPtr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Impossible happened")); + return -1; + } + return VIR_REALLOC_N(*arrPtr, *cntPtr + 1); } @@ -14118,6 +14124,12 @@ virDomainChrInsertPreAlloced(virDomainDefPtr vmdef, virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (!arrPtr || !cntPtr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Impossible happened")); + return; + } + VIR_APPEND_ELEMENT_INPLACE(*arrPtr, *cntPtr, chr); } @@ -14130,6 +14142,12 @@ virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (!arrPtr || !cntPtr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Impossible happened")); + return NULL; + } + for (i = 0; i < *cntPtr; i++) { ret = (*arrPtr)[i]; -- 2.8.3

On Tue, May 31, 2016 at 12:55:06 +0200, Peter Krempa wrote: virDomainChrGetDomainPtrsInternal can be made to return failure if the type is invalid rather than open-coding this multiple times. Additionally the error message is weird.