Re: [PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eyal Moscovici <eyal.moscovici@oracle.com>]

On 07/05/2020 0:49, Eric Blake wrote:
> On 5/6/20 4:34 PM, Eyal Moscovici wrote:
>> Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 
>> (util/cutils: Change
>> qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
>> cvtnum. As a result there is no need to check it separately outside 
>> of cvtnum.
>>
>> Acked-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>>   qemu-img.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 6a4327aaba..116a9c6349 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid buffer size specified");
>
> INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
> change allows larger buffer sizes, which is probably not a good idea.
I was the most hesitant about this patch because of the size difference. 
I decided to submit it because the type is int64 which pairs better with 
the MAX_INT64 check and I couldn't find a concrete reason to cap the 
variable at MAX_INT. Do you a concrete reason? Because the max size 
should rerally come into effect on very fringe cases and if you are 
asking for a really big buffer you should know the risks.
>
>>                   return 1;
>>               }
>> @@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid step size specified");
>>                   return 1;
>>               }
>> @@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
>>         res = cvtnum(arg);
>>   -    if (res <= 0 || res > INT_MAX) {
>> +    if (res <= 0) {
>>           error_report("invalid number: '%s'", arg);
>>           return 1;
>>       }
>>
>
> NACK.
>