On Tue, Jun 12, 2018 at 09:54:47AM -0600, Theo de Raadt wrote:
> I would prefer if the -q and -n descriptions were in a table. I dislike
> the ancient style of describing such things inline (harder to spot).
> And it really falls down when there are multiple ones. How do you
> feel about that jmc?
Agreed, I changed it a bit to improve readability.
> Also, do -qn and -nq work? How about -nnn. Not saying those make a
> lot of sense, but once getopt syntax is borrowed it should probably be
> honoured.
I redid this piece a little bit, and opted to go a bit stricter to leave
as much freedom as possible for future extensions.
OK:
-n command
-n -q command
-q -n command
-q command
command
Not OK:
-nn command
-qn command
-q -q command
-n -n -q command
My thinking is by being strict now, we make it possible to add arguments
to options in the future. If we allow "-nn" or "-nq" now, we won't be
able to allow "[email protected]" in the future. Or maybe we'll want
"-v" to mean something different than "-vv". I don't know, so prefer to
be less forgiving.
Kind regards,
Job
diff --git usr.sbin/cron/crontab.5 usr.sbin/cron/crontab.5
index 9c2e651980a..d9330698fd3 100644
--- usr.sbin/cron/crontab.5
+++ usr.sbin/cron/crontab.5
@@ -193,15 +193,29 @@ will be changed into newline characters, and all data
after the first
.Ql %
will be sent to the command as standard input.
-If the
+.Ss Options
+The
.Ar command
-field begins with
-.Ql -q ,
-execution will not be logged.
+field can begin with one or more options.
+.Bl -tag -width Ds
+.It Fl n
+No mail is send after a successful run.
+The execution output will only be mailed if the command exits with a non-zero
+exit code.
+The
+.Ql -n
+option is an attempt to cure potentially copious volumes of mail coming from
+.Xr cron 8 .
+.It Fl q
+Execution will not be logged.
+.El
+.Pp
Use whitespace to separate
-.Ql -q
-from the command.
+.Ql -n ,
+.Ql -q ,
+and the command.
.Pp
+.Ss Execution
Commands are executed by
.Xr cron 8
when the
@@ -329,6 +343,9 @@ Ranges may include
.It
Months or days of the week can be specified by name.
.It
+Mailing after a successful run can be suppressed with
+.Ql -n .
+.It
Logging can be suppressed with
.Ql -q .
.It
diff --git usr.sbin/cron/do_command.c usr.sbin/cron/do_command.c
index 6a4022fcc9a..4fbca61d170 100644
--- usr.sbin/cron/do_command.c
+++ usr.sbin/cron/do_command.c
@@ -3,6 +3,7 @@
/* Copyright 1988,1990,1993,1994 by Paul Vixie
* Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
* Copyright (c) 1997,2000 by Internet Software Consortium, Inc.
+ * Copyright (c) 2018 Job Snijders <[email protected]>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -80,7 +81,6 @@ child_process(entry *e, user *u)
char **p, *input_data, *usernm;
auth_session_t *as;
login_cap_t *lc;
- int children = 0;
extern char **environ;
/* mark ourselves as different to PS command watchers */
@@ -146,7 +146,9 @@ child_process(entry *e, user *u)
/* fork again, this time so we can exec the user's command.
*/
- switch (fork()) {
+
+ pid_t jobpid;
+ switch (jobpid = fork()) {
case -1:
syslog(LOG_ERR, "(CRON) CAN'T FORK (%m)");
_exit(EXIT_FAILURE);
@@ -260,8 +262,6 @@ child_process(entry *e, user *u)
break;
}
- children++;
-
/* middle process, child of original cron, parent of process running
* the user's command.
*/
@@ -283,7 +283,8 @@ child_process(entry *e, user *u)
* we would block here. thus we must fork again.
*/
- if (*input_data && fork() == 0) {
+ pid_t stdinjob;
+ if (*input_data && (stdinjob = fork()) == 0) {
FILE *out = fdopen(stdin_pipe[WRITE_PIPE], "w");
int need_newline = FALSE;
int escaped = FALSE;
@@ -331,8 +332,6 @@ child_process(entry *e, user *u)
*/
close(stdin_pipe[WRITE_PIPE]);
- children++;
-
/*
* read output from the grandchild. it's stderr has been redirected to
* it's stdout, which has been redirected to our pipe. if there is any
@@ -342,15 +341,17 @@ child_process(entry *e, user *u)
(void) signal(SIGPIPE, SIG_IGN);
in = fdopen(stdout_pipe[READ_PIPE], "r");
+
+ char *mailto;
+ FILE *mail = NULL;
+ int status = 0;
+ pid_t mailpid;
+ size_t bytes = 1;
+
if (in != NULL) {
int ch = getc(in);
if (ch != EOF) {
- FILE *mail = NULL;
- char *mailto;
- size_t bytes = 1;
- int status = 0;
- pid_t mailpid;
/* get name of recipient. this is MAILTO if set to a
* valid local username; USER otherwise.
@@ -415,30 +416,6 @@ child_process(entry *e, user *u)
fputc(ch, mail);
}
- /* only close pipe if we opened it -- i.e., we're
- * mailing...
- */
-
- if (mail) {
- /* Note: the pclose will probably see
- * the termination of the grandchild
- * in addition to the mail process, since
- * it (the grandchild) is likely to exit
- * after closing its stdout.
- */
- status = cron_pclose(mail, mailpid);
- }
-
- /* if there was output and we could not mail it,
- * log the facts so the poor user can figure out
- * what's going on.
- */
- if (mail && status) {
- syslog(LOG_NOTICE, "(%s) MAIL (mailed %zu byte"
- "%s of output but got status 0x%04x)", usernm,
- bytes, (bytes == 1) ? "" : "s", status);
- }
-
} /*if data from grandchild*/
fclose(in); /* also closes stdout_pipe[READ_PIPE] */
@@ -446,20 +423,50 @@ child_process(entry *e, user *u)
/* wait for children to die.
*/
- for (; children > 0; children--) {
- int waiter;
- pid_t pid;
-
- while ((pid = wait(&waiter)) < 0 && errno == EINTR)
+ int waiter;
+ if (jobpid > 0) {
+ ;
+ while (waitpid(jobpid, &waiter, 0) < 0 && errno == EINTR)
;
- if (pid < 0) {
- break;
+
+ /* If everything went well, and -n was set, _and_ we have mail,
+ * we won't be mailing... so shoot the messenger!
+ */
+ if (WIFEXITED(waiter) && WEXITSTATUS(waiter) == 0
+ && (e->flags & MAIL_WHEN_ERR) == MAIL_WHEN_ERR
+ && mail) {
+ kill(mailpid, SIGKILL);
+ (void)fclose(mail);
+ mail = NULL;
}
- /*
- if (WIFSIGNALED(waiter) && WCOREDUMP(waiter))
- Debug(DPROC, (", dumped core"))
+
+ /* only close pipe if we opened it -- i.e., we're
+ * mailing...
*/
+ if (mail) {
+ /*
+ * Note: the pclose will probably see the termination
+ * of the grandchild in addition to the mail process,
+ * since it (the grandchild) is likely to exit after
+ * closing its stdout.
+ */
+ status = cron_pclose(mail, mailpid);
+ }
+
+ /* if there was output and we could not mail it,
+ * log the facts so the poor user can figure out
+ * what's going on.
+ */
+ if (mail && status) {
+ syslog(LOG_NOTICE, "(%s) MAIL (mailed %zu byte"
+ "%s of output but got status 0x%04x)", usernm,
+ bytes, (bytes == 1) ? "" : "s", status);
+ }
}
+
+ if (stdinjob > 0)
+ while (waitpid(stdinjob, &waiter, 0) < 0 && errno == EINTR)
+ ;
}
int
diff --git usr.sbin/cron/entry.c usr.sbin/cron/entry.c
index 761f01d3d6f..cba6e805dc0 100644
--- usr.sbin/cron/entry.c
+++ usr.sbin/cron/entry.c
@@ -338,14 +338,32 @@ load_entry(FILE *file, void (*error_func)(const char *),
struct passwd *pw,
ch = get_char(file);
while (ch == '-') {
switch (ch = get_char(file)) {
+ case 'n':
+ /* only allow the user to set this option once */
+ if ((e->flags & MAIL_WHEN_ERR) == MAIL_WHEN_ERR) {
+ ecode = e_option;
+ goto eof;
+ }
+ e->flags |= MAIL_WHEN_ERR;
+ break;
case 'q':
+ /* only allow the user to set this option once */
+ if ((e->flags & DONT_LOG) == DONT_LOG) {
+ ecode = e_option;
+ goto eof;
+ }
e->flags |= DONT_LOG;
- Skip_Nonblanks(ch, file)
break;
default:
ecode = e_option;
goto eof;
}
+ ch = get_char(file);
+ if (ch!='\t' && ch!=' ') {
+ ecode = e_option;
+ goto eof;
+ }
+
Skip_Blanks(ch, file)
if (ch == EOF || ch == '\n') {
ecode = e_cmd;
diff --git usr.sbin/cron/structs.h usr.sbin/cron/structs.h
index af0d75c3b55..597f65d6614 100644
--- usr.sbin/cron/structs.h
+++ usr.sbin/cron/structs.h
@@ -38,6 +38,7 @@ typedef struct _entry {
#define DOW_STAR 0x08
#define WHEN_REBOOT 0x10
#define DONT_LOG 0x20
+#define MAIL_WHEN_ERR 0x40
} entry;
/* the crontab database will be a list of the
