[libvirt] [PATCH 0/2] Couple of coverity fixes
It was brought to my attention that coverity spotted couple of defects. Here are the fixes. Michal Privoznik (2): xenconfig: Properly check retval of virDomainGraphicsListenSetAddress virDomainFormatSchedDef: Check @schedMap bounds src/conf/domain_conf.c | 21 +++++++++++---------- src/xenconfig/xen_sxpr.c | 4 ++-- 2 files changed, 13 insertions(+), 12 deletions(-) -- 2.4.10
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port; if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error; if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port; if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error; if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) -- 2.4.10
On 10/02/16 11:37, Michal Privoznik wrote:
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style.
s/does/do/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0)
ACK Erik
On 02/10/2016 05:37 AM, Michal Privoznik wrote:
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I have patches on list which are similar, but I've been requested to change. In particular, this change leads to a memory leak. as the vnc needs to be deleted. John
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0)
On 10/02/16 12:02, John Ferlan wrote:
On 02/10/2016 05:37 AM, Michal Privoznik wrote:
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I have patches on list which are similar, but I've been requested to change. In particular, this change leads to a memory leak. as the vnc needs to be deleted.
John
Hmm, I can't see any memory leak caused by this particular patch, could you please point it out for me? The only thing that could possibly leak by an error within virDomainGraphicsListenAddress, is graphics and that pointer is properly freed at the end. Erik
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/10/2016 06:37 AM, Erik Skultety wrote:
On 10/02/16 12:02, John Ferlan wrote:
On 02/10/2016 05:37 AM, Michal Privoznik wrote:
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I have patches on list which are similar, but I've been requested to change. In particular, this change leads to a memory leak. as the vnc needs to be deleted.
John
Hmm, I can't see any memory leak caused by this particular patch, could you please point it out for me? The only thing that could possibly leak by an error within virDomainGraphicsListenAddress, is graphics and that pointer is properly freed at the end.
Erik
Michal asked me via IRC - the code looked eerily similar to the qemu_command code I had posted patches on which also has the same non status check. Should have more coffee in my system before diving in ;-) John
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 252a48b..fdfec2b 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -868,7 +868,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0) @@ -987,7 +987,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, graphics->data.vnc.port = port;
if (listenAddr && - virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true)) + virDomainGraphicsListenSetAddress(graphics, 0, listenAddr, -1, true) < 0) goto error;
if (VIR_STRDUP(graphics->data.vnc.auth.passwd, vncPasswd) < 0)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10.02.2016 12:50, John Ferlan wrote:
On 02/10/2016 06:37 AM, Erik Skultety wrote:
On 10/02/16 12:02, John Ferlan wrote:
On 02/10/2016 05:37 AM, Michal Privoznik wrote:
The function, like others in our code, returns zero on success and a negative value on error. However, there are two places in xenconfig source code where we check for non-zero value. While the function can't currently return a positive value, those checks look okay, but does not really follow our style.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/xenconfig/xen_sxpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I have patches on list which are similar, but I've been requested to change. In particular, this change leads to a memory leak. as the vnc needs to be deleted.
John
Hmm, I can't see any memory leak caused by this particular patch, could you please point it out for me? The only thing that could possibly leak by an error within virDomainGraphicsListenAddress, is graphics and that pointer is properly freed at the end.
Erik
Michal asked me via IRC - the code looked eerily similar to the qemu_command code I had posted patches on which also has the same non status check. Should have more coffee in my system before diving in ;-)
John
Okay, since we have a clear consensus here, I've pushed this one now. Michal
In the function, @schedMap is a bitmap used to track pinning of scheduling units (e.g. vcpu, iothreads). However, as a corner case it may happen that it's empty but the scheduler is set to either RR or FIFO. If that's the case we may get -1 when asking for the next bit set in @schedMap and proceed with that. The code is able to deal with that because we are ignoring some errors afterwards, but it's safer to check upfront. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 67415fa..a842ee4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21485,20 +21485,21 @@ virDomainFormatSchedDef(virDomainDefPtr def, case VIR_PROC_POLICY_FIFO: case VIR_PROC_POLICY_RR: virBitmapClearAll(prioMap); - hasPriority = true; /* we need to find a subset of vCPUs with the given scheduler * that share the priority */ - nextprio = virBitmapNextSetBit(schedMap, -1); - sched = func(def, nextprio); - priority = sched->priority; - - ignore_value(virBitmapSetBit(prioMap, nextprio)); - - while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + if ((nextprio = virBitmapNextSetBit(schedMap, -1)) > -1) { sched = func(def, nextprio); - if (sched->priority == priority) - ignore_value(virBitmapSetBit(prioMap, nextprio)); + priority = sched->priority; + hasPriority = true; + + ignore_value(virBitmapSetBit(prioMap, nextprio)); + + while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + sched = func(def, nextprio); + if (sched->priority == priority) + ignore_value(virBitmapSetBit(prioMap, nextprio)); + } } currentMap = prioMap; -- 2.4.10
On Wed, Feb 10, 2016 at 11:37:38 +0100, Michal Privoznik wrote:
In the function, @schedMap is a bitmap used to track pinning of scheduling units (e.g. vcpu, iothreads). However, as a corner case it may happen that it's empty but the scheduler is set to either RR or FIFO. If that's the case we may get -1 when asking for the next bit set in @schedMap and proceed with that. The code
This is not true. The parent loop condition checks that the bitmap is not empty thus -1 will never be returned.
is able to deal with that because we are ignoring some errors afterwards, but it's safer to check upfront.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
NACK, that is a false positive and the code you are adding is dead code. Peter
participants (4)
-
Erik Skultety -
John Ferlan -
Michal Privoznik -
Peter Krempa