On 07/09/2013 06:13 AM, Jincheng Miao wrote:

The index of the controller should not be limited in [zero, INT_MAX]. So use virStrToLong_ui() and check the limit of the controller index in virDomainControllerDefParseXML(). --- src/conf/domain_conf.c | 6 +++--- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/vmx/vmx.c | 3 +-- 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 402e6e9..d4c1054 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2655,7 +2655,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def)

for (i = 0; i < def->ncontrollers; i++) { cont = def->controllers[i]; - if (cont->idx > max_idx[cont->type]) + if ((int)cont->idx > max_idx[cont->type]) ^ missing space

max_idx[cont->type] = cont->idx; }

@@ -2663,7 +2663,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def) max_idx[VIR_DOMAIN_CONTROLLER_TYPE_USB] = -1;

for (i = 0; i < VIR_DOMAIN_CONTROLLER_TYPE_LAST; i++) { - if (max_idx[i] >= 0 && !(bitmaps[i] = virBitmapNew(max_idx[i] + 1))) + if (max_idx[i] >= 0 && !(bitmaps[i] = virBitmapNew((size_t)max_idx[i] + 1)))

This cast seems unnecessary.

goto no_memory; nbitmaps++; } @@ -5593,7 +5593,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,

idx = virXMLPropString(node, "index"); if (idx) { - if (virStrToLong_i(idx, NULL, 10, &def->idx) < 0) { + if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 || def->idx > INT_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse controller index %s"), idx); goto error;

If we parse it as unsigned, we should print it as unsigned too. I've split the line over 80 columns and squashed this in: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b515887..354131e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14382,7 +14382,7 @@ virDomainControllerDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, - " <controller type='%s' index='%d'", + " <controller type='%s' index='%u'", type, def->idx); if (model) {

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index da83eb6..7897b4b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -790,7 +790,7 @@ struct _virDomainVirtioSerialOpts { /* Stores the virtual disk controller configuration */ struct _virDomainControllerDef { int type; - int idx; + unsigned int idx; int model; /* -1 == undef */ unsigned int queues; union { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 46db28a..7fd1cbf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1585,7 +1585,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,

for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->idx > max_idx) + if ((int)def->controllers[i]->idx > max_idx) ^ missing space max_idx = def->controllers[i]->idx; } } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 5464d13..deddfaa 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1664,8 +1664,7 @@ virVMXParseConfig(virVMXContext *ctx,

for (controller = 0; controller < def->ncontrollers; ++controller) { if (def->controllers[controller]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - if (def->controllers[controller]->idx < 0 || - def->controllers[controller]->idx > 3) { + if (def->controllers[controller]->idx > 3) { virReportError(VIR_ERR_INTERNAL_ERROR, _("SCSI controller index %d out of [0..3] range"), def->controllers[controller]->idx);

ACK and pushed with a test added: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml new file mode 100644 index 0000000..be3d8f2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml @@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>fdr-br</name> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='pc-1.2'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='-1' model='pci-bridge'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d0d9cad..7d7332f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1015,6 +1015,9 @@ mymain(void) DO_TEST("mlock-off", QEMU_CAPS_MLOCK); DO_TEST("mlock-unsupported", NONE); + DO_TEST_PARSE_ERROR("pci-bridge-negative-index-invalid", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); Jan