Re: vmctl: off-by-one error handling mixing -a with a VM id

[Theo Buehler]

On Thu, Mar 25, 2021 at 08:07:53PM +0100, Preben Guldberg wrote:
> Dave Voutila wrote:
> > Preben Guldberg writes:
> > > The patch below addresses an off-by-one error reading argv when
> > > generating the error message.
> 
> > > I personally find it clearer if the condition of mixing -a with an id
> > > is highlighted. I included a suggestion in the patch below.
> 
> > Since -a and providing an id are mutually exclusive, I think it's more
> > helpful to print usage information via ctl_usage(res->ctl). From the
> > usage details, it's self explanatory what's wrong.
> 
> >   usage:  vmctl [-v] stop [-fw] [id | -a]
> 
> The updated diff below would do just that:
> 
>     % vmctl stop -a testvm
>     usage:  vmctl [-v] stop [-fw] [id | -a]

Yes, your diff would do that.

However, I think the current logic is both wrong and the wrong way
around.  I believe the following is much clearer. It doesn't have a dead
else branch and it deletes 'ret', so it doesn't use it uninitialized when
checking 'res->action == CMD_STOPALL && ret != -1' (e.g. 'vmctl stop -a').
Since the diff is slightly messy, this is the result:

        if (res->action == CMD_STOPALL) {
                if (argc != 0)
                        ctl_usage(res->ctl);
        } else {
                if (argc != 1)
                        ctl_usage(res->ctl);
                if (parse_vmid(res, argv[0], 0) == -1)
                        errx(1, "invalid id: %s", argv[0]);
        }

        return (vmmaction(res));

Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.62
diff -u -p -r1.62 main.c
--- main.c      3 Jan 2020 05:32:00 -0000       1.62
+++ main.c      25 Mar 2021 19:23:16 -0000
@@ -927,7 +927,7 @@ ctl_start(struct parse_result *res, int 
 int
 ctl_stop(struct parse_result *res, int argc, char *argv[])
 {
-       int              ch, ret;
+       int              ch;
 
        while ((ch = getopt(argc, argv, "afw")) != -1) {
                switch (ch) {
@@ -948,20 +948,15 @@ ctl_stop(struct parse_result *res, int a
        argc -= optind;
        argv += optind;
 
-       if (argc == 0) {
-               if (res->action != CMD_STOPALL)
+       if (res->action == CMD_STOPALL) {
+               if (argc != 0)
                        ctl_usage(res->ctl);
-       } else if (argc > 1)
-               ctl_usage(res->ctl);
-       else if (argc == 1)
-               ret = parse_vmid(res, argv[0], 0);
-       else
-               ret = -1;
-
-       /* VM id is only expected without the -a flag */
-       if ((res->action != CMD_STOPALL && ret == -1) ||
-           (res->action == CMD_STOPALL && ret != -1))
-               errx(1, "invalid id: %s", argv[1]);
+       } else {
+               if (argc != 1)
+                       ctl_usage(res->ctl);
+               if (parse_vmid(res, argv[0], 0) == -1)
+                       errx(1, "invalid id: %s", argv[0]);
+       }
 
        return (vmmaction(res));
 }