jes.soren...@redhat.com writes:
> From: Jes Sorensen <jes.soren...@redhat.com>
>
> strtosz() returns -1 on error.
>
> v2 renamed from strtobytes() to strtosz() as suggested by Markus.
>
> Signed-off-by: Jes Sorensen <jes.soren...@redhat.com>
> ---
> cutils.c | 39 +++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 1 +
> vl.c | 31 ++++++++++---------------------
> 3 files changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..ee591c5 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -283,3 +283,42 @@ int fcntl_setfl(int fd, int flag)
> }
> #endif
>
> +/*
> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
> + * G/g for GB or T/t for TB. Default without any postfix is MB.
> + * End pointer will be returned in *end, if end is valid.
> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> + int64_t value;
long long, please, because that's what strtoll() returns.
> + char *endptr;
> +
> + value = strtoll(nptr, &endptr, 0);
> + switch (*endptr++) {
> + case 'K':
> + case 'k':
> + value <<= 10;
> + break;
> + case 0:
> + case 'M':
> + case 'm':
> + value <<= 20;
> + break;
> + case 'G':
> + case 'g':
> + value <<= 30;
> + break;
> + case 'T':
> + case 't':
> + value <<= 40;
> + break;
> + default:
> + value = -1;
> + }
> +
> + if (end)
> + *end = endptr;
> +
> + return value;
Casts value to ssize_t, which might truncate.
> +}
Sloppy use of strtoll().
Both tolerable as long as the patch doesn't make things worse. Let's
see:
> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
> int qemu_fls(int i);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>
> /* path.c */
> void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
> if (get_param_value(option, 128, "mem", optarg) == 0) {
> node_mem[nodenr] = 0;
> } else {
> - value = strtoull(option, &endptr, 0);
> - switch (*endptr) {
> - case 0: case 'M': case 'm':
> - value <<= 20;
> - break;
> - case 'G': case 'g':
> - value <<= 30;
> - break;
> + ssize_t sval;
> + sval = strtosz(option, NULL);
> + if (sval < 0) {
> + fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> + exit(1);
Before After
Invalid number silently interpreted as zero no change
Overflow silently capped to ULLONG_MAX LLONG_MAX, then
trunc ssize_t
Invalid size suffix silently ignored rejected
> }
> - node_mem[nodenr] = value;
> + node_mem[nodenr] = sval;
> }
> if (get_param_value(option, 128, "cpus", optarg) == 0) {
> node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
> exit(0);
> break;
> case QEMU_OPTION_m: {
> - uint64_t value;
> - char *ptr;
> + ssize_t value;
>
> - value = strtoul(optarg, &ptr, 10);
> - switch (*ptr) {
> - case 0: case 'M': case 'm':
> - value <<= 20;
> - break;
> - case 'G': case 'g':
> - value <<= 30;
> - break;
> - default:
> + value = strtosz(optarg, NULL);
> + if (value < 0) {
> fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
> exit(1);
> }
Before After
Invalid number silently interpreted as zero no change
Overflow silently capped to ULLONG_MAX LLONG_MAX, then
trunc ssize_t
Invalid size suffix rejected no change
A bit more context:
/* On 32-bit hosts, QEMU is limited by virtual address space
*/
if (value > (2047 << 20) && HOST_LONG_BITS == 32) {
fprintf(stderr, "qemu: at most 2047 MB RAM can be
simulated\n");
exit(1);
}
if (value != (uint64_t)(ram_addr_t)value) {
fprintf(stderr, "qemu: ram size too large\n");
exit(1);
}
ram_size = value;
break;
I'm afraid you break both conditionals for 32 bit hosts.
On such hosts, ssize_t is 32 bits wide. strtosz() parses 64 bits
internally, but truncates to 32 bits silently.
The old code reliably rejects values larger than 2047MiB. Your
truncation can change a value exceeding the limit to one within the
limit. First conditional becomes unreliable.
The second conditional becomes useless: it sign-extends a non-negative
32 bit integer value to 64 bit, then truncates back, and checks the
value stays the same. It trivially does.
I strongly recommend to make strtosz() sane from the start, not in a
later patch: proper error checking, including proper handling of
overflow.
Perhaps squashing 1-3/7 would get us there, or at least closer.
