[libvirt] [PATCH 0/6] Final round of build fixes
Hopefully. Michal Privoznik (6): virNetDevBridgeGet: Avoid NULL dereference virDomainFormatSchedDef: Handle case when func returns NULL esxStorageVolGetXMLDesc: Lookup SCSI lun properly qemuMonitorTextGetAllBlockStatsInfo: Fix line validation ppc64Compute: Avoid false positive domain_conf: Avoid false positive src/conf/domain_conf.c | 27 ++++++++++++++++++++++++--- src/cpu/cpu_ppc64.c | 12 +++++++++++- src/esx/esx_storage_backend_iscsi.c | 4 ++-- src/qemu/qemu_monitor_text.c | 2 +- src/util/virnetdevbridge.c | 6 +++++- 5 files changed, 43 insertions(+), 8 deletions(-) -- 2.8.3
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@redhat.com> --- 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
On Tue, May 31, 2016 at 12:33:25 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/util/virnetdevbridge.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
How about fixing the single caller that passes NULL in @ifr? virNetDevBridgeGetVlanFiltering
On 31.05.2016 13:06, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:25 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/util/virnetdevbridge.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
How about fixing the single caller that passes NULL in @ifr?
virNetDevBridgeGetVlanFiltering
What about it? AFAIK, SIOCDEVPRIVATE ioctl is not able to tell us whether vlan filtering is enabled. Therefore I don't see much sense in providing a fd and ifreq struct just so that we can go through else branch too. Michal
On Tue, May 31, 2016 at 13:17:30 +0200, Michal Privoznik wrote:
On 31.05.2016 13:06, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:25 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/util/virnetdevbridge.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
How about fixing the single caller that passes NULL in @ifr?
virNetDevBridgeGetVlanFiltering
What about it? AFAIK, SIOCDEVPRIVATE ioctl is not able to tell us whether vlan filtering is enabled. Therefore I don't see much sense in providing a fd and ifreq struct just so that we can go through else branch too.
Then the check should be based on @fd being -1 where apparently the second case doesn't make sense at all whether @ifr is set or not. I think it would make sense to extract the part that operates on @path and call it separately since you don't need the fallback path at all rather than using the compound function.
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@redhat.com> --- 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:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
On 31.05.2016 12:52, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
Well, having covery reports a false positive is one thing, being unable to compile is another one. This is what I was afraid of and expressed it here: https://www.redhat.com/archives/libvir-list/2016-May/msg02063.html On the other hand, you use more recent compiler than I do, so you'll hit these error sooner O:-) IOW, one day we will have to deal with this. Again. Michal
On Tue, May 31, 2016 at 13:05:16 +0200, Michal Privoznik wrote:
On 31.05.2016 12:52, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
Well, having covery reports a false positive is one thing, being unable to compile is another one. This is what I was afraid of and expressed it here:
https://www.redhat.com/archives/libvir-list/2016-May/msg02063.html
On the other hand, you use more recent compiler than I do, so you'll hit these error sooner O:-) IOW, one day we will have to deal with this. Again.
I'm thinking of disabling that warning at all since it's making us work it around in cases where it clearly doesn't make sense. It's very similar to the warning of comparison between signed and unsigned.
On 31.05.2016 13:10, Peter Krempa wrote:
On Tue, May 31, 2016 at 13:05:16 +0200, Michal Privoznik wrote:
On 31.05.2016 12:52, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
Well, having covery reports a false positive is one thing, being unable to compile is another one. This is what I was afraid of and expressed it here:
https://www.redhat.com/archives/libvir-list/2016-May/msg02063.html
On the other hand, you use more recent compiler than I do, so you'll hit these error sooner O:-) IOW, one day we will have to deal with this. Again.
I'm thinking of disabling that warning at all since it's making us work it around in cases where it clearly doesn't make sense. It's very similar to the warning of comparison between signed and unsigned.
Is it? What about the following code: char *c = NULL; *c = 'a'; Should you get a warning here or not? I would like to see a warning. But I agree that it is a question of personal preference. Michal
On Tue, May 31, 2016 at 13:32:50 +0200, Michal Privoznik wrote:
On 31.05.2016 13:10, Peter Krempa wrote:
On Tue, May 31, 2016 at 13:05:16 +0200, Michal Privoznik wrote:
On 31.05.2016 12:52, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
Well, having covery reports a false positive is one thing, being unable to compile is another one. This is what I was afraid of and expressed it here:
https://www.redhat.com/archives/libvir-list/2016-May/msg02063.html
On the other hand, you use more recent compiler than I do, so you'll hit these error sooner O:-) IOW, one day we will have to deal with this. Again.
I'm thinking of disabling that warning at all since it's making us work it around in cases where it clearly doesn't make sense. It's very similar to the warning of comparison between signed and unsigned.
Is it? What about the following code:
Yes.
char *c = NULL;
*c = 'a';
How about: int a = -1; size_t i; for (i = 0; i < a; i++) do_stuff(i);
Should you get a warning here or not? I would like to see a warning. But
Same question. Similar source of bugs. And we've disabled it! It's a prefernce until it creates more trouble than gain. Hence we've disabled the -Wsign-compare flag.
On 31.05.2016 13:48, Peter Krempa wrote:
On Tue, May 31, 2016 at 13:32:50 +0200, Michal Privoznik wrote:
On 31.05.2016 13:10, Peter Krempa wrote:
On Tue, May 31, 2016 at 13:05:16 +0200, Michal Privoznik wrote:
On 31.05.2016 12:52, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:26 +0200, Michal Privoznik wrote:
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.
Same as in the coverity case complaining about this it won't happen here since we are iterating a known list of items that was collected beforehand and thus no crashing will happen.
Well, having covery reports a false positive is one thing, being unable to compile is another one. This is what I was afraid of and expressed it here:
https://www.redhat.com/archives/libvir-list/2016-May/msg02063.html
On the other hand, you use more recent compiler than I do, so you'll hit these error sooner O:-) IOW, one day we will have to deal with this. Again.
I'm thinking of disabling that warning at all since it's making us work it around in cases where it clearly doesn't make sense. It's very similar to the warning of comparison between signed and unsigned.
Is it? What about the following code:
Yes.
Not quite. Is is only because of this warning being enabled that I've found two bugs fixed in this series.
char *c = NULL;
*c = 'a';
How about:
int a = -1; size_t i;
for (i = 0; i < a; i++) do_stuff(i);
There is no bug in here. In my example above, there's a real bug. Therefore I find these two distinct. Michal
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@redhat.com> --- 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
On Tue, May 31, 2016 at 12:33:27 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/esx/esx_storage_backend_iscsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
There is no way for a guest_model to be NULL. But gcc fails to see that. cpu/cpu_ppc64.c: In function 'ppc64Compute': cpu/cpu_ppc64.c:620:27: error: potential null pointer dereference [-Werror=null-dereference] if (STRNEQ(guest_model->name, host_model->name)) { ~~~~~~~~~~~^~~ cpu/cpu_ppc64.c:620:9: note: in expansion of macro 'STRNEQ' if (STRNEQ(guest_model->name, host_model->name)) { ^~~~~~ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_ppc64.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 799fb8e..cca051b 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -586,7 +586,7 @@ ppc64Compute(virCPUDefPtr host, if (cpu->type == VIR_CPU_TYPE_GUEST) { /* Guest CPU information */ virCPUCompareResult tmp; - switch (cpu->mode) { + switch ((virCPUMode) cpu->mode) { case VIR_CPU_MODE_HOST_MODEL: /* host-model only: * we need to take compatibility modes into account */ @@ -610,6 +610,10 @@ ppc64Compute(virCPUDefPtr host, if (!(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; break; + + case VIR_CPU_MODE_LAST: + /* nada */ + break; } } else { /* Other host CPU information */ @@ -617,6 +621,12 @@ ppc64Compute(virCPUDefPtr host, goto cleanup; } + if (!guest_model) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest_model not set")); + goto cleanup; + } + if (STRNEQ(guest_model->name, host_model->name)) { VIR_DEBUG("host CPU model does not match required CPU model %s", guest_model->name); -- 2.8.3
On Tue, May 31, 2016 at 12:33:28 +0200, Michal Privoznik wrote:
There is no way for a guest_model to be NULL. But gcc fails to see that.
cpu/cpu_ppc64.c: In function 'ppc64Compute': cpu/cpu_ppc64.c:620:27: error: potential null pointer dereference [-Werror=null-dereference] if (STRNEQ(guest_model->name, host_model->name)) { ~~~~~~~~~~~^~~ cpu/cpu_ppc64.c:620:9: note: in expansion of macro 'STRNEQ' if (STRNEQ(guest_model->name, host_model->name)) { ^~~~~~ cc1: all warnings being treated as errors
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_ppc64.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 799fb8e..cca051b 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -586,7 +586,7 @@ ppc64Compute(virCPUDefPtr host, if (cpu->type == VIR_CPU_TYPE_GUEST) { /* Guest CPU information */ virCPUCompareResult tmp; - switch (cpu->mode) { + switch ((virCPUMode) cpu->mode) { case VIR_CPU_MODE_HOST_MODEL: /* host-model only: * we need to take compatibility modes into account */ @@ -610,6 +610,10 @@ ppc64Compute(virCPUDefPtr host, if (!(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; break; + + case VIR_CPU_MODE_LAST: + /* nada */
Drop this comment.
+ break; } } else { /* Other host CPU information */ @@ -617,6 +621,12 @@ ppc64Compute(virCPUDefPtr host, goto cleanup; }
+ if (!guest_model) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest_model not set")); + goto cleanup; + }
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.
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@redhat.com> --- 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
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@redhat.com> --- 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
On Tue, May 31, 2016 at 12:33:30 +0200, Michal Privoznik wrote:
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.
The last sentence doesn't make much sense.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
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@redhat.com> --- 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
There is no way for a guest_model to be NULL. But gcc fails to see that. cpu/cpu_ppc64.c: In function 'ppc64Compute': cpu/cpu_ppc64.c:620:27: error: potential null pointer dereference [-Werror=null-dereference] if (STRNEQ(guest_model->name, host_model->name)) { ~~~~~~~~~~~^~~ cpu/cpu_ppc64.c:620:9: note: in expansion of macro 'STRNEQ' if (STRNEQ(guest_model->name, host_model->name)) { ^~~~~~ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_ppc64.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 799fb8e..cca051b 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -586,7 +586,7 @@ ppc64Compute(virCPUDefPtr host, if (cpu->type == VIR_CPU_TYPE_GUEST) { /* Guest CPU information */ virCPUCompareResult tmp; - switch (cpu->mode) { + switch ((virCPUMode) cpu->mode) { case VIR_CPU_MODE_HOST_MODEL: /* host-model only: * we need to take compatibility modes into account */ @@ -610,6 +610,10 @@ ppc64Compute(virCPUDefPtr host, if (!(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; break; + + case VIR_CPU_MODE_LAST: + /* nada */ + break; } } else { /* Other host CPU information */ @@ -617,6 +621,12 @@ ppc64Compute(virCPUDefPtr host, goto cleanup; } + if (!guest_model) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest_model not set")); + goto cleanup; + } + if (STRNEQ(guest_model->name, host_model->name)) { VIR_DEBUG("host CPU model does not match required CPU model %s", guest_model->name); -- 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@redhat.com> --- 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:33:33 +0200, Michal Privoznik wrote:
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@redhat.com> --- 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); }
NACK
On Tue, May 31, 2016 at 12:55:06 +0200, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:33:33 +0200, Michal Privoznik wrote:
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@redhat.com> --- 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); }
NACK
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.
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@redhat.com> --- 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
participants (2)
-
Michal Privoznik -
Peter Krempa