Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count.

[Jes Sorensen]

On 10/11/10 10:51, Markus Armbruster wrote:
> address@hidden writes:
>> +/*
>> + * 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.

I merged patches 1-3 as you suggested, so comments based on the combined
patch.

This is longer an issue as it is switched to strtod().

>> +    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.

The new patch does:

    int64_t tmpval;
    tmpval = (val * mul);
    if (tmpval >= ~(size_t)0)
        goto fail;

so anything that is out of bounds is checked and caught before returning
a possibly truncated valued.

> Sloppy use of strtoll().    tmpval = (val * mul);
    if (tmpval >= ~(size_t)0)
        goto fail;
> 
> 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

What do you mean by invalid number here?

LLONG_MAX seems a reasonable limit, ie. 31 bit on 32 bit hosts or 63
bits on 64 bit hosts. strtosz() is meant to return a size, so IMHO thats
a fair limitation. We could use size_t instead of ssize_t but it would
require ugly casts in any function calling the function to test for error.

>                         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.

I believe the combined patch is taking care of this fine with the
>= ~(size_t)0 comparison. If the value is above that, it returns an
error. For 32 bit hosts that means we should be able to specify 2047MB
RAM fine.

The only place where I see the latter being a potential problem is on
P64 systems such as win64, since ram_addr_t is defined to be unsigned
long, but afaik we don't support win64 anyway. On both 32 bit and LP64
systems sizeof(ram_addr_t) == sizeof(ssize_t), so it should be fine.


> 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.

I have squashed 1-3, but I don't think 7 should be squashed. Adding a
new feature and taking away the old one in the same patch isn't right IMHO.

Cheers,
Jes