Bug 747514

Created attachment 529189 [details]
CoverityScan-0.8.2-15.el5_6.5

Bug 747514 analysis - after analysis, I can state:

Might be worth backporting:
c1fd7d7
c24c07f
5a81401
2ea9409
9892f7b


Error: CHECKED_RETURN:
/builddir/build/BUILD/libvirt-0.8.2/src/util/storage_file.c:431: check_return: Calling function "virAsprintf" without checking return value (as is done elsewhere 264 out of 266 times).

False positive.  Not worth backporting upstream 44ebb18.


Error: CHECKED_RETURN:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:2658: check_return: Calling function "qemuBuildDeviceAddressStr" without checking return value (as is done elsewhere 5 out of 6 times).

Real bug, but can only be provoked by user passing bad XML, and result is merely a poor quality message with no resource leak.  Not worth backporting 14c22b3.


Error: CONSTANT_EXPRESSION_RESULT:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:1237: result_independent_of_operands: (driver->securityDriver && driver->securityDriver->domainClearSecuritySocketLabel && (*driver->securityDriver->domainClearSecuritySocketLabel)(driver->securityDriver, vm)) < 0 is always false regardless of the values of its operands. This occurs as the logical operand of if.

Real bug (two instances this file); worth backporting since it would mask a security driver failure.  Upstream fix in c1fd7d7.


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:1439: dead_error_condition: On this path, the condition "virtfn" cannot be true.

Harmless dead code.  Upstream has added a feature in its place, but feature additions are not appropriate to backport.


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/src/secret/secret_driver.c:521: dead_error_condition: On this path, the condition "list != NULL" cannot be true.

Harmless dead code.  Not worth backporting ba4983d.


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/tests/statstest.c:205: dead_error_condition: On this path, the condition "ret == 0" cannot be false.

Harmless dead code, and in the testsuite.  Not worth backporting ba5c9af.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/daemon/libvirtd.c:2320: var_compare_op: Comparing "server->clients" to null implies that "server->clients" might be null.

Real bug (several times in this function); however, only possible on OOM situations.  Probably not worth backporting 3db08ae.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/src/node_device/node_device_udev.c:1238: var_compare_op: Comparing "parent_sysfs_path" to null implies that "parent_sysfs_path" might be null.

Real bug, but only hits on unlikely error situation.  Not worth backporting 98cd17b.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/src/nwfilter/nwfilter_ebiptables_driver.c:3003: var_compare_op: Comparing "inst" to null implies that "inst" might be null.

False positive: dereference is guarded by nruleInstances.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:3828: var_compare_op: Comparing "def->graphics" to null implies that "def->graphics" might be null.

False positive: dereference is guarded by def->ngraphics.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:343: var_compare_op: Comparing "driver->ebtables" to null implies that "driver->ebtables" might be null.

Real bug, hits on failure to configure mac_filter; could cause segv.  Might be worth backporting commit c24c07f, depending on whether this situation is likely.


Error: MISSING_BREAK:
/builddir/build/BUILD/libvirt-0.8.2/src/conf/nwfilter_conf.c:2482: unterminated_case: This case (value 4) is not terminated by a 'break' statement.

False positives (several times in this file, also tools/virsh.c:5618).  Not worth backporting 1eca8c3.


Error: NULL_RETURNS:
/builddir/build/BUILD/libvirt-0.8.2/src/storage/storage_backend.c:596: returned_null: Function "strstr" returns null (checked 110 out of 113 times).

Real bug, but only possible if qemu-img misbehaves.  Not worth a backport.  Upstream patch just submitted: https://www.redhat.com/archives/libvir-list/2011-October/msg01070.html, now commit c04beb5.


Error: OVERRUN_STATIC:
/builddir/build/BUILD/libvirt-0.8.2/src/util/virtaudit.c:128: overrun-local: Overrunning static array "record_types", with 3 elements, at position 3 with index variable "type".

Real bug, but untriggered in source code.  Not worth backporting 59953c3.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/daemon/remote.c:833: alloc_arg: Calling allocation function "virAllocN" on "params".

Real bugs, but can be triggered only by OOM or malicious rpc packets (several times in this file).  Backporting 158ba87 is prohibitively expensive.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/esx/esx_vmx.c:921: alloc_fn: Calling allocation function "virConfReadMem".

VMX is not part of RHEL; I didn't analyze this one.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/nwfilter/nwfilter_gentech_driver.c:645: alloc_arg: Calling allocation function "virNWFilterRuleInstancesToArray" on "ptrs".

Real bug, and detected with valgrind of typical operation.  Worth backporting 5a81401, since we backported that to 6.1.z.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:6034: alloc_arg: Calling allocation function "virAlloc" on "disk".

Real bugs (two instances this function), can be triggered by domxml-from-native, although that is not called frequently.  Might be worth backporting 2ea9409, depending on the anticipated call rate.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:4433: alloc_fn: Calling allocation function "virSocketFormatAddr".

Real bug, but only possible on OOM.  Fixed incidentally by 6a7e7c4, but that is too invasive to backport.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:7040: alloc_arg: Calling allocation function "qemudProbeMachineTypes" on "machines".

Real bug, but only hits if close() or waitpid() in qemudProbeMachineTypes fails unexpectedly.  Fixed incidentally by 60ae1c3, but that is too invasive to backport.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/storage/storage_backend_fs.c:400: alloc_fn: Calling allocation function "malloc".

Real bug; triggered by starting up mounted storage pools, althouth that tends to be a startup-only task.  Might be worth backporting 9892f7b, if this action is frequent enough.


Error: RETURN_LOCAL:
/builddir/build/BUILD/libvirt-0.8.2/gnulib/lib/areadlink.c:64: local_ptr_assign_local: Assigning: "buffer" = "initial_buf" (address of local variable "initial_buf").

False positive.  Nothing to fix here.


Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.8.2/src/esx/esx_vi.c:1429: deref_ptr: Directly dereferencing pointer "objectSpec".

Harmless dead conditional.  Not worth backporting 1518042.


Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:906: deref_ptr: Directly dereferencing pointer "nth".

Harmless dead conditional.  Not worth backporting d69b79a (especially since df3d8c3 moves it src/util/interface.c).


Error: SIGN_EXTENSION:
/builddir/build/BUILD/libvirt-0.8.2/python/libvirt-override.c:407: sign_extension: Suspicious implicit sign extension: "dominfo.nrVirtCpu" with type "unsigned short" (16 bits, unsigned) is promoted in "dominfo.nrVirtCpu * cpumaplen" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned).  If "dominfo.nrVirtCpu * cpumaplen" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.

Real bug, but can only be triggered by bad callers.  Not worth backporting f73198d.


Error: SIGN_EXTENSION:
/builddir/build/BUILD/libvirt-0.8.2/src/util/storage_file.c:278: sign_extension: Suspicious implicit sign extension: "buf[16]" with type "unsigned char" (8 bits, unsigned) is promoted in "(buf[16] << 24) | (buf[17] << 16) | (buf[18] << 8) | buf[19]" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned).  If "(buf[16] << 24) | (buf[17] << 16) | (buf[18] << 8) | buf[19]" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.

Real bug, but exploiting it would take corrupt qcow2 image.  Not worth backporting 54456cc.


Error: UNINIT:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:7375: var_decl: Declaring variable "guestAddr" without initializer.

Real bug, triggered by hot-plugging a disk, but only possible if qemu lacks -device support, so not triggered on RHEL.  Needs fixing upstream, similar to the fix for https://www.redhat.com/archives/libvir-list/2011-October/msg01074.html, but not worth backporting.


Error: UNUSED_VALUE:
/builddir/build/BUILD/libvirt-0.8.2/src/esx/esx_storage_driver.c:563: returned_pointer: Pointer "vmfsInfo" returned by "esxVI_VmfsDatastoreInfo_DynamicCast(info)" is never used.

ESX is not part of RHEL 5; I didn't analyze this one.


Error: USE_AFTER_FREE:
/builddir/build/BUILD/libvirt-0.8.2/src/conf/domain_conf.c:1259: freed_arg: "virDomainDeviceInfoClear" frees "info->alias".

False positive.  VIR_FREE set info->alias to NULL before second free.


Error: USE_AFTER_FREE:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:852: alias: Assigning: "binary" = "kvmbin". Now both point to the same storage.

False positive.  The code already guarded against a double free.

(In reply to comment #5)
> Bug 747514 analysis - after analysis, I can state:
> 
> Might be worth backporting:
> c1fd7d7
> c24c07f
> 5a81401
> 2ea9409
> 9892f7b

Also:

2c27dfa
https://www.redhat.com/archives/libvir-list/2011-October/msg01275.html

as RHEL 5.8 qemu is old enough to lack -device.

> 
> Error: UNINIT:
> /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:7375: var_decl:
> Declaring variable "guestAddr" without initializer.
> 
> Real bug, triggered by hot-plugging a disk, but only possible if qemu lacks
> -device support, so not triggered on RHEL.  Needs fixing upstream, similar to
> the fix for
> https://www.redhat.com/archives/libvir-list/2011-October/msg01074.html, but not
> worth backporting.

My analysis was wrong - this is indeed worth backporting (it is 2c27dfa in the list above).

In POST (modulo one upstream patch still awaiting review):
http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-October/msg01055.html

(In reply to comment #7)
> In POST (modulo one upstream patch still awaiting review):

Patch 8/8 finally finished the approvals, and is now added to the series.

Created attachment 530816 [details]
CoverityScan result

This is a latest coverity result on libvirt-0.8.2-23.el5.src.rpm.

Eric, it shouldn't belong to this bug, which should be about ZStream, so Need I file a new bug for rhel5.7 or rhel5.8? thanks.

Okay we have a set of fixes for the main issues found out, let's fix
them now that the job of chasing those has been done !

Daniel

Created attachment 530944 [details]
CoverityScan

This is a latest coverity result on libvirt-0.8.2-24.el5.src.rpm.

Analysis summary report:
------------------------
Files analyzed                 : 184
Total LoC input to cov-analyze : 218828
Functions analyzed             : 5142
Paths analyzed                 : 529150
New defects found              : 29 Total
                                  5 CHECKED_RETURN
                                  1 DEADCODE
                                  3 FORWARD_NULL
                                  7 MISSING_BREAK
                                  1 NULL_RETURNS
                                  1 OVERRUN_STATIC
                                  5 RESOURCE_LEAK
                                  1 RETURN_LOCAL
                                  1 REVERSE_INULL
                                  2 SIGN_EXTENSION
                                  1 UNUSED_VALUE
                                  1 USE_AFTER_FREE

Exceeded path limit of 5000 paths in 0.23% of functions (normally up to 5% of functions encounter this limitation)
Elapsed time: 00:08:34

(In reply to comment #12)
> Created attachment 530944 [details]
> CoverityScan
> 
> This is a latest coverity result on libvirt-0.8.2-24.el5.src.rpm.
> 
> Analysis summary report:
> ------------------------
> Files analyzed                 : 184
> Total LoC input to cov-analyze : 218828
> Functions analyzed             : 5142
> Paths analyzed                 : 529150
> New defects found              : 29 Total
>                                   5 CHECKED_RETURN
>                                   1 DEADCODE
>                                   3 FORWARD_NULL
>                                   7 MISSING_BREAK
>                                   1 NULL_RETURNS
>                                   1 OVERRUN_STATIC
>                                   5 RESOURCE_LEAK
>                                   1 RETURN_LOCAL
>                                   1 REVERSE_INULL
>                                   2 SIGN_EXTENSION
>                                   1 UNUSED_VALUE
>                                   1 USE_AFTER_FREE

This is a latest coverity result on libvirt-0.8.2-23.el5.src.rpm.

Analysis summary report:
------------------------
Files analyzed                 : 184
Total LoC input to cov-analyze : 218827
Functions analyzed             : 5142
Paths analyzed                 : 529180
New defects found              : 36 Total
                                  5 CHECKED_RETURN
                                  2 CONSTANT_EXPRESSION_RESULT
                                  1 DEADCODE
                                  4 FORWARD_NULL
                                  7 MISSING_BREAK
                                  1 NULL_RETURNS
                                  1 OVERRUN_STATIC
                                  8 RESOURCE_LEAK
                                  1 RETURN_LOCAL
                                  1 REVERSE_INULL
                                  2 SIGN_EXTENSION
                                  1 UNINIT
                                  1 UNUSED_VALUE
                                  1 USE_AFTER_FREE


There are 7(36-29) issues have been fixed:
2 CONSTANT_EXPRESSION_RESULT
1 FORWARD_NULL
3 RESOURCE_LEAK
1 UNINIT

Moreover, for the rest of issues:
5 CHECKED_RETURN     (Not worth backporting)
1 DEADCODE           (Harmless dead code)
3 FORWARD_NULL       (Not worth backporting/dereference is guarded by previous)
7 MISSING_BREAK      (False positive)
1 NULL_RETURNS       (only possible if qemu-img misbehaves.Not worth a backport)
1 OVERRUN_STATIC     (untriggered in source code. Not worth backporting 59953c3)
5 RESOURCE_LEAK      (VMX is not part of RHEL)
1 RETURN_LOCAL       (False positive.Nothing to fix here)
1 REVERSE_INULL      (Harmless dead conditional)
2 SIGN_EXTENSION     (Not worth backporting f73198d and 54456cc)
1 UNUSED_VALUE       (ESX is not part of RHEL 5)
1 USE_AFTER_FREE     (False positive)


Eric, please confirm these, for RESOURCE_LEAK, Is it also okay for you?

Thanks,
Alex

(In reply to comment #13)
> 
> Eric, please confirm these, for RESOURCE_LEAK, Is it also okay for you?

I confirm that for all the remaining issues identified against -24, the triggers are unlikely (only on OOM or not possible in RHEL), and the fixes for them are too invasive to backport to RHEL 5 for the minimal benefit they might provide.  I think we can mark this verified.

Move to VERIFIED according to Comment 13 and comment 14.

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-2012-0248.html