[libvirt] [PATCH V2] virsh: forbid negative vcpu argument to vcpupin.
vcpupin will allow argument --vcpu as a signed number, and pass it to virDomainPinVcpu directlly without checking if this value is positive(valid).
virsh vcpupin r7 -1 0 error: numerical overflow: input too large: 4294967295
This message is inaccurate, and the negative vcpu is non-valuable. So forbid vcpu argument as a negative. After patching, the result likes:
virsh vcpupin r6 -1 error: vcpupin: Invalid vCPU number.
virsh vcpupin r6 --cpulist 0-1 error: vcpupin: Missing vCPU number in pin mode.
virsh vcpupin r6 --vcpu ABC error: vcpupin: Invalid vCPU number in query mode.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- tools/virsh-domain.c | 42 ++++++++++++++++++++++++------------------ 1 files changed, 24 insertions(+), 18 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..e302459 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5797,7 +5797,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; virDomainPtr dom; - int vcpu = -1; + unsigned int vcpu; const char *cpulist = NULL; bool ret = false; unsigned char *cpumap = NULL; @@ -5809,6 +5809,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); bool query = false; /* Query mode if no cpulist */ + int get_vcpu; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -5830,29 +5831,34 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) query = !cpulist; - /* In query mode, "vcpu" is optional */ - if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) { - vshError(ctl, "%s", - _("vcpupin: Invalid or missing vCPU number.")); - virDomainFree(dom); - return false; - } - - if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { - virDomainFree(dom); - return false; + get_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu); + if (get_vcpu <= 0) { + /* In query mode, "vcpu" is optional */ + if (query && get_vcpu < 0) { + vshError(ctl, "%s", + _("vcpupin: Invalid vCPU number in query mode.")); + goto cleanup; + } + /* In pin mode, "vcpu" is necessary */ + if (!query) { + vshError(ctl, "%s", + _("vcpupin: Missing vCPU number in pin mode.")); + goto cleanup; + } } if (virDomainGetInfo(dom, &info) != 0) { vshError(ctl, "%s", _("vcpupin: failed to get domain information.")); - virDomainFree(dom); - return false; + goto cleanup; } if (vcpu >= info.nrVirtCpu) { vshError(ctl, "%s", _("vcpupin: Invalid vCPU number.")); - virDomainFree(dom); - return false; + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { + goto cleanup; } cpumaplen = VIR_CPU_MAPLEN(maxcpu); @@ -5871,7 +5877,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); vshPrintExtra(ctl, "----------------------------------\n"); for (i = 0; i < ncpus; i++) { - if (vcpu != -1 && i != vcpu) + if (get_vcpu > 0 && i != vcpu) continue; vshPrint(ctl, "%4zu: ", i); @@ -5880,8 +5886,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!ret) break; } - } + VIR_FREE(cpumaps); goto cleanup; } -- 1.7.1
On 05/29/14 05:34, Jincheng Miao wrote:
vcpupin will allow argument --vcpu as a signed number, and pass it to virDomainPinVcpu directlly without checking if this value is positive(valid).
virsh vcpupin r7 -1 0 error: numerical overflow: input too large: 4294967295
This message is inaccurate, and the negative vcpu is non-valuable. So forbid vcpu argument as a negative.
After patching, the result likes:
virsh vcpupin r6 -1 error: vcpupin: Invalid vCPU number.
virsh vcpupin r6 --cpulist 0-1 error: vcpupin: Missing vCPU number in pin mode.
virsh vcpupin r6 --vcpu ABC error: vcpupin: Invalid vCPU number in query mode.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- tools/virsh-domain.c | 42 ++++++++++++++++++++++++------------------ 1 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..e302459 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5797,7 +5797,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; virDomainPtr dom; - int vcpu = -1; + unsigned int vcpu;
This needs to be initialized to 0 otherwise it might be used uninitialized if the user didn's specify any argument.
const char *cpulist = NULL; bool ret = false; unsigned char *cpumap = NULL; @@ -5809,6 +5809,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); bool query = false; /* Query mode if no cpulist */ + int get_vcpu;
we usually use past tense for such names: s/get_cpu/got_cpu/
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -5830,29 +5831,34 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
query = !cpulist;
- /* In query mode, "vcpu" is optional */ - if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) { - vshError(ctl, "%s", - _("vcpupin: Invalid or missing vCPU number.")); - virDomainFree(dom); - return false; - } - - if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { - virDomainFree(dom); - return false; + get_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu);
Still not quite right ... vshCommandOptUInt currently wraps negative numbers to their 2's complement and stores them in the uint. This would currently work as: $ tools/virsh vcpupin RHEL_nightly -4294967295 VCPU: CPU Affinity ---------------------------------- 1: 0-3 We need to modify the vshCommandOptUInt helper to reject negative numbers.
+ if (get_vcpu <= 0) { + /* In query mode, "vcpu" is optional */ + if (query && get_vcpu < 0) { + vshError(ctl, "%s", + _("vcpupin: Invalid vCPU number in query mode.")); + goto cleanup; + } + /* In pin mode, "vcpu" is necessary */ + if (!query) { + vshError(ctl, "%s", + _("vcpupin: Missing vCPU number in pin mode.")); + goto cleanup; + } }
if (virDomainGetInfo(dom, &info) != 0) { vshError(ctl, "%s", _("vcpupin: failed to get domain information.")); - virDomainFree(dom); - return false; + goto cleanup; }
if (vcpu >= info.nrVirtCpu) { vshError(ctl, "%s", _("vcpupin: Invalid vCPU number.")); - virDomainFree(dom); - return false; + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { + goto cleanup; }
cpumaplen = VIR_CPU_MAPLEN(maxcpu); @@ -5871,7 +5877,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); vshPrintExtra(ctl, "----------------------------------\n"); for (i = 0; i < ncpus; i++) { - if (vcpu != -1 && i != vcpu) + if (get_vcpu > 0 && i != vcpu)
This will be either 0 or 1 here, so the "> 0" part can be omitted.
continue;
vshPrint(ctl, "%4zu: ", i); @@ -5880,8 +5886,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!ret) break; } - } + VIR_FREE(cpumaps); goto cleanup; }
This patch results into breakage of the vcpupin test as outputs are changing. ~/libvirt/tests $ VIR_TEST_DEBUG=2 ./vcpupin --- exp 2014-05-29 13:17:40.170153617 +0200 +++ out 2014-05-29 13:17:40.166820284 +0200 @@ -1,2 +1,2 @@ -error: vcpupin: Invalid or missing vCPU number. +error: vcpupin: Missing vCPU number in pin mode. I'm going to post a v3 of this patch with the mistakes fixed and with the additional patch to fix vshCommandOptUInt. Also I'm going to improve the tests for all the cases I was able to break the command. Peter
On 05/29/2014 05:47 AM, Peter Krempa wrote:
Still not quite right ... vshCommandOptUInt currently wraps negative numbers to their 2's complement and stores them in the uint.
I recently tweaked virstring.c to provide virStrToLong_uip and friends for rejecting negative input when parsing unsigned numbers. The intent was that we need to make a case-by-case decision on which of the two parsing styles to use - sometimes, wrapping -1 to max is desirable, other times it is not.
We need to modify the vshCommandOptUInt helper to reject negative numbers.
It may mean that we need two flavors of vshCommandOptUInt. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/29/14 16:51, Eric Blake wrote:
On 05/29/2014 05:47 AM, Peter Krempa wrote:
Still not quite right ... vshCommandOptUInt currently wraps negative numbers to their 2's complement and stores them in the uint.
I recently tweaked virstring.c to provide virStrToLong_uip and friends for rejecting negative input when parsing unsigned numbers.
The intent was that we need to make a case-by-case decision on which of the two parsing styles to use - sometimes, wrapping -1 to max is desirable, other times it is not.
We need to modify the vshCommandOptUInt helper to reject negative numbers.
It may mean that we need two flavors of vshCommandOptUInt.
Actually we don't. All callers of vshCommandOptUInt don't expect to use the wrap-to-2's-complement "feature". I've sent a V3 that does exactly that.
participants (3)
-
Eric Blake -
Jincheng Miao -
Peter Krempa