On Sun, Aug 19, 2018 at 09:40:43AM +0100, Ricardo Mestre wrote:
>
> With your suggestion now it doesn't segfault anymore, but it still outputs
> crazy stuff, do we really want this? Also -p still shows contents of the file
> as per below:
>
> 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c
> total 762774013172614.25
> 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c -p
> opyright (c) 1994 Christopher G. 338687831669364.19
> etain the above copyright
> * 258230964833358.44
> user_list *Users = NULL;
> static 46353282047238.85
> -") == 0)
> fp = stdin;
> else if 43998158041234.41
> g second */
> yesterday++;
> for 39385397001446.54
> se it
> */
> tlp = lp;
> lp 18139557014551.83
> they came in on a pseudo-tty, t 17978822565419.97
> */
> head = log_out(head, &usr);
> 0.00
> total 762774013172614.25
utmp(5) is a binary format, so crazy input causes crazy output.
I don't think we should be trying to outsmart the user. At least,
not trying too hard.
> What if we still add the check secs < 0 || secs > LLONG_MAX but instead of
> erroring out just set secs to 0? It's the same behaviour as we have right now
> if we use 'ac -w ./bogus_file user', this will call update_user with secs set
> to 0L and the program won't segfault as it does if 'user' is not specified.
First, there's no reason to check secs > LLONG_MAX. time_t is a signed
64-bit value on all platforms, as is long long, so that comparison is
always false.
Next, I don't think we should add a sanity check to handle a case where the
user literally needs to feed the program garbage to trigger the behavior.
The only case in which I see the check being useful is if the file is
corrupted. And in that case I don't think we should truncate to to 0
and continue running. We should either handle the value as-is, allowing
the user to see the crazy value in the output and reason about it, or fail
closed and indicate that the file is invalid.
I'm leaning toward not changing anything because it's at best a super
rare edge case and I'm worried we're missing something.
I want to think on it a bit more but at the moment I'm not ok with adding
the check.
Addressing the incorrect use of strlcpy/strchr is still totally ok though,
as they are definitely bugs.
More comments inline below.
> Index: ac.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ac/ac.c,v
> retrieving revision 1.25
> diff -u -p -u -r1.25 ac.c
> --- ac.c 26 Apr 2018 12:42:51 -0000 1.25
> +++ ac.c 19 Aug 2018 08:26:54 -0000
> @@ -29,6 +29,7 @@
>
> #include <err.h>
> #include <errno.h>
> +#include <limits.h>
> #include <pwd.h>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -171,6 +172,9 @@ update_user(struct user_list *head, char
> {
> struct user_list *up;
>
> + if (secs < 0 || secs > LLONG_MAX)
> + secs = 0;
> +
> for (up = head; up != NULL; up = up->next) {
> if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) {
> up->secs += secs;
> @@ -187,7 +191,8 @@ update_user(struct user_list *head, char
> if ((up = malloc(sizeof(struct user_list))) == NULL)
> err(1, "malloc");
While you're here could you allocate sizeof(*up) and change the err(3) from
"malloc" -> NULL?
> up->next = head;
> - strlcpy(up->name, name, sizeof (up->name));
> + strncpy(up->name, name, sizeof (up->name));
> + up->name[sizeof(up->name) - 1] = '\0';
That strncpy should be
strncpy(up->name, name, sizeof(up->name) - 1);
> up->secs = secs;
> Total += secs;
> return up;
> @@ -446,7 +451,8 @@ ac(FILE *fp)
> */
> if (*usr.ut_name) {
> if (strncmp(usr.ut_line, "tty", 3) != 0 ||
> - strchr("pqrstuvwxyzPQRST", usr.ut_line[3])
> != NULL ||
> + memchr("pqrstuvwxyzPQRST", usr.ut_line[3],
> + sizeof("pqrstuvwxyzPQRST")) != NULL ||
The memchr length needs to be sizeof("p...") - 1, otherwise you'll include
the NUL terminator in your search.
> *usr.ut_host != '\0')
> head = log_in(head, &usr);
> } else
> @@ -457,7 +463,8 @@ ac(FILE *fp)
> (void)fclose(fp);
> if (!(Flags & AC_W))
> usr.ut_time = time(NULL);
> - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line);
> + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line);
> + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0';
Same deal here, use sizeof(usr.ut_line) - 1 for the strncpy length.
>
> if (Flags & AC_D) {
> ltm = localtime(&usr.ut_time);
>
