[PATCH] VSMS: fv_vssd_to_domain() resolve Coverity error
by John Ferlan
Coverity discovered that the free(domain->os_info.fv.arch) and then
usage later on during get_default_machine() and get_default_emulator()
calls could result in using free()'d memory.
If the 'cu_get_str_prop() failed or capsinfo == NULL, then the fv.arch
wouldn't necessarily be strdup()'d.
Passing a NULL os_info.fv_arch into the get*() API's is fine since
they'll call findDomainInfo() which can handle a NULL arch value.
Also added an initialization of val just to be safe. I don't think it's
necessary though.
---
NOTE:
I found this during a Coverity run applying the endianness patches. For
some reason Coverity "woke up" and saw this even though it hasn't found
this issue in a couple months of runs since the changes to this module
were made. See commit id '117dabb9'.
src/Virt_VirtualSystemManagementService.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c
index d51f230..9f8b5b9 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -464,7 +464,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
{
int ret = 1;
int retr;
- const char *val;
+ const char *val = NULL;
const char *domtype = NULL;
const char *ostype = "hvm";
struct capabilities *capsinfo = NULL;
@@ -494,6 +494,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
}
free(domain->os_info.fv.arch);
+ domain->os_info.fv.arch = NULL;
retr = cu_get_str_prop(inst, "Arch", &val);
if (retr != CMPI_RC_OK) {
if (capsinfo != NULL) { /* set default */
--
1.8.3.1
10 years, 11 months
- 2
- 2
[PATCH] PanicCheckABIStability: Need to check for existence
by John Ferlan
Commit id '4313fead' added a call to virDomainPanicCheckABIStability()
which did not check whether the panic device existed before making a call
to virDomainDeviceInfoCheckABIStability() which ended up segfaulting:
Thread 1 (Thread 0x7f5332837700 (LWP 10964)):
(src=<optimized out>, dst=<optimized out>)
at conf/domain_conf.c:13007
(dst=<optimized out>, src=<optimized out>)
at conf/domain_conf.c:13712
(src=<optimized out>, dst=<optimized out>)
at conf/domain_conf.c:14056
(domain=domain@entry=0x7f53000057c0, vm=vm@entry=0x7f53000036d0,
defptr=defptr@entry=0x7f5332836978, snap=snap@entry=0x7f5332836970,
update_current=update_current@entry=0x7f5332836962, flags=flags@entry=1)
at conf/snapshot_conf.c:1230
(domain=0x7f53000057c0, xmlDesc=<optimized out>, flags=1)
at qemu/qemu_driver.c:12719
(domain=domain@entry=0x7f53000057c0, xmlDesc=0x7f53000081d0
"<domainsnapshot>\n <name>snap2</name>\n
<description>new-desc</description>\n <state>running</state>\n
<parent>\n <name>snap1</name>\n </parent>\n
<creationTime>1387487268</creationTime>\n <memory s"..., flags=1)
at libvirt.c:19695
...
(gdb) up 3
(gdb) print *other->def->dom
$2 = {virtType = 2, id = -1, ..
...
rng = 0x0, panic = 0x0, namespaceData = 0x0,...
...
(gdb) print *def->dom
$3 = {virtType = 2, id = -1, ...
...
rng = 0x0, panic = 0x0, namespaceData = 0x0,...
...
(gdb)
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
NOTE: Optionally the call could be changed to:
"if (src->panic && !virDomainPanicCheckABIStability(...)"
I went with what I did following the RNGDefCheckABIStability.
src/conf/domain_conf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0079234..c86af9a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13709,6 +13709,9 @@ static bool
virDomainPanicCheckABIStability(virDomainPanicDefPtr src,
virDomainPanicDefPtr dst)
{
+ if (!src && !dst)
+ return true;
+
return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
}
--
1.8.3.1
11 years
- 1
- 1
[PATCH 00/20] REWORK/PARIAL: Changes to solve unsupported tag issue
by John Ferlan
This is a *partial rework* of Xu Wang's patches sent last month:
https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html
Although not the complete set of changes - it's a good stopping point
insomuch as it handles the "others" parsing. If this looks good, I can
push it, then work through the changes to write the xml.
I have run all the changes through cimtest - even with the patches on the
list from Viktor. No new issues are found.
Changes to the original patches
1. I rebased to top of tree as of today (11/14/13)
2. I reworked each of the patches to only add the 'others' to the
particular *_device structure. This was done to be able to show
the progression and to ensure I didn't forget something
2a. This found that there needed to be a cleanup_vcpu_device() and
cleanup_mem_device() since both added "others" to their structure
but were never cleaned up during cleanup_virt_device()
2b. I did not see a need for "others" in "vnc_device" and "sdl_device"
2c. I added a cleanup_others() call in _get_dominfo()
3. I added fetch_device_address_from_others() to replace parse_device_address().
This bridges the "gap" bewteen the parsing of the <address> tag that was
recently added and the 'others' parsing code which didn't handle it.
All I did was traverse the others list looking for parent name and id
match for TYPE_PROP entries. Essentially anything inside of <address...>
us then copied in to name/value structure managed by devaddr. Once the last
caller of parse_device_address() was removed, that function went away
4. I changed the type for unknown from CIM_RES_TYPE_UNKNOWN to
CIM_RES_TYPE_UNKDEV. Turns out the former is used in other contexts
and if/when cleanup_virt_device() was called from one of those contexts
the result was a core in cimprovagt. This was seen in the cimtest
for VirtualSystemManagementService 16_removeresource.py.
4. Various "bug fixes" based on a coverity run and what I saw in code
review from Boris.
- Original code had a "ddev->disk_type == DISK_PHY;"
- There was a "if (node->name == NULL)" check in parse_graphics_device
after "else if (STREQC(gdev->type, "pty")) {" which coverity flagged
as unnecessary since earlier code assumed node->name != NULL
- In parse_os() there was a "STREQC(dominfo->os_info.pv.type, "linux")"
which needed a "dominfo->os_info.pv.type &&" prior to it since the value
could have been NULL according to a check earlier in the routine
- I added the "free(dev->name);" to cleanup_unknown_device()
- Checks for others->{name|parent_name|value} were removed. Since the
structures are calloc()'d and free(NULL) does nothing, this is OK.
Xu Wang (20):
Add others member for saving unsupported tag and unknown device
Add others and unknown_device clean up
Add basic operations for reading data from xml node
Fix xml parsing algorithm for parse_fs_device()
Fix xml parsing algorithm for parse_block_device()
Fix xml parsing algorithm for parse_vsi_device()
Fix xml parsing algorithm for parse_net_device()
Fix xml parsing algorithm for parse_vcpu_device()
Fix xml parsing algorithm for parse_emu_device()
Fix xml parsing algorithm for parse_mem_device()
Fix xml parsing algorithm for parse_console_device()
Fix xml parsing algorithm for parse_graphics_device()
Fix xml parsing algorithm for parse_input_device()
Add parse_unknown_device()
Add parse_devices() for unknown type in get_dominfo_from_xml()
Fix xml parsing algorithm in parse_domain()
Fix xml parsing algorithm in parse_os()
Fix xml parsing algorithm in parse_features()
Add dup function for device copy
Add type CIM_RES_TYPE_DELETED and modify type as it after resource_del
libxkutil/device_parsing.c | 2055 ++++++++++++++++++++++++-----
libxkutil/device_parsing.h | 58 +
src/Virt_VirtualSystemManagementService.c | 2 +-
src/svpc_types.h | 2 +
4 files changed, 1774 insertions(+), 343 deletions(-)
--
1.8.3.1
11 years
- 5
- 36
- ← Newer
- 1
- Older →