check hhmm for leave

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

check hhmm for leave

Ted Unangst-6
In testing leave, I found it accepts "12" and will alarm at 00:12, which is
probably not desirable. this check thats the specified time has 4 digits so
that the hour/minute math works properly.


Index: leave.c
===================================================================
RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
retrieving revision 1.19
diff -u -p -r1.19 leave.c
--- leave.c 10 Feb 2018 00:00:47 -0000 1.19
+++ leave.c 22 Jan 2020 02:13:03 -0000
@@ -35,6 +35,7 @@
 #include <err.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 
 static __dead void usage(void);
@@ -83,6 +84,9 @@ main(int argc, char *argv[])
  plusnow = 1;
  ++cp;
  }
+
+ if (strlen(cp) != 4)
+ usage();
 
  for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
  if (!isdigit((unsigned char)c))

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Theo de Raadt-2
I suspect this breaks an usage case which is not documented.

People may be doing leave +1 to mean "1 minutes".

% leave +1
Alarm set for Tue Jan 21 19:23. (pid 55602)
% date
Tue Jan 21 19:22:32 MST 2020
% Just one more minute!

1) it isn't documented that + can take a smaller number
2) it will be hard to train people to use +0001

Ted Unangst <[hidden email]> wrote:

> In testing leave, I found it accepts "12" and will alarm at 00:12, which is
> probably not desirable. this check thats the specified time has 4 digits so
> that the hour/minute math works properly.
>
>
> Index: leave.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 leave.c
> --- leave.c 10 Feb 2018 00:00:47 -0000 1.19
> +++ leave.c 22 Jan 2020 02:13:03 -0000
> @@ -35,6 +35,7 @@
>  #include <err.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <unistd.h>
>  
>  static __dead void usage(void);
> @@ -83,6 +84,9 @@ main(int argc, char *argv[])
>   plusnow = 1;
>   ++cp;
>   }
> +
> + if (strlen(cp) != 4)
> + usage();
>  
>   for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
>   if (!isdigit((unsigned char)c))
>

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Scott Cheloha
On Tue, Jan 21, 2020 at 07:23:43PM -0700, Theo de Raadt wrote:

> I suspect this breaks an usage case which is not documented.
>
> People may be doing leave +1 to mean "1 minutes".
>
> % leave +1
> Alarm set for Tue Jan 21 19:23. (pid 55602)
> % date
> Tue Jan 21 19:22:32 MST 2020
> % Just one more minute!
>
> 1) it isn't documented that + can take a smaller number
> 2) it will be hard to train people to use +0001

These are my concerns as well.

An idea I had a while back was to drop support for +hhmm and just
support +minutes, like we do with shutdown(8).  The invocation would
then be:

usage: leave [hhmm | +minutes]

so you could enforce strict string length requirements in the former
case.

I think typing `leave +0100` to say "remind me to leave in one hour"
is pretty strange.  Typing `leave +60` seems more natural.  And typing
`leave +5` is much better than `leave +0005`.

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Theo de Raadt-2
Scott Cheloha <[hidden email]> wrote:

> On Tue, Jan 21, 2020 at 07:23:43PM -0700, Theo de Raadt wrote:
> > I suspect this breaks an usage case which is not documented.
> >
> > People may be doing leave +1 to mean "1 minutes".
> >
> > % leave +1
> > Alarm set for Tue Jan 21 19:23. (pid 55602)
> > % date
> > Tue Jan 21 19:22:32 MST 2020
> > % Just one more minute!
> >
> > 1) it isn't documented that + can take a smaller number
> > 2) it will be hard to train people to use +0001
>
> These are my concerns as well.
>
> An idea I had a while back was to drop support for +hhmm and just
> support +minutes, like we do with shutdown(8).  The invocation would
> then be:
>
> usage: leave [hhmm | +minutes]
>
> so you could enforce strict string length requirements in the former
> case.
>
> I think typing `leave +0100` to say "remind me to leave in one hour"
> is pretty strange.  Typing `leave +60` seems more natural.  And typing
> `leave +5` is much better than `leave +0005`.

Is that 60 minutes or 64 minutes?

/duck

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Theo de Raadt-2
In reply to this post by Scott Cheloha
Scott Cheloha <[hidden email]> wrote:

> On Tue, Jan 21, 2020 at 07:23:43PM -0700, Theo de Raadt wrote:
> > I suspect this breaks an usage case which is not documented.
> >
> > People may be doing leave +1 to mean "1 minutes".
> >
> > % leave +1
> > Alarm set for Tue Jan 21 19:23. (pid 55602)
> > % date
> > Tue Jan 21 19:22:32 MST 2020
> > % Just one more minute!
> >
> > 1) it isn't documented that + can take a smaller number
> > 2) it will be hard to train people to use +0001
>
> These are my concerns as well.
>
> An idea I had a while back was to drop support for +hhmm and just
> support +minutes, like we do with shutdown(8).  The invocation would
> then be:
>
> usage: leave [hhmm | +minutes]
>
> so you could enforce strict string length requirements in the former
> case.
>
> I think typing `leave +0100` to say "remind me to leave in one hour"
> is pretty strange.  Typing `leave +60` seems more natural.  And typing
> `leave +5` is much better than `leave +0005`.

I do like the +minutes proposal.

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Ted Unangst-6
In reply to this post by Scott Cheloha
Scott Cheloha wrote:
> > 1) it isn't documented that + can take a smaller number
> > 2) it will be hard to train people to use +0001
>
> These are my concerns as well.
>
> An idea I had a while back was to drop support for +hhmm and just
> support +minutes, like we do with shutdown(8).  The invocation would
> then be:

this adds the check to just the clock time case. leave +mm isn't bad, but it's
a little awkward past 60. leave +180 for three hours? unfortunately that
clashes with existing usage. but my original concern is primarily with hhmm so
let's just address that.


Index: leave.c
===================================================================
RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
retrieving revision 1.19
diff -u -p -r1.19 leave.c
--- leave.c 10 Feb 2018 00:00:47 -0000 1.19
+++ leave.c 22 Jan 2020 04:07:41 -0000
@@ -35,6 +35,7 @@
 #include <err.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 
 static __dead void usage(void);
@@ -83,6 +84,9 @@ main(int argc, char *argv[])
  plusnow = 1;
  ++cp;
  }
+
+ if (!plusnow && strlen(cp) != 4)
+ usage();
 
  for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
  if (!isdigit((unsigned char)c))

Reply | Threaded
Open this post in threaded view
|

Re: check hhmm for leave

Theo de Raadt-2
fine.

Ted Unangst <[hidden email]> wrote:

> Scott Cheloha wrote:
> > > 1) it isn't documented that + can take a smaller number
> > > 2) it will be hard to train people to use +0001
> >
> > These are my concerns as well.
> >
> > An idea I had a while back was to drop support for +hhmm and just
> > support +minutes, like we do with shutdown(8).  The invocation would
> > then be:
>
> this adds the check to just the clock time case. leave +mm isn't bad, but it's
> a little awkward past 60. leave +180 for three hours? unfortunately that
> clashes with existing usage. but my original concern is primarily with hhmm so
> let's just address that.
>
>
> Index: leave.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/leave/leave.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 leave.c
> --- leave.c 10 Feb 2018 00:00:47 -0000 1.19
> +++ leave.c 22 Jan 2020 04:07:41 -0000
> @@ -35,6 +35,7 @@
>  #include <err.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <unistd.h>
>  
>  static __dead void usage(void);
> @@ -83,6 +84,9 @@ main(int argc, char *argv[])
>   plusnow = 1;
>   ++cp;
>   }
> +
> + if (!plusnow && strlen(cp) != 4)
> + usage();
>  
>   for (hours = 0; (c = *cp) && c != '\n'; ++cp) {
>   if (!isdigit((unsigned char)c))