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

[Max Reitz]

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 <address@hidden>
> > ---
> >   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);
> >           }
> >       }
> >