| Message ID | 20230512021033.1378730-18-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix qemu_strtosz() read-out-of-bounds | expand |
On Thu, May 11, 2023 at 09:10:31PM -0500, Eric Blake wrote: > > Rather than open-coding two different ways to check for an unwanted > negative sign, reuse the same code in both functions. That way, if we > decide down the road to accept "-0" instead of rejecting it, we have > fewer places to change. Also, it means we now get ERANGE instead of > EINVAL for negative values in qemu_strtosz, which is reasonable for > what it represents. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 7 +++---- > util/cutils.c | 8 ++------ > 2 files changed, 5 insertions(+), 10 deletions(-) Returning ERANGE instead of EINVAL changes the expected output for iotests 49 and 178; this needs to be squashed in: diff --git i/tests/qemu-iotests/049.out w/tests/qemu-iotests/049.out index 8719c91b483..34e1b452e6e 100644 --- i/tests/qemu-iotests/049.out +++ w/tests/qemu-iotests/049.out @@ -92,13 +92,10 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off comp == 3. Invalid sizes == qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024 -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2 -qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64 -Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta- -and exabytes, respectively. +qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size' qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for diff --git i/tests/qemu-iotests/178.out.qcow2 w/tests/qemu-iotests/178.out.qcow2 index 0d51fe401ec..fe193fd5f4f 100644 --- i/tests/qemu-iotests/178.out.qcow2 +++ w/tests/qemu-iotests/178.out.qcow2 @@ -13,8 +13,7 @@ qemu-img: Invalid option list: , qemu-img: Invalid parameter 'snapshot.foo' qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' qemu-img: --output must be used with human or json as argument. -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. qemu-img: Unknown file format 'foo' == Size calculation for a new file (human) == diff --git i/tests/qemu-iotests/178.out.raw w/tests/qemu-iotests/178.out.raw index 116241ddef2..445e460fad9 100644 --- i/tests/qemu-iotests/178.out.raw +++ w/tests/qemu-iotests/178.out.raw @@ -13,8 +13,7 @@ qemu-img: Invalid option list: , qemu-img: Invalid parameter 'snapshot.foo' qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' qemu-img: --output must be used with human or json as argument. -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. qemu-img: Unknown file format 'foo' == Size calculation for a new file (human) ==
On 12.05.23 21:34, Eric Blake wrote: > On Thu, May 11, 2023 at 09:10:31PM -0500, Eric Blake wrote: >> Rather than open-coding two different ways to check for an unwanted >> negative sign, reuse the same code in both functions. That way, if we >> decide down the road to accept "-0" instead of rejecting it, we have >> fewer places to change. Also, it means we now get ERANGE instead of >> EINVAL for negative values in qemu_strtosz, which is reasonable for >> what it represents. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/unit/test-cutils.c | 7 +++---- >> util/cutils.c | 8 ++------ >> 2 files changed, 5 insertions(+), 10 deletions(-) > Returning ERANGE instead of EINVAL changes the expected output for > iotests 49 and 178; this needs to be squashed in: > > diff --git i/tests/qemu-iotests/049.out w/tests/qemu-iotests/049.out > index 8719c91b483..34e1b452e6e 100644 > --- i/tests/qemu-iotests/049.out > +++ w/tests/qemu-iotests/049.out > @@ -92,13 +92,10 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off comp > == 3. Invalid sizes == > > qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024 > -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for > -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. > +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. > > qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2 > -qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64 > -Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta- > -and exabytes, respectively. > +qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size' > > qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k > qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for > diff --git i/tests/qemu-iotests/178.out.qcow2 w/tests/qemu-iotests/178.out.qcow2 > index 0d51fe401ec..fe193fd5f4f 100644 > --- i/tests/qemu-iotests/178.out.qcow2 > +++ w/tests/qemu-iotests/178.out.qcow2 > @@ -13,8 +13,7 @@ qemu-img: Invalid option list: , > qemu-img: Invalid parameter 'snapshot.foo' > qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' > qemu-img: --output must be used with human or json as argument. > -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for > -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. > +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. > qemu-img: Unknown file format 'foo' > > == Size calculation for a new file (human) == > diff --git i/tests/qemu-iotests/178.out.raw w/tests/qemu-iotests/178.out.raw > index 116241ddef2..445e460fad9 100644 > --- i/tests/qemu-iotests/178.out.raw > +++ w/tests/qemu-iotests/178.out.raw > @@ -13,8 +13,7 @@ qemu-img: Invalid option list: , > qemu-img: Invalid parameter 'snapshot.foo' > qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' > qemu-img: --output must be used with human or json as argument. > -qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for > -qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. > +qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. > qemu-img: Unknown file format 'foo' > > == Size calculation for a new file (human) == With that squashed in: Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c index 1272638582a..b8ad4d7fbac 100644 --- a/tests/unit/test-cutils.c +++ b/tests/unit/test-cutils.c @@ -3396,10 +3396,9 @@ static void test_qemu_strtosz_trailing(void) static void test_qemu_strtosz_erange(void) { /* FIXME negative values fit better as ERANGE */ - do_strtosz(" -0", -EINVAL /* FIXME -ERANGE */, 0, 0 /* FIXME 3 */); - do_strtosz("-1", -EINVAL /* FIXME -ERANGE */, 0, 0 /* FIXME 2 */); - do_strtosz_full("-2M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */, 0, - 0 /* FIXME 2 */, -EINVAL, 0); + do_strtosz(" -0", -ERANGE, 0, 3); + do_strtosz("-1", -ERANGE, 0, 2); + do_strtosz_full("-2M", qemu_strtosz, -ERANGE, 0, 2, -EINVAL, 0); do_strtosz(" -.0", -EINVAL /* FIXME -ERANGE */, 0, 0 /* FIXME 4 */); do_strtosz_full("-.1k", qemu_strtosz, -EINVAL /* FIXME -ERANGE */, 0, 0 /* FIXME 3 */, -EINVAL, 0); diff --git a/util/cutils.c b/util/cutils.c index b5a6641fa0f..550abbe5c06 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -201,6 +201,7 @@ static int64_t suffix_mul(char suffix, int64_t unit) * - hex with scaling suffix, such as 0x20M * - octal, such as 08 * - fractional hex, such as 0x1.8 + * - negative values, including -0 * - floating point exponents, such as 1e3 * * The end pointer will be returned in *end, if not NULL. If there is @@ -226,15 +227,10 @@ static int do_strtosz(const char *nptr, const char **end, int64_t mul; /* Parse integral portion as decimal. */ - retval = qemu_strtou64(nptr, &endptr, 10, &val); + retval = parse_uint(nptr, &endptr, 10, &val); if (retval) { goto out; } - if (memchr(nptr, '-', endptr - nptr) != NULL) { - endptr = nptr; - retval = -EINVAL; - goto out; - } if (val == 0 && (*endptr == 'x' || *endptr == 'X')) { /* Input looks like hex; reparse, and insist on no fraction or suffix. */ retval = qemu_strtou64(nptr, &endptr, 16, &val);
Rather than open-coding two different ways to check for an unwanted negative sign, reuse the same code in both functions. That way, if we decide down the road to accept "-0" instead of rejecting it, we have fewer places to change. Also, it means we now get ERANGE instead of EINVAL for negative values in qemu_strtosz, which is reasonable for what it represents. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/unit/test-cutils.c | 7 +++---- util/cutils.c | 8 ++------ 2 files changed, 5 insertions(+), 10 deletions(-)