Created attachment 551752 [details]
CoverityScan-libvirt-0.8.2-25.el5

Alex, can you determine if any of these issues are new in 5.8?

(In reply to comment #2)
> Alex, can you determine if any of these issues are new in 5.8?

Hi Dave, Yeah, I have compared coverity result between -24 and -25, some new resource leaks and NULL pointer derefing are introduced on -25.

(In reply to comment #3)
> Hi Dave, Yeah, I have compared coverity result between -24 and -25, some new
> resource leaks and NULL pointer derefing are introduced on -25.

Ugh, ok; can you file separate BZs on those?

(In reply to comment #4)
> (In reply to comment #3)
> > Hi Dave, Yeah, I have compared coverity result between -24 and -25, some new
> > resource leaks and NULL pointer derefing are introduced on -25.
> 
> Ugh, ok; can you file separate BZs on those?

Hi Dave, according to your advice, a separate bug is used for tracing new resource leaks and NULL pointer deref issues on -25:

https://bugzilla.redhat.com/show_bug.cgi?id=772848

This request was evaluated by Red Hat Product Management for inclusion
in a Red Hat Enterprise Linux release.  Product Management has
requested further review of this request by Red Hat Engineering, for
potential inclusion in a Red Hat Enterprise Linux release for currently
deployed products.  This request is not yet committed for inclusion in
a release.

I'm now going through the list, and determining if any of these represent problems worth backporting (since most, if not all, of these issues have already been fixed upstream).

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).

Safe to ignore.  Upstream commit 44ebb18e not worth backporting.


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

Fixed upstream in commit 14c22b3b; backport is simple enough.


Error: CHECKED_RETURN:
/builddir/build/BUILD/libvirt-0.8.2/tests/shunloadhelper.c:46: check_return: Calling function "virInitialize" without checking return value (as is done elsewhere 16 out of 18 times).

Test code is not installed.  Upstream still has this issue, but even if it is fixed upstream, it is not worth backporting.


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.

Feature added in commit 5abbf7b is not appropriate for backporting.


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/src/libvirt.c:5235: dead_error_line: Execution cannot reach this expression "maxinfo < 0" inside statement "if (!cpumaps ? maplen != 0 ...".

Noise.  Coverity is reading too much into a generic macro for checking integer overflow, and there is nothing to fix.  (Several DEADCODE at this same line).


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/src/secret/secret_driver.c:522: dead_error_begin: Execution cannot reach this statement "virSecretEntryPtr s;".

Harmless to leave in.  Upstream commit ba4983d not worth backporting.


Error: DEADCODE:
/builddir/build/BUILD/libvirt-0.8.2/tests/statstest.c:205: dead_error_line: Execution cannot reach this expression "1" inside statement "return (ret == 0) ? 0 : 1;".

Tests are not installed.  Backporting commit ba5c0af is not worth it.


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

False positive.  def->graphics is non-NULL when ngraphics is non-zero.  Upstream code in qemu_command.c still looks the same.


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.

Fixed for bug 772848.


Error: FORWARD_NULL:
/builddir/build/BUILD/libvirt-0.8.2/src/util/storage_file.c:263: var_compare_op: Comparing "format" to null implies that "format" might be null.

Fixed for bug 772848.


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.
/builddir/build/BUILD/libvirt-0.8.2/src/nwfilter/nwfilter_ebiptables_driver.c:3008: var_deref_op: Dereferencing null variable "inst".

False positive; inst is non-NULL if nRuleInstances is non-zero.  sa_assert() at line 3007 should have silenced Coverity; was this Coverity run properly configured?


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.
/builddir/build/BUILD/libvirt-0.8.2/daemon/libvirtd.c:2340: var_deref_op: Dereferencing null variable "server->clients".

False positive: server->clients is non-NULL if server->nclients is non-zero.


Error: MISSING_BREAK:
/builddir/build/BUILD/libvirt-0.8.2/src/conf/nwfilter_conf.c:1216: unterminated_case: This case (value 4) is not terminated by a 'break' statement.
/builddir/build/BUILD/libvirt-0.8.2/src/conf/nwfilter_conf.c:1218: fallthrough: The above case falls through to this one.

Intentional fallthrough.  Backporting commit 1eca8c3 to shut up Coverity doesn't seem worth it.  (Several MISSING_BREAK affected.)


Error: NULL_RETURNS:
/builddir/build/BUILD/libvirt-0.8.2/src/storage/storage_backend.c:597: dereference: Dereferencing a pointer that might be null "start" when calling "strstr". (The dereference is assumed on the basis of the 'nonnull' parameter attribute.)

Fixed upstream by commit c04beb5d.  However, failure is not possible without a groken qemu-img, so not worth backporting.


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".

Fixed upstream by commit 59953c3, but not triggered in the current codebase so not worth backporting.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:680: leaked_handle: Handle variable "null" going out of scope leaks the handle.

Fixed upstream by commit 60ae1c3, but that commit is invasive and not worth backporting.  Only triggered if libvirtd is run with stdin closed, which is unlikely and therefore not worth worrying about.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:265: leaked_handle: Handle variable "fd" going out of scope leaks the handle.

Fixed upstream by commit 0327ff0.  Only triggered if libvirtd is run with stdin closed, which is unlikely and therefore not worth worrying about.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/esx/esx_vi.c:1471: leaked_storage: Variable "objectSpec" going out of scope leaks the storage it points to.

ESX is not supported on RHEL, and therefore not worth worrying about.  (Couple of RESOURCE_LEAK reports.)


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:4571: leaked_storage: Variable "addr" going out of scope leaks the storage it points to.

Fixed in commit 6a7e7c4 by virCommand rewrite, but this is too invasive to backport.  The leak is only possible on OOM, so not worth worrying about.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:7070: leaked_storage: Variable "machines" going out of scope leaks the storage it points to.

Fixed upstream by commit 2211518 no longer failing after allocation, but backporting virCommand is prohibitive.  Since the error only happens on an unlikely close or waitpid failure, not worth worrying about.


Error: RESOURCE_LEAK:
/builddir/build/BUILD/libvirt-0.8.2/daemon/remote.c:843: leaked_storage: Variable "params" going out of scope leaks the storage it points to.

Fixed upstream with commit 158ba87, but this patch is rather invasive.  The leaks affect mainly error cases, particularly after OOM, and not common-case success paths.  At this point, the risk of backporting outweighs the benefits (several RESOURCE_LEAK issues).


Error: RETURN_LOCAL:
/builddir/build/BUILD/libvirt-0.8.2/gnulib/lib/areadlink.c:113: return_local_addr_alias: Returning pointer "buffer" which points to local variable "initial_buf".

False positive in Coverity.


Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:938: check_after_deref: Dereferencing "nth" before a null check.

This was fixed upstream in commit d69b79ab7, but the code diverged between RHEL 5 and the upstream fix so that the backport is not trivial.  Furthermore, since this is only a dead code warning instead of an exploitable bug, it is not worth fixing.


Error: REVERSE_INULL:
/builddir/build/BUILD/libvirt-0.8.2/src/esx/esx_vi.c:1459: check_after_deref: Dereferencing "objectSpec" before a null check.

See above notes about ESX.


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.

Fixed upstream by commit 54456cc, fix is easy to backport.


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.

Fixed upstream by commit f73198df, but won't hit in practice so 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.

See above notes about ESX.


Error: USE_AFTER_FREE:
/builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)".

Fixed upstream in commit 731c7386.  However, backporting that is rather invasive (at least b16cd226 would also be needed), and the usage on RHEL does not trigger the double free when things are installed correctly, so not worth worrying about.


=======
Summary: 2 commits worth backporting

In POST:
http://post-office.corp.redhat.com/archives/rhvirt-patches/2012-June/msg00226.html

Created attachment 594711 [details]
libvirt covscan error result for libvirt-0.8.2-26.el5

Accroding to comment 12, check two commits which worth backporting,
fixed.

move to verified. New issues found in covscan, trace in Bug 772848

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/RHSA-2013-0127.html