From: Eric Blake <eblake@redhat.com>
To: qemu-devel@nongnu.org
Cc: Hanna Czenczek <hreitz@redhat.com>
Subject: [PULL 17/21] cutils: Set value in all qemu_strtosz* error paths
Date: Thu, 1 Jun 2023 17:03:01 -0500 [thread overview]
Message-ID: <20230601220305.2130121-18-eblake@redhat.com> (raw)
In-Reply-To: <20230601220305.2130121-1-eblake@redhat.com>
Making callers determine whether or not *value was populated on error
is not nice for usability. Pre-patch, we have unit tests that check
that *result is left unchanged on most EINVAL errors and set to 0 on
many ERANGE errors. This is subtly different from libc strtoumax()
behavior which returns UINT64_MAX on ERANGE errors, as well as
different from our parse_uint() which slams to 0 on EINVAL on the
grounds that we want our functions to be harder to mis-use than
strtoumax().
Let's audit callers:
- hw/core/numa.c:parse_numa() fixed in the previous patch to check for
errors
- migration/migration-hmp-cmds.c:hmp_migrate_set_parameter(),
monitor/hmp.c:monitor_parse_arguments(),
qapi/opts-visitor.c:opts_type_size(),
qapi/qobject-input-visitor.c:qobject_input_type_size_keyval(),
qemu-img.c:cvtnum_full(), qemu-io-cmds.c:cvtnum(),
target/i386/cpu.c:x86_cpu_parse_featurestr(), and
util/qemu-option.c:parse_option_size() appear to reject all failures
(although some with distinct messages for ERANGE as opposed to
EINVAL), so it doesn't matter what is in the value parameter on
error.
- All remaining callers are in the testsuite, where we can tweak our
expectations to match our new desired behavior.
Advancing to the end of the string parsed on overflow (ERANGE), while
still returning 0, makes sense (UINT64_MAX as a size is unlikely to be
useful); likewise, our size parsing code is complex enough that it's
easier to always return 0 when endptr is NULL but trailing garbage was
found, rather than trying to return the value of the prefix actually
parsed (no current caller cared about the value of the prefix).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Message-Id: <20230522190441.64278-16-eblake@redhat.com>
---
tests/unit/test-cutils.c | 106 +++++++++++++++++++--------------------
util/cutils.c | 17 +++++--
2 files changed, 63 insertions(+), 60 deletions(-)
diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index a629ef2ea39..2189ebc92f3 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -3396,7 +3396,7 @@ static void test_qemu_strtosz_float(void)
do_strtosz("1.k", 0, 1024, 3);
/* FIXME An empty fraction head should be tolerated */
- do_strtosz(" .5k", -EINVAL /* FIXME 0 */, 0xbaadf00d /* FIXME 512 */,
+ do_strtosz(" .5k", -EINVAL /* FIXME 0 */, 0 /* FIXME 512 */,
0 /* FIXME 4 */);
/* For convenience, we permit values that are not byte-exact */
@@ -3421,29 +3421,29 @@ static void test_qemu_strtosz_float(void)
static void test_qemu_strtosz_invalid(void)
{
- do_strtosz(NULL, -EINVAL, 0xbaadf00d, 0);
+ do_strtosz(NULL, -EINVAL, 0, 0);
/* Must parse at least one digit */
- do_strtosz("", -EINVAL, 0xbaadf00d, 0);
- do_strtosz(" \t ", -EINVAL, 0xbaadf00d, 0);
- do_strtosz(".", -EINVAL, 0xbaadf00d, 0);
- do_strtosz(" .", -EINVAL, 0xbaadf00d, 0);
- do_strtosz(" .k", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("inf", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("NaN", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("", -EINVAL, 0, 0);
+ do_strtosz(" \t ", -EINVAL, 0, 0);
+ do_strtosz(".", -EINVAL, 0, 0);
+ do_strtosz(" .", -EINVAL, 0, 0);
+ do_strtosz(" .k", -EINVAL, 0, 0);
+ do_strtosz("inf", -EINVAL, 0, 0);
+ do_strtosz("NaN", -EINVAL, 0, 0);
/* Lone suffix is not okay */
- do_strtosz("k", -EINVAL, 0xbaadf00d, 0);
- do_strtosz(" M", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("k", -EINVAL, 0, 0);
+ do_strtosz(" M", -EINVAL, 0, 0);
/* Fractional values require scale larger than bytes */
- do_strtosz("1.1B", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("1.1", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("1.1B", -EINVAL, 0, 0);
+ do_strtosz("1.1", -EINVAL, 0, 0);
/* FIXME underflow in the fraction tail should matter for 'B' */
- do_strtosz("1.00001B", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("1.00001B", -EINVAL, 0, 0);
do_strtosz("1.00000000000000000001B", 0 /* FIXME -EINVAL */,
- 1 /* FIXME 0xbaadf00d */, 23 /* FIXME 0 */);
+ 1 /* FIXME 0 */, 23 /* FIXME 0 */);
do_strtosz("1."
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
@@ -3452,62 +3452,60 @@ static void test_qemu_strtosz_invalid(void)
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
- "1B", 0 /* FIXME -EINVAL */, 1 /* FIXME 0xbaadf00d */,
+ "1B", 0 /* FIXME -EINVAL */, 1 /* FIXME 0 */,
354 /* FIXME 0 */);
/* No hex fractions */
- do_strtosz("0x1.8k", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("0x1.k", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("0x1.8k", -EINVAL, 0, 0);
+ do_strtosz("0x1.k", -EINVAL, 0, 0);
/* No hex suffixes */
- do_strtosz("0x18M", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("0x1p1", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("0x18M", -EINVAL, 0, 0);
+ do_strtosz("0x1p1", -EINVAL, 0, 0);
/* decimal in place of scaling suffix */
- do_strtosz("1.1.k", -EINVAL, 0xbaadf00d, 0);
- do_strtosz("1.1.", -EINVAL, 0xbaadf00d, 0);
+ do_strtosz("1.1.k", -EINVAL, 0, 0);
+ do_strtosz("1.1.", -EINVAL, 0, 0);
}
static void test_qemu_strtosz_trailing(void)
{
/* Trailing whitespace */
- do_strtosz_full("1k ", qemu_strtosz, 0, 1024, 2, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("1k ", qemu_strtosz, 0, 1024, 2, -EINVAL, 0);
/* Unknown suffix overrides even implied scale*/
- do_strtosz_full("123xxx", qemu_strtosz, 0, 123, 3, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("123xxx", qemu_strtosz, 0, 123, 3, -EINVAL, 0);
/* Implied scale allows partial parse */
- do_strtosz_full("123xxx", qemu_strtosz_MiB, 0, 123 * MiB, 3,
- -EINVAL, 0xbaadf00d);
- do_strtosz_full("1.5.k", qemu_strtosz_MiB, 0, 1.5 * MiB, 3,
- -EINVAL, 0xbaadf00d);
+ do_strtosz_full("123xxx", qemu_strtosz_MiB, 0, 123 * MiB, 3, -EINVAL, 0);
+ do_strtosz_full("1.5.k", qemu_strtosz_MiB, 0, 1.5 * MiB, 3, -EINVAL, 0);
/* Junk after one-byte suffix */
- do_strtosz_full("1kiB", qemu_strtosz, 0, 1024, 2, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("1kiB", qemu_strtosz, 0, 1024, 2, -EINVAL, 0);
/* Incomplete hex is an unknown suffix */
- do_strtosz_full("0x", qemu_strtosz, 0, 0, 1, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("0x", qemu_strtosz, 0, 0, 1, -EINVAL, 0);
/* Hex literals use only one leading zero */
- do_strtosz_full("00x1", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("00x1", qemu_strtosz, 0, 0, 2, -EINVAL, 0);
/* No support for binary literals; 'b' is valid suffix */
- do_strtosz_full("0b1000", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("0b1000", qemu_strtosz, 0, 0, 2, -EINVAL, 0);
/* Junk after decimal */
- do_strtosz_full("0.NaN", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("0.NaN", qemu_strtosz, 0, 0, 2, -EINVAL, 0);
/* Although negatives are invalid, '-' may be in trailing junk */
- do_strtosz_full("123-45", qemu_strtosz, 0, 123, 3, -EINVAL, 0xbaadf00d);
- do_strtosz_full(" 123 - 45", qemu_strtosz, 0, 123, 4, -EINVAL, 0xbaadf00d);
+ do_strtosz_full("123-45", qemu_strtosz, 0, 123, 3, -EINVAL, 0);
+ do_strtosz_full(" 123 - 45", qemu_strtosz, 0, 123, 4, -EINVAL, 0);
/* FIXME should stop parse after 'e'. No floating point exponents */
do_strtosz_full("1.5e1k", qemu_strtosz, -EINVAL /* FIXME 0 */,
- 0xbaadf00d /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
- -EINVAL, 0xbaadf00d);
+ 0 /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
+ -EINVAL, 0);
do_strtosz_full("1.5E+0k", qemu_strtosz, -EINVAL /* FIXME 0 */,
- 0xbaadf00d /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
- -EINVAL, 0xbaadf00d);
+ 0 /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
+ -EINVAL, 0);
/*
* FIXME overflow in fraction is so buggy it can read beyond bounds
@@ -3515,20 +3513,19 @@ static void test_qemu_strtosz_trailing(void)
*/
do_strtosz_full("1.5E999\0\0" /* FIXME 1.5E999" */, qemu_strtosz,
0, 1 /* FIXME EiB * 1.5 */, 8 /* FIXME 4 */,
- 0 /* FIXME -EINVAL */, 1 /* FIXME 0xbaadf00d */);
+ 0 /* FIXME -EINVAL */, 1 /* FIXME 0 */);
}
static void test_qemu_strtosz_erange(void)
{
/* FIXME negative values fit better as ERANGE */
- do_strtosz(" -0", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d, 0 /* FIXME 3 */);
- do_strtosz("-1", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d, 0 /* FIXME 2 */);
- do_strtosz_full("-2M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
- 0xbaadf00d, 0 /* FIXME 2 */, -EINVAL, 0xbaadf00d);
- do_strtosz(" -.0", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d,
- 0 /* FIXME 4 */);
- do_strtosz_full("-.1k", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
- 0xbaadf00d, 0 /* FIXME 3 */, -EINVAL, 0xbaadf00d);
+ 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", -EINVAL /* FIXME -ERANGE */, 0, 0 /* FIXME 4 */);
+ do_strtosz_full("-.1k", qemu_strtosz, -EINVAL /* FIXME -ERANGE */, 0,
+ 0 /* FIXME 3 */, -EINVAL, 0);
do_strtosz_full(" -."
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
@@ -3537,21 +3534,20 @@ static void test_qemu_strtosz_erange(void)
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
"00000000000000000000000000000000000000000000000000"
- "1M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
- 0xbaadf00d, 0 /* FIXME 354 */, -EINVAL, 0xbaadf00d);
+ "1M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */, 0,
+ 0 /* FIXME 354 */, -EINVAL, 0);
/* 2^64; see strtosz_simple for 2^64-1 */
- do_strtosz("18446744073709551616", -ERANGE, 0xbaadf00d, 20);
+ do_strtosz("18446744073709551616", -ERANGE, 0, 20);
- do_strtosz("20E", -ERANGE, 0xbaadf00d, 3);
+ do_strtosz("20E", -ERANGE, 0, 3);
/* FIXME Fraction tail can cause ERANGE overflow */
do_strtosz("15.9999999999999999999999999999999999999999999999999999E",
- 0 /* FIXME -ERANGE */, 15ULL * EiB /* FIXME 0xbaadf00d */, 56);
+ 0 /* FIXME -ERANGE */, 15ULL * EiB /* FIXME 0 */, 56);
/* EINVAL has priority over ERANGE */
- do_strtosz_full("100000Pjunk", qemu_strtosz, -ERANGE, 0xbaadf00d, 7,
- -EINVAL, 0xbaadf00d);
+ do_strtosz_full("100000Pjunk", qemu_strtosz, -ERANGE, 0, 7, -EINVAL, 0);
}
static void test_qemu_strtosz_metric(void)
diff --git a/util/cutils.c b/util/cutils.c
index 1dc67d201dc..c5530a5c2be 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -205,13 +205,15 @@ static int64_t suffix_mul(char suffix, int64_t unit)
*
* The end pointer will be returned in *end, if not NULL. If there is
* no fraction, the input can be decimal or hexadecimal; if there is a
- * fraction, then the input must be decimal and there must be a suffix
- * (possibly by @default_suffix) larger than Byte, and the fractional
- * portion may suffer from precision loss or rounding. The input must
- * be positive.
+ * non-zero fraction, then the input must be decimal and there must be
+ * a suffix (possibly by @default_suffix) larger than Byte, and the
+ * fractional portion may suffer from precision loss or rounding. The
+ * input must be positive.
*
* Return -ERANGE on overflow (with *@end advanced), and -EINVAL on
- * other error (with *@end left unchanged).
+ * other error (with *@end at @nptr). Unlike strtoull, *@result is
+ * set to 0 on all errors, as returning UINT64_MAX on overflow is less
+ * likely to be usable as a size.
*/
static int do_strtosz(const char *nptr, const char **end,
const char default_suffix, int64_t unit,
@@ -311,6 +313,11 @@ out:
}
if (retval == 0) {
*result = val;
+ } else {
+ *result = 0;
+ if (end && retval == -EINVAL) {
+ *end = nptr;
+ }
}
return retval;
--
2.40.1
next prev parent reply other threads:[~2023-06-01 22:04 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-01 22:02 [PULL 00/21] NBD and miscellaneous patches for 2023-06-01 Eric Blake
2023-06-01 22:02 ` [PULL 01/21] iotests: Fix test 104 under NBD Eric Blake
2023-06-01 22:02 ` [PULL 02/21] qcow2: Explicit mention of padding bytes Eric Blake
2023-06-01 22:02 ` [PULL 03/21] test-cutils: Avoid g_assert in unit tests Eric Blake
2023-06-01 22:02 ` [PULL 04/21] test-cutils: Use g_assert_cmpuint where appropriate Eric Blake
2023-06-01 22:02 ` [PULL 05/21] test-cutils: Test integral qemu_strto* value on failures Eric Blake
2023-06-01 22:02 ` [PULL 06/21] test-cutils: Test more integer corner cases Eric Blake
2023-06-02 12:34 ` Eric Blake
2023-06-01 22:02 ` [PULL 07/21] cutils: Fix wraparound parsing in qemu_strtoui Eric Blake
2023-06-03 8:17 ` Michael Tokarev
2023-06-05 13:32 ` Eric Blake
2023-06-01 22:02 ` [PULL 08/21] cutils: Document differences between parse_uint and qemu_strtou64 Eric Blake
2023-06-01 22:02 ` [PULL 09/21] cutils: Adjust signature of parse_uint[_full] Eric Blake
2023-06-02 6:16 ` Markus Armbruster
2023-06-02 12:22 ` Eric Blake
2023-06-01 22:02 ` [PULL 10/21] cutils: Allow NULL endptr in parse_uint() Eric Blake
2023-06-01 22:02 ` [PULL 11/21] test-cutils: Add coverage of qemu_strtod Eric Blake
2023-06-01 22:02 ` [PULL 12/21] test-cutils: Prepare for upcoming semantic change in qemu_strtosz Eric Blake
2023-06-01 22:02 ` [PULL 13/21] test-cutils: Refactor qemu_strtosz tests for less boilerplate Eric Blake
2023-06-01 22:02 ` [PULL 14/21] cutils: Allow NULL str in qemu_strtosz Eric Blake
2023-06-01 22:02 ` [PULL 15/21] numa: Check for qemu_strtosz_MiB error Eric Blake
2023-06-01 22:03 ` [PULL 16/21] test-cutils: Add more coverage to qemu_strtosz Eric Blake
2023-06-01 22:03 ` Eric Blake [this message]
2023-06-01 22:03 ` [PULL 18/21] cutils: Set value in all integral qemu_strto* error paths Eric Blake
2023-06-01 22:03 ` [PULL 19/21] cutils: Use parse_uint in qemu_strtosz for negative rejection Eric Blake
2023-06-01 22:03 ` [PULL 20/21] cutils: Improve qemu_strtod* error paths Eric Blake
2023-06-01 22:03 ` [PULL 21/21] cutils: Improve qemu_strtosz handling of fractions Eric Blake
2023-06-02 3:58 ` [PULL 00/21] NBD and miscellaneous patches for 2023-06-01 Richard Henderson
2023-06-02 12:27 ` Eric Blake
2023-06-02 6:32 ` Conclusion of yet another expensive UI folly (was: [PULL 00/21] NBD and miscellaneous patches for 2023-06-01) Markus Armbruster
2023-06-02 12:29 ` Eric Blake
2023-06-02 13:02 ` Conclusion of yet another expensive UI folly Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230601220305.2130121-18-eblake@redhat.com \
--to=eblake@redhat.com \
--cc=hreitz@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.