Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification

[Max Reitz <mreitz@redhat.com>]

On 2015-03-11 at 07:30, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> This patch makes sure the result of strtol() does not overflow (by
>> storing it in long integers instead of plain integers, and by checking
>> errno), allows the user to specify "--discard on" and
>> "--detect-zeroes unmap" in any order and strips the trailing \n from two
>> error messages.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-nbd.c | 40 +++++++++++++++++++++++++++-------------
>>   1 file changed, 27 insertions(+), 13 deletions(-)
> Let's introduce a general purpose utility instead of duplicating the
> checks in every strto* caller.
>
> For example, you can ensure that errno is checked and, if NULL is
> passed as the second argument, that the whole string is a number.
> Something like this:
>
>     int qemu_strtol(const char *name, const char **next, int base, long *result)
>     {
>         char *p;
>         errno = 0;
>         *result = strtol(name, &p, base);
>         if (!next && *p) {
>             return -EINVAL;
>         }
>         if (next) {
>             *next = p;
>         }
>         return -errno;
>     }

I was afraid you might say this... Will do.


Thank you for reviewing!

Max

> Paolo
>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index fd1e0c8..7376a35 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -51,7 +51,7 @@ static char *srcpath;
>>   static char *sockpath;
>>   static int persistent = 0;
>>   static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
>> -static int shared = 1;
>> +static long shared = 1;
>>   static int nb_fds;
>>   
>>   static void usage(const char *name)
>> @@ -432,10 +432,10 @@ int main(int argc, char **argv)
>>       };
>>       int ch;
>>       int opt_ind = 0;
>> -    int li;
>> +    long li;
>>       char *end;
>>       int flags = BDRV_O_RDWR;
>> -    int partition = -1;
>> +    long partition = -1;
>>       int ret = 0;
>>       int fd;
>>       bool seen_cache = false;
>> @@ -510,11 +510,6 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
>>                        error_get_pretty(local_err));
>>               }
>> -            if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> -                !(flags & BDRV_O_UNMAP)) {
>> -                errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
>> -                                   "without setting discard operation to unmap");
>> -            }
>>               break;
>>           case 'b':
>>               bindto = optarg;
>> @@ -530,13 +525,17 @@ int main(int argc, char **argv)
>>               port = (uint16_t)li;
>>               break;
>>           case 'o':
>> -                dev_offset = strtoll (optarg, &end, 0);
>> +            errno = 0;
>> +            dev_offset = strtoll(optarg, &end, 0);
>>               if (*end) {
>>                   errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>>               }
>>               if (dev_offset < 0) {
>>                   errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>>               }
>> +            if (errno) {
>> +                err(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>> +            }
>>               break;
>>           case 'l':
>>               if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
>> @@ -559,13 +558,13 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
>>               }
>>               if (partition < 1 || partition > 8) {
>> -                errx(EXIT_FAILURE, "Invalid partition %d", partition);
>> +                errx(EXIT_FAILURE, "Invalid partition %s", optarg);
>>               }
>>               break;
>>           case 'k':
>>               sockpath = optarg;
>>               if (sockpath[0] != '/') {
>> -                errx(EXIT_FAILURE, "socket path must be absolute\n");
>> +                errx(EXIT_FAILURE, "socket path must be absolute");
>>               }
>>               break;
>>           case 'd':
>> @@ -580,7 +579,12 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
>>               }
>>               if (shared < 1) {
>> -                errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
>> +                errx(EXIT_FAILURE,
>> +                     "Shared device number must be greater than 0");
>> +            }
>> +            if (shared >= INT_MAX) {
>> +                errx(EXIT_FAILURE,
>> +                     "Shared device number must be less than %i", INT_MAX);
>>               }
>>               break;
>>           case 'f':
>> @@ -606,6 +610,12 @@ int main(int argc, char **argv)
>>           }
>>       }
>>   
>> +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> +        !(flags & BDRV_O_UNMAP)) {
>> +        errx(EXIT_FAILURE, "Setting detect-zeroes to unmap is not allowed "
>> +                           "without setting discard operation to unmap");
>> +    }
>> +
>>       if ((argc - optind) != 1) {
>>           errx(EXIT_FAILURE, "Invalid number of argument.\n"
>>                "Try `%s --help' for more information.",
>> @@ -730,10 +740,14 @@ int main(int argc, char **argv)
>>       }
>>   
>>       if (partition != -1) {
>> +        if (dev_offset) {
>> +            errx(EXIT_FAILURE, "Cannot use both -o and -P at the same time");
>> +        }
>> +
>>           ret = find_partition(blk, partition, &dev_offset, &fd_size);
>>           if (ret < 0) {
>>               errno = -ret;
>> -            err(EXIT_FAILURE, "Could not find partition %d", partition);
>> +            err(EXIT_FAILURE, "Could not find partition %ld", partition);
>>           }
>>       }
>>   
>>