Bug 739704

In POST:
http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-September/msg00572.html

Using Coverity to run on libvirt-0.9.4-12.el6.src.rpm, I haven't found other resource leak except for 'virVMXParseConfig', which has been fixed on upstream, 
as Eric suggested, we needn't build vmx in rhel, coverity report can be treated as a false positive, so move the bug to VERIFIED status.

Please see attachment for Coverity result based on -12.

Alex

Created attachment 524139 [details]
CoverityScan

(In reply to comment #4)
> Using Coverity to run on libvirt-0.9.4-12.el6.src.rpm, I haven't found other
> resource leak except for 'virVMXParseConfig', which has been fixed on upstream, 
> as Eric suggested, we needn't build vmx in rhel, coverity report can be treated
> as a false positive, so move the bug to VERIFIED status.
> 
> Please see attachment for Coverity result based on -12.

That scan still shows to non-false-positive bugs that need to be fixed.  I'm moving this back to ASSIGNED to track those:

16 	FORWARD_NULL 	/root/rpmbuild/BUILD/libvirt-0.9.4/src/qemu/qemu_process.c 	qemuProcessReconnect 	UNINSPECTED

30 	REVERSE_INULL 	/root/rpmbuild/BUILD/libvirt-0.9.4/src/locking/lock_driver_sanlock.c 	virLockManagerSanlockSetupLockspace 	UNINSPECTED

(In reply to comment #6)
> (In reply to comment #4)
> > Using Coverity to run on libvirt-0.9.4-12.el6.src.rpm, I haven't found other
> > resource leak except for 'virVMXParseConfig', which has been fixed on upstream, 
> > as Eric suggested, we needn't build vmx in rhel, coverity report can be treated
> > as a false positive, so move the bug to VERIFIED status.
> > 
> > Please see attachment for Coverity result based on -12.
> 
> That scan still shows to non-false-positive bugs that need to be fixed.  I'm
> moving this back to ASSIGNED to track those:
> 
> 16  FORWARD_NULL  /root/rpmbuild/BUILD/libvirt-0.9.4/src/qemu/qemu_process.c 
> qemuProcessReconnect  UNINSPECTED

https://www.redhat.com/archives/libvir-list/2011-September/msg00858.html

> 
> 30  REVERSE_INULL 
> /root/rpmbuild/BUILD/libvirt-0.9.4/src/locking/lock_driver_sanlock.c 
> virLockManagerSanlockSetupLockspace  UNINSPECTED

https://www.redhat.com/archives/libvir-list/2011-September/msg00857.html


Alex

(In reply to comment #6) 
> That scan still shows to non-false-positive bugs that need to be fixed.  I'm
> moving this back to ASSIGNED to track those:
> 
> 16  FORWARD_NULL  /root/rpmbuild/BUILD/libvirt-0.9.4/src/qemu/qemu_process.c 
> qemuProcessReconnect  UNINSPECTED

https://www.redhat.com/archives/libvir-list/2011-September/msg00858.html  (ACKed and pushed)

> 
> 30  REVERSE_INULL 
> /root/rpmbuild/BUILD/libvirt-0.9.4/src/locking/lock_driver_sanlock.c 
> virLockManagerSanlockSetupLockspace  UNINSPECTED

https://www.redhat.com/archives/libvir-list/2011-September/msg00876.html (Resend a new patch based on Eric's suggestion)


Thanks,
Alex

(In reply to comment #8)
> > 
> > 16  FORWARD_NULL  /root/rpmbuild/BUILD/libvirt-0.9.4/src/qemu/qemu_process.c 
> > qemuProcessReconnect  UNINSPECTED
> 
> https://www.redhat.com/archives/libvir-list/2011-September/msg00858.html 
> (ACKed and pushed)

The following is the same as above:
https://www.redhat.com/archives/libvir-list/2011-September/msg00859.html (ACKed and pushed)

In POST:
http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-September/msg00762.html

Hi Eric,
The above issues have been resolved on libvirt-0.9.4-13. Another question, I think '17' item should be fixed from attachment web page, if 'vmdef' is NULL, but we do nothing for this, it will potentially result in other issue such as 'NULL pointer deference', IMHO, so it makes sense to fix this, and have committed a patch for upstream:

http://www.redhat.com/archives/libvir-list/2011-September/msg00979.html

Alex

Created attachment 524862 [details]
CoverityScan-13

(In reply to comment #11)
> Hi Eric,
> The above issues have been resolved on libvirt-0.9.4-13. Another question, I
> think '17' item should be fixed from attachment web page, if 'vmdef' is NULL,
> but we do nothing for this, it will potentially result in other issue such as
> 'NULL pointer deference', IMHO, so it makes sense to fix this, and have
> committed a patch for upstream:
> 
> http://www.redhat.com/archives/libvir-list/2011-September/msg00979.html

Concur that this thread is worth resolving (there has been some back and forth, so the patch is not quite final yet), so moving this bz back to ASSIGNED.

Back in POST for yet another fix.
http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-September/msg00918.html

Hi Eric,
The previous issues have been resolved based on latest coverity result in attachment.

However, it seems we still need to fix some issues such as 'NULL_RETURNS' and 'REVERSE_INULL', please confirm it.

Thanks,
Alex

Created attachment 525443 [details]
CoverityScan-14

Created attachment 526885 [details]
List of fixed libvirt defects

Agreed that the latest attachment still points out problems worth fixing.  Moving back to assigned for yet another round of upstream patching.

Created attachment 527058 [details]
CoverityScan-16

This is a latest coverity result on libvirt-0.9.4-16.el6.src.rpm.

Created attachment 527596 [details]
CoverityScan-17

This is a latest coverity result on libvirt-0.9.4-17.el6.src.rpm.

I've posted several relevant upstream patches, such as:
https://www.redhat.com/archives/libvir-list/2011-October/msg00533.html

I'm hoping to get these all approved and backported to RHEL by Friday.

My analysis of the -17 results:

Error: CHECKED_RETURN:
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_json.c:1437: check_return: Calling function "virJSONValueObjectGetBoolean" without checking return value (as is done elsewhere 4 out of 5 times).
False positive, but upstream will silence this.

Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.9.4/gnulib/lib/strerror_r.c:134: dead_error_condition: On this path, the condition "msg" cannot be true.
Truthful on Linux, but inconsequential.  The gnulib code has to cater to other platforms, which does indeed introduce dead code on Linux.  Not worth fixing.

Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.9.4/gnulib/lib/strerror.c:47: dead_error_condition: On this path, the condition "msg" cannot be true.
Same story.

Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.9.4/tools/virsh.c:12627: dead_error_condition: On this path, the condition "parent" cannot be true.
I didn't see this one in my upstream scan; that may be related to the fact that the code has further changed due to 'snapshot-list --tree' which was not backported; so fixing this is probably RHEL-only.  Or we can safely ignore it.

Error: MISSING_RETURN:
/tmp/tmpG4rrjF.c:1: missing_return: Arriving at the end of a function without returning a value.
Not libvirt's fault.  This is caused by dprobe emitting a function with an int return type but no body.  We can safely ignore it.


Error: NULL_RETURNS:
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_json.c:1377: returned_null: Function "qemuMonitorJSONMakeCommand" returns null (checked 57 out of 58 times).
Real, and part of my upstream patches.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.9.4/src/vmx/vmx.c:1221: alloc_fn: Calling allocation function "virConfReadMem".
Real and upstream, but we don't build VMX for RHEL, so not worth worrying about.

Error: RETURN_LOCAL:
/builddir/build/BUILD/libvirt-0.9.4/gnulib/lib/careadlinkat.c:101: local_ptr_assign_local: Assigning: "buffer" = "stack_buf" (address of local variable "stack_buf").
False positive.  Coverity is mis-analyzing gnulib code.

Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.9.4/src/esx/esx_vi.c:1667: deref_ptr: Directly dereferencing pointer "objectSpec".
Upstream.  We build ESX for RHEL, but this one is harmless in effect (basically a sign of dead code, and not an actual logic bug).

Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_text.c:738: deref_ptr: Directly dereferencing pointer "p".
Real and upstream, and in my patch set (several instances in one function).

(In reply to comment #22)
> I've posted several relevant upstream patches, such as:
> https://www.redhat.com/archives/libvir-list/2011-October/msg00533.html
> 
> I'm hoping to get these all approved and backported to RHEL by Friday.
> 
> My analysis of the -17 results:
> 
> Error: CHECKED_RETURN:
> /builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_json.c:1437:
> check_return: Calling function "virJSONValueObjectGetBoolean" without checking
> return value (as is done elsewhere 4 out of 5 times).
> False positive, but upstream will silence this.

Backport of commit 0654d27

> 
> Error: DEADCODE:
> /builddir/build/BUILD/libvirt-0.9.4/tools/virsh.c:12627: dead_error_condition:
> On this path, the condition "parent" cannot be true.
> I didn't see this one in my upstream scan; that may be related to the fact that
> the code has further changed due to 'snapshot-list --tree' which was not
> backported; so fixing this is probably RHEL-only.  Or we can safely ignore it.

Safely ignore.  Introduced by RHEL-specific conflict resolution when backporting commit d1be48f for bug 742410

> Error: NULL_RETURNS:
> /builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_json.c:1377:
> returned_null: Function "qemuMonitorJSONMakeCommand" returns null (checked 57
> out of 58 times).
> Real, and part of my upstream patches.

backport of commit ce521f2

> Error: REVERSE_INULL:
> /builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_monitor_text.c:738:
> deref_ptr: Directly dereferencing pointer "p".
> Real and upstream, and in my patch set (several instances in one function).

backport of commit 60be9e8

Plus I'm lumping in some other backports for bug fixes found by code analysis.

In POST again (maybe fourth time's the charm):
http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-October/msg00441.html

Created attachment 528730 [details]
CoverityScan-18

This is a latest coverity result on libvirt-0.9.4-18.el6.src.rpm

The previous issues have been resolve based on latest result and Comment 22 & Comment 23.

The following is a new patch issue:

Error: NULL_RETURNS:
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_hostdev.c:230: returned_null: Function "pciDeviceListFind" returns null (checked 8 out of 9 times).
/builddir/build/BUILD/libvirt-0.9.4/src/util/pci.c:1536: return_null: Explicitly returning NULL.
......
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_hostdev.c:230: var_assigned: Assigning: "activeDev" = null return value from "pciDeviceListFind".
/builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_hostdev.c:232: dereference: Dereferencing a pointer that might be null "activeDev" when calling "pciDeviceSetUsedBy".
/builddir/build/BUILD/libvirt-0.9.4/src/util/pci.c:1400: deref_parm: Directly dereferencing parameter "dev".

And I have discussed this with Eric, which think we may consolidate loop 4 and loop 5 into one to silence Coverity complains, but I think it's not a necessary choose, and use 6 loops is a clear design, Eric, maybe, may we ignore this?

BTW, There are also some issues for libvirt-0.8.7-18.el6_1.2 and libvirt-0.8.2-15.el5_6.5, whether I need to file 2 bugs for them again?

Thanks,
Alex

(In reply to comment #25)
> Created attachment 528730 [details]
> CoverityScan-18
> 
> The following is a new patch issue:
> 
> Error: NULL_RETURNS:
> /builddir/build/BUILD/libvirt-0.9.4/src/qemu/qemu_hostdev.c:230: returned_null:
> Function "pciDeviceListFind" returns null (checked 8 out of 9 times).
> /builddir/build/BUILD/libvirt-0.9.4/src/util/pci.c:1536: return_null:
> Explicitly returning NULL.

We can safely ignore this one.  The code is structured so that loop 4 assigns the ref into the list, and loop 5 then queries the ref from that list.  Loop 5 cannot be reached unless loop 4 succeeded, so this will never fail.  Given that this was the only new finding against -18, I am confident that we can mark this bug as resolved.

> BTW, There are also some issues for libvirt-0.8.7-18.el6_1.2 and
> libvirt-0.8.2-15.el5_6.5, whether I need to file 2 bugs for them again?

File a separate BZ for separate build streams.

(In reply to comment #26)
> We can safely ignore this one.  The code is structured so that loop 4 assigns
> the ref into the list, and loop 5 then queries the ref from that list.  Loop 5
> cannot be reached unless loop 4 succeeded, so this will never fail.  Given that
> this was the only new finding against -18, I am confident that we can mark this
> bug as resolved.
Agree.
> 
> > BTW, There are also some issues for libvirt-0.8.7-18.el6_1.2 and
> > libvirt-0.8.2-15.el5_6.5, whether I need to file 2 bugs for them again?
> 
> File a separate BZ for separate build streams.
Okay, thanks Eric.

The bug has been resolved based on Comment 25 and Comment 26, so move the bug to VERIFIED status.

Ouch, one of the patches (conf: plug memory leak on error) for this BZ introduced a crasher: bug 751287

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1513.html