[libvirt] [PATCHv4 0/4] virsh: Clean unsigned value parsing
Jincheng Miao (1): virsh: forbid negative vcpu argument to vcpupin Peter Krempa (3): virsh: Reject negative numbers in vshCommandOptUInt virsh: Reject negative numbers in vshCommandOptUL virsh: Reject negative numbers in vshCommandOptULongLong tests/vcpupin | 29 ++++++++++- tools/virsh-domain.c | 39 ++++++++------- tools/virsh-volume.c | 8 +-- tools/virsh.c | 137 +++++++++++++++++++++++++++++++++++++++++---------- tools/virsh.h | 9 ++++ 5 files changed, 172 insertions(+), 50 deletions(-) -- 1.9.3
Use virStrToLong_uip instead of virStrToLong_ui to reject negative numbers in the helper. None of the callers expects the wraparound "feature" for negative numbers. Also add a function that allows wrapping of negative numbers as it might be used in the future and be explicit about the new semantics in the function docs. --- tools/virsh.c | 48 ++++++++++++++++++++++++++++++++++++++---------- tools/virsh.h | 3 +++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 15f3025..cbab6b0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1494,6 +1494,28 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) return 1; } +static int +vshCommandOptUIntInternal(const vshCmd *cmd, + const char *name, + unsigned int *value, + bool wrap) +{ + vshCmdOpt *arg; + int ret; + + if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) + return ret; + + if (wrap) { + if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) + return -1; + } else { + if (virStrToLong_uip(arg->data, NULL, 10, value) < 0) + return -1; + } + + return 1; +} /** * vshCommandOptUInt: @@ -1501,22 +1523,28 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) * @name option name * @value result * - * Convert option to unsigned int + * Convert option to unsigned int, reject negative numbers * See vshCommandOptInt() */ int vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) { - vshCmdOpt *arg; - int ret; - - ret = vshCommandOpt(cmd, name, &arg, true); - if (ret <= 0) - return ret; + return vshCommandOptUIntInternal(cmd, name, value, false); +} - if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) - return -1; - return 1; +/** + * vshCommandOptUIntWrap: + * @cmd command reference + * @name option name + * @value result + * + * Convert option to unsigned int, wraps negative numbers to positive + * See vshCommandOptInt() + */ +int +vshCommandOptUIntWrap(const vshCmd *cmd, const char *name, unsigned int *value) +{ + return vshCommandOptUIntInternal(cmd, name, value, true); } diff --git a/tools/virsh.h b/tools/virsh.h index 1ffc003..9afbbb5 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -285,6 +285,9 @@ int vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) int vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int vshCommandOptUIntWrap(const vshCmd *cmd, const char *name, + unsigned int *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -- 1.9.3
To follow the new semantics of the vshCommandOptToU* functions convert this one to reject negative numbers too. To allow using -1 for "maximum" semantics for the two bandwidth functions that use this helper introduce vshCommandOptULWrap. Although currently the migrate-setspeed function for the qemu driver will reject -1 as maximum. --- tools/virsh-domain.c | 4 ++-- tools/virsh.c | 46 +++++++++++++++++++++++++++++++++++++--------- tools/virsh.h | 3 +++ 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..d76865b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1457,7 +1457,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) { + if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { vshError(ctl, "%s", _("bandwidth must be a number")); goto cleanup; } @@ -9223,7 +9223,7 @@ cmdMigrateSetMaxSpeed(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) { + if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { vshError(ctl, "%s", _("migrate: Invalid bandwidth")); goto done; } diff --git a/tools/virsh.c b/tools/virsh.c index cbab6b0..77a4f99 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1547,6 +1547,28 @@ vshCommandOptUIntWrap(const vshCmd *cmd, const char *name, unsigned int *value) return vshCommandOptUIntInternal(cmd, name, value, true); } +static int +vshCommandOptULInternal(const vshCmd *cmd, + const char *name, + unsigned long *value, + bool wrap) +{ + vshCmdOpt *arg; + int ret; + + if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) + return ret; + + if (wrap) { + if (virStrToLong_ul(arg->data, NULL, 10, value) < 0) + return -1; + } else { + if (virStrToLong_ulp(arg->data, NULL, 10, value) < 0) + return -1; + } + + return 1; +} /* * vshCommandOptUL: @@ -1560,16 +1582,22 @@ vshCommandOptUIntWrap(const vshCmd *cmd, const char *name, unsigned int *value) int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) { - vshCmdOpt *arg; - int ret; - - ret = vshCommandOpt(cmd, name, &arg, true); - if (ret <= 0) - return ret; + return vshCommandOptULInternal(cmd, name, value, false); +} - if (virStrToLong_ul(arg->data, NULL, 10, value) < 0) - return -1; - return 1; +/** + * vshCommandOptULWrap: + * @cmd command reference + * @name option name + * @value result + * + * Convert option to unsigned long, wraps negative numbers to positive + * See vshCommandOptInt() + */ +int +vshCommandOptULWrap(const vshCmd *cmd, const char *name, unsigned long *value) +{ + return vshCommandOptULInternal(cmd, name, value, true); } /** diff --git a/tools/virsh.h b/tools/virsh.h index 9afbbb5..4f5c336 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -291,6 +291,9 @@ int vshCommandOptUIntWrap(const vshCmd *cmd, const char *name, int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int vshCommandOptULWrap(const vshCmd *cmd, const char *name, + unsigned long *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; int vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -- 1.9.3
To follow the new semantics of the vshCommandOptToU* functions convert this one to reject negative numbers too. To allow using -1 for "maximum" semantics for the vol-*load two bandwidth functions that use this helper introduce vshCommandOptULongLongWrap. --- tools/virsh-volume.c | 8 ++++---- tools/virsh.c | 51 ++++++++++++++++++++++++++++++++++++++++----------- tools/virsh.h | 3 +++ 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index f284fa5..724a86b 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -677,12 +677,12 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; unsigned long long offset = 0, length = 0; - if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) { + if (vshCommandOptULongLongWrap(cmd, "offset", &offset) < 0) { vshError(ctl, _("Unable to parse integer")); return false; } - if (vshCommandOptULongLong(cmd, "length", &length) < 0) { + if (vshCommandOptULongLongWrap(cmd, "length", &length) < 0) { vshError(ctl, _("Unable to parse integer")); return false; } @@ -787,12 +787,12 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; bool created = false; - if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) { + if (vshCommandOptULongLongWrap(cmd, "offset", &offset) < 0) { vshError(ctl, _("Unable to parse integer")); return false; } - if (vshCommandOptULongLong(cmd, "length", &length) < 0) { + if (vshCommandOptULongLongWrap(cmd, "length", &length) < 0) { vshError(ctl, _("Unable to parse integer")); return false; } diff --git a/tools/virsh.c b/tools/virsh.c index 77a4f99..82669b0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1699,31 +1699,60 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, return 1; } +static int +vshCommandOptULongLongInternal(const vshCmd *cmd, + const char *name, + unsigned long long *value, + bool wrap) +{ + vshCmdOpt *arg; + int ret; + + if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) + return ret; + + if (wrap) { + if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) + return -1; + } else { + if (virStrToLong_ullp(arg->data, NULL, 10, value) < 0) + return -1; + } + + return 1; +} + /** * vshCommandOptULongLong: * @cmd command reference * @name option name * @value result * - * Returns option as long long + * Returns option as long long, rejects negative numbers * See vshCommandOptInt() */ int vshCommandOptULongLong(const vshCmd *cmd, const char *name, unsigned long long *value) { - vshCmdOpt *arg; - int ret; - - ret = vshCommandOpt(cmd, name, &arg, true); - if (ret <= 0) - return ret; - - if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) - return -1; - return 1; + return vshCommandOptULongLongInternal(cmd, name, value, false); } +/** + * vshCommandOptULongLongWrap: + * @cmd command reference + * @name option name + * @value result + * + * Returns option as long long, wraps negative numbers to positive + * See vshCommandOptInt() + */ +int +vshCommandOptULongLongWrap(const vshCmd *cmd, const char *name, + unsigned long long *value) +{ + return vshCommandOptULongLongInternal(cmd, name, value, true); +} /** * vshCommandOptScaledInt: diff --git a/tools/virsh.h b/tools/virsh.h index 4f5c336..7656407 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -307,6 +307,9 @@ int vshCommandOptLongLong(const vshCmd *cmd, const char *name, int vshCommandOptULongLong(const vshCmd *cmd, const char *name, unsigned long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int vshCommandOptULongLongWrap(const vshCmd *cmd, const char *name, + unsigned long long *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; int vshCommandOptScaledInt(const vshCmd *cmd, const char *name, unsigned long long *value, int scale, unsigned long long max) -- 1.9.3
From: Jincheng Miao <jmiao@redhat.com> The vcpupin command allowed specifying a negative number for the --vcpu argument. This would the overflow when the underlying virDomainPinVcpu API was called. $ virsh vcpupin r7 -1 0 error: numerical overflow: input too large: 4294967295 Switch the vCPU variable to a unsigned int and parse it using the corresponding function. Also improve the vcpupin test to cover all the defects. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1101059 Signed-off-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/vcpupin | 29 ++++++++++++++++++++++++++++- tools/virsh-domain.c | 35 ++++++++++++++++++----------------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/tests/vcpupin b/tests/vcpupin index f1fb038..9f34ec0 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -34,7 +34,7 @@ fail=0 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: Invalid or missing vCPU number. +error: vcpupin: Invalid vCPU number. EOF compare exp out || fail=1 @@ -43,9 +43,36 @@ compare exp out || fail=1 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 +error: vcpupin: vCPU index out of range. + +EOF +compare exp out || fail=1 + +# Negative number +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test -100 0,1 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 error: vcpupin: Invalid vCPU number. EOF compare exp out || fail=1 +# missing argument +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test --cpulist 0,1 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: vcpupin: Missing vCPU number in pin mode. + +EOF +compare exp out || fail=1 + +# without arguments. This should succeed but the backend function in the +# test driver isn't implemented +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: this function is not supported by the connection driver: virDomainGetVcpuPinInfo + +EOF +compare exp out || fail=1 (exit $fail); exit $fail diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d76865b..e43c380 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 = 0; 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 got_vcpu; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -5830,29 +5831,29 @@ 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 ((got_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu)) < 0) { + vshError(ctl, "%s", _("vcpupin: Invalid vCPU number.")); + goto cleanup; } - if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { - virDomainFree(dom); - return false; + /* In pin mode, "vcpu" is necessary */ + if (!query && got_vcpu == 0) { + 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; + vshError(ctl, "%s", _("vcpupin: vCPU index out of range.")); + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { + goto cleanup; } cpumaplen = VIR_CPU_MAPLEN(maxcpu); @@ -5871,7 +5872,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 (got_vcpu && i != vcpu) continue; vshPrint(ctl, "%4zu: ", i); @@ -5880,8 +5881,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!ret) break; } - } + VIR_FREE(cpumaps); goto cleanup; } -- 1.9.3
On 06/04/2014 11:43 AM, Peter Krempa wrote:
Jincheng Miao (1): virsh: forbid negative vcpu argument to vcpupin
Peter Krempa (3): virsh: Reject negative numbers in vshCommandOptUInt virsh: Reject negative numbers in vshCommandOptUL virsh: Reject negative numbers in vshCommandOptULongLong
tests/vcpupin | 29 ++++++++++- tools/virsh-domain.c | 39 ++++++++------- tools/virsh-volume.c | 8 +-- tools/virsh.c | 137 +++++++++++++++++++++++++++++++++++++++++---------- tools/virsh.h | 9 ++++ 5 files changed, 172 insertions(+), 50 deletions(-)
ACK
On 06/12/14 14:05, Ján Tomko wrote:
On 06/04/2014 11:43 AM, Peter Krempa wrote:
Jincheng Miao (1): virsh: forbid negative vcpu argument to vcpupin
Peter Krempa (3): virsh: Reject negative numbers in vshCommandOptUInt virsh: Reject negative numbers in vshCommandOptUL virsh: Reject negative numbers in vshCommandOptULongLong
tests/vcpupin | 29 ++++++++++- tools/virsh-domain.c | 39 ++++++++------- tools/virsh-volume.c | 8 +-- tools/virsh.c | 137 +++++++++++++++++++++++++++++++++++++++++---------- tools/virsh.h | 9 ++++ 5 files changed, 172 insertions(+), 50 deletions(-)
ACK
Series pushed; Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa