Hide Forgot
Description of problem: Coverity found some issues by defecting libvirt-0.8.7-18.el6_1.2.src.rpm, for details, please see attachment. Version-Release number of selected component (if applicable): libvirt-0.8.7-18.el6_1.2.src.rpm How reproducible: always Steps to Reproduce: 1. Coverity scan Actual results: Error: UNINIT: /builddir/build/BUILD/libvirt-0.8.7/src/lxc/veth.c:94: var_decl: Declaring variable "rc" without initializer. /builddir/build/BUILD/libvirt-0.8.7/src/lxc/veth.c:132: uninit_use: Using uninitialized value "rc". Error: UNINIT: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_hotplug.c:436: var_decl: Declaring variable "driveAddr" without initializer. /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_hotplug.c:445: uninit_use_in_call: Using uninitialized value "driveAddr": field "driveAddr".controller is uninitialized when calling "memcpy". Error: RESOURCE_LEAK: /builddir/build/BUILD/libvirt-0.8.7/src/storage/storage_backend_iscsi.c:588: alloc_fn: Calling allocation function "virStoragePoolDefParseSourceString". /builddir/build/BUILD/libvirt-0.8.7/src/conf/storage_conf.c:521: alloc_arg: "virAlloc" opens handle stored into "def". ...... Expected results: There are some items are worth fixing. Additional info:
Created attachment 529193 [details] CoverityScan-0.8.7-18.el6_1.2
After five hours analysis, I can state: Upstream commits that might be worth backporting: c24c07f 2ea9409 9892f7b https://www.redhat.com/archives/libvir-list/2011-October/msg01072.html https://www.redhat.com/archives/libvir-list/2011-October/msg01074.html The last two would also require backporting to 6.2. Error: CHECKED_RETURN: /builddir/build/BUILD/libvirt-0.8.7/src/util/storage_file.c:514: check_return: Calling function "virAsprintf" without checking return value (as is done elsewhere 322 out of 332 times). False positive. Not worth backporting upstream 44ebb18. Error: CHECKED_RETURN: /builddir/build/BUILD/libvirt-0.8.7/src/lxc/lxc_container.c:907: check_return: Calling function "waitpid" without checking return value (as is done elsewhere 16 out of 19 times). Real bug (potential, but rare, resource leak); but LXC is tech preview, so not worth backporting any fix. Upstream patch just submitted: https://www.redhat.com/archives/libvir-list/2011-October/msg01058.html Error: CHECKED_RETURN: /builddir/build/BUILD/libvirt-0.8.7/src/network/bridge_driver.c:146: check_return: Calling function "virAsprintf" without checking return value (as is done elsewhere 322 out of 332 times). Several false positives in this file. Not worth backporting upstream 44ebb18. Error: CHECKED_RETURN: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_audit.c:267: check_return: Calling function "virCgroupPathOfController" without checking return value (as is done elsewhere 4 out of 5 times). False positive. Not worth backporting upstream aaea56d. Error: CHECKED_RETURN: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_audit.c:48: check_return: Calling function "virAsprintf" without checking return value (as is done elsewhere 322 out of 332 times). False positive. Not worth backporting upstream 44ebb18 (especially due to conflicts with file motion to src/conf/domain_audit.c in meantime). Error: CONSTANT_EXPRESSION_RESULT: /builddir/build/BUILD/libvirt-0.8.7/src/util/macvtap.c:1059: missing_parentheses: !nla_put(nl_msg, 1, sizeof (ifla_vf_mac) /*36*/, &ifla_vf_mac) < 0 is always false regardless of the values of its operands. Did you intend to either negate the entire comparison expression, in which case parentheses would be required around the entire comparison expression to force that interpretation, or negate the sense of the comparison (that is, use '>=' rather than '<')? This occurs as the logical operand of if. Real bug (twice in this file), but no consequence, per the analysis in commit be23e2b. No point backporting that commit. Error: DEADCODE: /builddir/build/BUILD/libvirt-0.8.7/src/util/macvtap.c:1393: 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.7/src/secret/secret_driver.c:520: dead_error_condition: On this path, the condition "list != NULL" cannot be true. Harmless dead code. Not worth backporting ba4983d. Error: FORWARD_NULL: /builddir/build/BUILD/libvirt-0.8.7/src/util/command.c:827: var_compare_op: Comparing "cmd->errbuf" to null implies that "cmd->errbuf" 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.7/daemon/libvirtd.c:2362: var_compare_op: Comparing "server->clients" to null implies that "server->clients" might be null. False positive: dereference is guarded by for loop that is skipped if server->nclients is 0. Error: FORWARD_NULL: /builddir/build/BUILD/libvirt-0.8.7/src/node_device/node_device_udev.c:1239: 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.7/src/nwfilter/nwfilter_ebiptables_driver.c:3331: 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.7/src/qemu/qemu_command.c:2895: 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.7/src/qemu/qemu_conf.c:393: 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.7/src/conf/nwfilter_conf.c:2697: unterminated_case: This case (value 4) is not terminated by a 'break' statement. False positives (several times in this file). Not worth backporting 1eca8c3. Error: MISSING_RETURN: /tmp/tmpzJMDul.c:1: missing_return: Arriving at the end of a function without returning a value. Not libvirt's fault. dtrace generates a bogus function with int return type but no body. Nothing we can do about it. Error: NEGATIVE_RETURNS: /builddir/build/BUILD/libvirt-0.8.7/src/util/command.c:843: var_tested_neg: Variable "infd" tests negative. False positive. Not worth backporting 89e651f. Error: NULL_RETURNS: /builddir/build/BUILD/libvirt-0.8.7/src/storage/storage_backend.c:621: returned_null: Function "strstr" returns null (checked 148 out of 151 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 Error: OVERRUN_STATIC: /builddir/build/BUILD/libvirt-0.8.7/src/util/virtaudit.c:126: 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.7/src/util/command.c:128: alloc_fn: Calling allocation function "virCommandNew". Real bug, but only triggered on OOM. Not worth backporting bb88952. Error: RESOURCE_LEAK: /builddir/build/BUILD/libvirt-0.8.7/daemon/remote.c:2391: 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.7/src/vmx/vmx.c:1208: 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.7/src/qemu/qemu_command.c:5637: alloc_arg: Calling allocation function "virAlloc" on "def". 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.7/src/storage/storage_backend_fs.c:397: 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: RESOURCE_LEAK: /builddir/build/BUILD/libvirt-0.8.7/src/storage/storage_backend_iscsi.c:588: alloc_fn: Calling allocation function "virStoragePoolDefParseSourceString". Real bug, but only triggered by virConnectFindStoragePoolSources when iscsi is present. Might be worth backporting proposed upstream patch 1/2 (RHEL 6.2 would also need this fix), depending on whether this is likely to be frequent: https://www.redhat.com/archives/libvir-list/2011-October/msg01072.html Error: RETURN_LOCAL: /builddir/build/BUILD/libvirt-0.8.7/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.7/src/esx/esx_vi.c:1653: deref_ptr: Directly dereferencing pointer "objectSpec". Harmless dead conditional. Not worth backporting 1518042. Error: REVERSE_INULL: /builddir/build/BUILD/libvirt-0.8.7/src/util/macvtap.c:867: 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.7/python/libvirt-override.c:421: 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.7/src/util/storage_file.c:294: 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.7/src/lxc/veth.c:94: var_decl: Declaring variable "rc" without initializer. Real bug, but LXC is tech preview, so not worth backporting c59176c. Error: UNINIT: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_hotplug.c:436: var_decl: Declaring variable "driveAddr" without initializer. Real bug, triggered by hot-plugging a disk. Might be worth backporting proposed upstream patch (RHEL 6.2 would also need this fix): https://www.redhat.com/archives/libvir-list/2011-October/msg01074.html Error: UNUSED_VALUE: /builddir/build/BUILD/libvirt-0.8.7/tools/virsh.c:619: returned_pointer: Pointer "c" returned by "vshCmddefSearch(name)" is never used. Harmless dead assignment (twice this function). Not worth backporting 1a82c5f. Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.7/src/conf/domain_conf.c:1563: freed_arg: "virDomainDeviceInfoClear" frees "info->alias". False positive; Coverity failed to realize that free(NULL) is safe. Still need to analyze these two. Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.7/src/fdstream.c:217: alias: Assigning: "fdst" = "st->privateData". Now both point to the same storage. /builddir/build/BUILD/libvirt-0.8.7/src/fdstream.c:225: freed_arg: "virFDStreamFree" frees "fdst". /builddir/build/BUILD/libvirt-0.8.7/src/fdstream.c:209: freed_arg: "virFree" frees parameter "fdst". /builddir/build/BUILD/libvirt-0.8.7/src/util/memory.c:311: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.7/src/fdstream.c:229: deref_after_free: Dereferencing freed pointer "fdst". Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_capabilities.c:475: alias: Assigning: "binary" = "kvmbin". Now both point to the same storage. /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_capabilities.c:644: freed_arg: "virFree" frees "binary". /builddir/build/BUILD/libvirt-0.8.7/src/util/memory.c:311: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_capabilities.c:645: double_free: Calling "virFree" frees pointer "kvmbin" which has already been freed.
In POST for 6.2: http://post-office.corp.redhat.com/archives/rhvirt-patches/2011-October/msg00974.html This bug needs to be cloned to cover 6.1.z fixes.
Created attachment 530222 [details] CoverityScan-20 Fixed in version libvirt-0.9.4-20.el6, the problem is that coverity hasn't found previous issues such as iscsi memory leak and leaking uninit data from hotplug to dumpxml on libvirt-0.9.4-18.el6, in fact, there is the same coverity result between version libvirt-0.9.4-20.el6 and libvirt-0.9.4-18.el6, although some fixing is worth backporting to rhel6.2, maybe, we need to build a package with these fixing for 6.1.z? Alex
The bug has been verified on libvirt-0.9.4-20.el6 based on Comment 9(coverity result), Comment 10 and Comment 13, so move the bug to VERIFIED status.
Corrections to comment 4: > Error: UNINIT: > /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_hotplug.c:436: var_decl: > Declaring variable "driveAddr" without initializer. > > Real bug, triggered by hot-plugging a disk. Might be worth backporting > proposed upstream patch (RHEL 6.2 would also need this fix): > https://www.redhat.com/archives/libvir-list/2011-October/msg01074.html On further analysis, the bug can only be triggered by qemu that lacks -device, so RHEL is immune. It's already in the 6.2 via 0.9.4-20, so it is not worth reverting there; but since it cannot affect RHEL, it is not worth backporting 2c27dfa to 6.1.z. > Still need to analyze these two. Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.7/src/fdstream.c:217: alias: Assigning: "fdst" = "st->privateData". Now both point to the same storage. Real bug, could crash libvirtd if an ill-timed context switch tries to treat random memory as a mutex to unlock. Worth backporting 34b999b to 6.1.z (already in 6.2). Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.7/src/qemu/qemu_capabilities.c:475: alias: Assigning: "binary" = "kvmbin". Now both point to the same storage. False positive. The code already guarded against a double free.
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