mem_nodes[i].ndistances is written outside the loop causing an out-of-bounds
write leading to heap corruption.
While we are at it, the entire cleanup portion can be removed as it can be
handled in virDomainNumaFree. One instance of VIR_FREE is also removed and
replaced with g_autofree.
This patch also adds a testcase which would be picked up by ASAN, if this
portion regresses.
Signed-off-by: Rayhan Faizel <rayhan.faizel@xxxxxxxxx>
---
src/conf/numa_conf.c | 30 ++++++-------------
...ance-nonexistent-sibling.x86_64-latest.err | 1 +
.../cpu-numa-distance-nonexistent-sibling.xml | 29 ++++++++++++++++++
tests/qemuxmlconftest.c | 1 +
4 files changed, 40 insertions(+), 21 deletions(-)
create mode 100644 tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index d8120de6d2..0a0e2911f7 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -343,8 +343,7 @@ virDomainNumaFree(virDomainNuma *numa)
virBitmapFree(numa->mem_nodes[i].cpumask);
virBitmapFree(numa->mem_nodes[i].nodeset);
- if (numa->mem_nodes[i].ndistances > 0)
- g_free(numa->mem_nodes[i].distances);
+ g_free(numa->mem_nodes[i].distances);
g_free(numa->mem_nodes[i].caches);
}
@@ -685,9 +684,8 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
xmlXPathContextPtr ctxt,
unsigned int cur_cell)
{
- int ret = -1;
int sibling;
- xmlNodePtr *nodes = NULL;
+ g_autofree xmlNodePtr *nodes = NULL;
size_t i, ndistances = def->nmem_nodes;
if (ndistances == 0)
@@ -698,12 +696,12 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
return 0;
if ((sibling = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes)) < 0)
- goto cleanup;
+ return -1;
if (sibling == 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("NUMA distances defined without siblings"));
- goto cleanup;
+ return -1;
}
for (i = 0; i < sibling; i++) {
@@ -713,19 +711,19 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
if (virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_REQUIRED,
&sibling_id) < 0)
- goto cleanup;
+ return -1;
/* The "id" needs to be within numa/cell range */
if (sibling_id >= ndistances) {
virReportError(VIR_ERR_XML_ERROR,
_("'sibling_id %1$d' does not refer to a valid cell within NUMA 'cell id %2$d'"),
sibling_id, cur_cell);
- goto cleanup;
+ return -1;
}
if (virXMLPropUInt(nodes[i], "value", 10, VIR_XML_PROP_REQUIRED,
&sibling_value) < 0)
- goto cleanup;
+ return -1;
/* Assure LOCAL_DISTANCE <= "value" <= UNREACHABLE
* and correct LOCAL_DISTANCE setting if such applies.
@@ -739,7 +737,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
virReportError(VIR_ERR_XML_ERROR,
_("'value %1$d' is invalid for 'sibling id %2$d' under NUMA 'cell id %3$d'"),
sibling_value, sibling_id, cur_cell);
- goto cleanup;
+ return -1;
}
/* Apply the local / remote distance */
@@ -770,17 +768,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
rdist[cur_cell].value = sibling_value;
}
- ret = 0;
-
- cleanup:
- if (ret < 0) {
- for (i = 0; i < ndistances; i++)
- VIR_FREE(def->mem_nodes[i].distances);
- def->mem_nodes[i].ndistances = 0;
- }
- VIR_FREE(nodes);
-
- return ret;
+ return 0;
}
diff --git a/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
new file mode 100644
index 0000000000..4866ff5e80
--- /dev/null
+++ b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: 'sibling_id 2' does not refer to a valid cell within NUMA 'cell id 1'
diff --git a/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
new file mode 100644
index 0000000000..62a6c32fbe
--- /dev/null
+++ b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
@@ -0,0 +1,29 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>16</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='network'/>
+ </os>
+ <cpu>
+ <topology sockets='2' dies='1' cores='4' threads='2'/>
+ <numa>
+ <cell id='1' cpus='8-15' memory='109550' unit='KiB'>
+ <distances>
+ <sibling id='2' value='10'/>
+ </distances>
+ </cell>
+ <cell id='0' cpus='0-7' memory='109550' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 4a711fceeb..af49934c33 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2160,6 +2160,7 @@ mymain(void)
DO_TEST_CAPS_LATEST_PARSE_ERROR("cpu-numa3");
DO_TEST_CAPS_LATEST("cpu-numa-disjoint");
DO_TEST_CAPS_LATEST("cpu-numa-memshared");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("cpu-numa-distance-nonexistent-sibling");
/* host-model cpu expansion depends on the cpu reported by qemu and thus
* we invoke it for all real capability dumps we have */
--
2.34.1
