Description of problem: As summary, Memory leaks/NULL pointer dereference often causes programming/process bad issues such as crashing a process, so we should fix them early. Version-Release number of selected component (if applicable): libvirt-0.8.2-25.el5.src.rpm How reproducible: always Steps to Reproduce: 1. CoverityScan 2. 3. Actual results: Analysis summary report: ------------------------ 5 FORWARD_NULL 11 RESOURCE_LEAK 2 REVERSE_INULL Expected results: Fix them. Additional info: Comparing -25 result with -24, there are some new issues: Error: RESOURCE_LEAK: /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:467: open_fn: Calling opening function "open". /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:467: var_assign: Assigning: "null" = handle returned from "open("/dev/null", 0)". /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:678: off_by_one: Testing whether handle "null" is strictly greater than zero is suspicious. Did you intend to include equality with zero? "null" leaks when it is zero. /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:680: leaked_handle: Handle variable "null" going out of scope leaks the handle. Note, it may be okay in here, because the file descriptor shouldn't be 0, please confirm this? Error: RESOURCE_LEAK: /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:256: open_fn: Calling opening function "open". /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:256: var_assign: Assigning: "fd" = handle returned from "open(paths[i], 0)". /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:257: off_by_one: Testing whether handle "fd" is strictly greater than zero is suspicious. Did you intend to include equality with zero? "fd" leaks when it is zero. /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:265: leaked_handle: Handle variable "fd" going out of scope leaks the handle. Note, the same issue with above. 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. /builddir/build/BUILD/libvirt-0.8.2/src/node_device/node_device_udev.c:1243: var_deref_model: Passing null variable "parent_sysfs_path" to function "virNodeDeviceFindBySysfsPath", which dereferences it. /builddir/build/BUILD/libvirt-0.8.2/src/conf/node_device_conf.c:103: deref_parm_in_call: Function "__coverity_strcmp" dereferences parameter "sysfs_path". Note, I think log level is also incorrect in line 1239, it should be warning or error not info, if it is error, codes should follow some cleanup and return operations, but can't see these in codes, and although comparing 'NULL' with 'parent_sysfs_path', if it's NULL, the 1243 line still deref the NULL pointer. 1237 parent_sysfs_path = udev_device_get_syspath(parent_device); 1238 if (parent_sysfs_path == NULL) { 1239 VIR_INFO("Could not get syspath for parent of '%s'", 1240 udev_device_get_syspath(parent_device)); 1241 } 1242 1243 dev = virNodeDeviceFindBySysfsPath(&driverState->devs, 1244 parent_sysfs_path); 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. /builddir/build/BUILD/libvirt-0.8.2/src/util/storage_file.c:320: var_deref_model: Passing null variable "format" to function "qcow2GetBackingStoreFormat", which dereferences it. /builddir/build/BUILD/libvirt-0.8.2/src/util/storage_file.c:238: deref_parm: Directly dereferencing parameter "format". Note, as above 'Error' said, if 'format' is NULL, the line 320 still deref the NULL pointer, the line 319 should be changed like this "if (format && isQCow2)" 263 if (format) 264 *format = VIR_STORAGE_FILE_AUTO; ...... 319 if (isQCow2) 320 qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, offset); Error: REVERSE_INULL: /builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:906: deref_ptr: Directly dereferencing pointer "nth". /builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:938: check_after_deref: Dereferencing "nth" before a null check. Note, as above 'Error' said, it's okay for current upstream, codes should remove line 938 and 0 is the interface itself. 895 static int 896 ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent, 897 int *parent_ifindex, char *parent_ifname, 898 unsigned int *nth) 899 { 900 int rc; 901 struct nlattr *tb[IFLA_MAX + 1] = { NULL, }; 902 char *recvbuf = NULL; 903 bool end = false; 904 unsigned int i = 0; 905 906 *nth = 0; ...... 938 if (nth) 939 *nth = i - 1;
(In reply to comment #0) > Additional info: > > Comparing -25 result with -24, there are some new issues: > > Error: RESOURCE_LEAK: > /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:467: open_fn: Calling > opening function "open". > /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:467: var_assign: Assigning: > "null" = handle returned from "open("/dev/null", 0)". > /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:678: off_by_one: Testing > whether handle "null" is strictly greater than zero is suspicious. Did you > intend to include equality with zero? "null" leaks when it is zero. > /builddir/build/BUILD/libvirt-0.8.2/src/util/util.c:680: leaked_handle: Handle > variable "null" going out of scope leaks the handle. > > Note, it may be okay in here, because the file descriptor shouldn't be 0, > please confirm this? In theory, the code has a leak. In practice, libvirt is never run with stdin closed, so 'null' will never be zero, and there is no leak. This has been fixed upstream, by conversion to VIR_FORCE_CLOSE and subsequent moving of the code in question to command.c, but I don't think we need to worry about it for 5.8. > > Error: RESOURCE_LEAK: > /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:256: open_fn: Calling > opening function "open". > /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:256: var_assign: Assigning: > "fd" = handle returned from "open(paths[i], 0)". > /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:257: off_by_one: Testing > whether handle "fd" is strictly greater than zero is suspicious. Did you > intend to include equality with zero? "fd" leaks when it is zero. > /builddir/build/BUILD/libvirt-0.8.2/src/util/uuid.c:265: leaked_handle: Handle > variable "fd" going out of scope leaks the handle. > > Note, the same issue with above. This bug is still present upstream; but the same comments (about fd 0 always being open in practice) applies, so I don't think we need to worry about it for 5.8. > > > 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. > /builddir/build/BUILD/libvirt-0.8.2/src/node_device/node_device_udev.c:1243: > var_deref_model: Passing null variable "parent_sysfs_path" to function > "virNodeDeviceFindBySysfsPath", which dereferences it. > /builddir/build/BUILD/libvirt-0.8.2/src/conf/node_device_conf.c:103: > deref_parm_in_call: Function "__coverity_strcmp" dereferences parameter > "sysfs_path". > > Note, I think log level is also incorrect in line 1239, it should be warning or > error not info, if it is error, codes should follow some cleanup and return > operations, but can't see these in codes, and although comparing 'NULL' with > 'parent_sysfs_path', if it's NULL, the 1243 line still deref the NULL pointer. > > 1237 parent_sysfs_path = udev_device_get_syspath(parent_device); > 1238 if (parent_sysfs_path == NULL) { > 1239 VIR_INFO("Could not get syspath for parent of '%s'", > 1240 udev_device_get_syspath(parent_device)); > 1241 } > 1242 > 1243 dev = virNodeDeviceFindBySysfsPath(&driverState->devs, > 1244 parent_sysfs_path); This was fixed upstream in commit 98cd17bd18; it's probably worth backporting since it is so simple. At the same time, this error path is unlikely to trigger in normal cases, so I'm okay if this doesn't go in until z-stream. I'll look at the rest later.
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.
(In reply to comment #2) For 5.9, we don't need to worry about libvirtd ever being run with fd 0 closed (or, more precisely, the size of the backports to protect against fd 0 being closed is not worth the risk). > > I'll look at the rest later. (In reply to comment #0) > > 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. > Note, as above 'Error' said, if 'format' is NULL, the line 320 still deref > the NULL pointer, the line 319 should be changed like this "if (format && > isQCow2)" Fixed upstream by commit a001a5; trivial to backport. > Error: REVERSE_INULL: > /builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:906: deref_ptr: > Directly dereferencing pointer "nth". > /builddir/build/BUILD/libvirt-0.8.2/src/util/macvtap.c:938: > check_after_deref: Dereferencing "nth" before a null check. > > Note, as above 'Error' said, it's okay for current upstream, codes should > remove line 938 and 0 is the interface itself. 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. That leaves this bug with just two patches worth applying; I will post those patches shortly.
In POST: http://post-office.corp.redhat.com/archives/rhvirt-patches/2012-June/msg00217.html
run Coverity scan for build libvirt-0.8.2-26.el5 defects please see attachment
Created attachment 594707 [details] libvirt covscan error result
checking with the result util: avoid null deref on qcowXGetBackingStore and node_device: avoid null dereference on error all fixed. but found some new issues: USE_AFTER_FREE: Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:856: alias: Assigning: "binary" = "kvmbin". Now both point to the same storage. /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:1015: freed_arg: "virFree" frees "binary". /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:1016: double_free: Calling "virFree" frees pointer "kvmbin" which has already been freed. /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". Error: USE_AFTER_FREE: /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8310: freed_arg: "usbFreeDevice" frees "usb". /builddir/build/BUILD/libvirt-0.8.2/src/util/hostusb.c:315: freed_arg: "virFree" frees parameter "dev". /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8359: pass_freed_arg: Passing freed pointer "usb" as an argument to function "usbDeviceListSteal". Hi Eric: Please help to confirm these issues, very thanks!
Created attachment 594709 [details] scan result with patch
(In reply to comment #13) > checking with the result > util: avoid null deref on qcowXGetBackingStore and node_device: avoid null > dereference on error all fixed. > but found some new issues: > USE_AFTER_FREE: > > Error: USE_AFTER_FREE: > /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:856: alias: > Assigning: "binary" = "kvmbin". Now both point to the same storage. False alarm (aka Coverity issue, not a libvirt bug). > /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:1015: freed_arg: > "virFree" frees "binary". But line 1015 can't be reached if 'binary == kvmbin' (line 1011), and therefore the code already takes care to avoid a double free. > > Error: USE_AFTER_FREE: > /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8310: freed_arg: > "usbFreeDevice" frees "usb". Real bug introduced in libvirt-0.8.2-26.el5, but should be fixed in libvirt-0.8.2-28.el5 which touched the code some more. If you want to run yet one more coverity scan, be my guest; but I'm happy that we've resolved everything worth resolving for RHEL 5.
Hi Eric: I run coverity scan with libvirt-0.8.2-28.el5,but also met issues,same with libvirt-0.8.2-26.el5. Error: USE_AFTER_FREE (CWE-416): /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8366: freed_arg: "usbFreeDevice" frees "usb". /builddir/build/BUILD/libvirt-0.8.2/src/util/hostusb.c:315: freed_arg: "virFree" frees parameter "dev". /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8417: pass_freed_arg: Passing freed pointer "usb" as an argument to function "usbDeviceListSteal" is the bug fixed? or the patch not in build?please help confirm this, very thanks.
(In reply to comment #16) > Hi Eric: > I run coverity scan with libvirt-0.8.2-28.el5,but also met issues,same with > libvirt-0.8.2-26.el5. > > Error: USE_AFTER_FREE (CWE-416): > /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_driver.c:8366: freed_arg: > "usbFreeDevice" frees "usb". Real regression, present upstream. Upstream fix proposed: https://www.redhat.com/archives/libvir-list/2012-July/msg01722.html
Back in POST: http://post-office.corp.redhat.com/archives/rhvirt-patches/2012-July/msg00331.html
Okay I just made a libvirt-0.8.2-29.el5 fixing that USE_AFTER_FREE from comment #16, so need to be QE'ed again Daniel
Run coverity scan with libvirt-0.8.2-29.el5, result: ....... Error: USE_AFTER_FREE (CWE-416): /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:856: alias: Assigning: "binary" = "kvmbin". Now both point to the same storage. /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:1015: freed_arg: "virFree" frees "binary". /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". /builddir/build/BUILD/libvirt-0.8.2/src/qemu/qemu_conf.c:1016: double_free: Calling "virFree" frees pointer "kvmbin" which has already been freed. /builddir/build/BUILD/libvirt-0.8.2/src/util/memory.c:283: freed_arg: "free" frees parameter "*((void **)ptrptr)". no new critical issue found,according to comment 15 and comment 17,verification passed.
Created attachment 601641 [details] full scan result for libvirt-0.8.2-29
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