does the wrong thing with /etc/cron.d/zfs-auto-snapshot

[loreb]

$ cat /etc/cron.d/zfs-auto-snapshot

PATH="/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"

*/15 * * * * root which zfs-auto-snapshot > /dev/null || exit 0 ; zfs-auto-snapshot --quiet --syslog --label=frequent --keep=4 //

git clone, make, ./crond -f

./crond 4.5 dillon's cron daemon, started with loglevel notice failed parsing crontab for user root: PATH="/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"

root also gets an email saying "/bin/sh: 1: root: not found"

This is from a linux distro (voidlinux fyi); I don't know if the dragonflybsd convention is different, but here at least files in /etc/cron.d are supposed to be that way (see eg http://deb.debian.org/debian/pool/main/a/anacron/anacron_2.3-27.debian.tar.xz), ie the first field after the time specification is a username to run the command as, and lines like "foo=bar" mean environment assignment (the one above looks pointless to me, and I'm not sure if any other cron implementation gets confused by the quotes; I'm assuming they are ok since https://bugs.debian.org/cgi-bin/pkgreport.cgi?pkg=zfs-auto-snapshot;dist=unstable doesn't say anything)

If there's any more info or anything you need please do let me know.

[loreb]

Here's a first, fugly patch to run entries in /etc/cron.d as their own user. Among other things, I used C++ style comments to make them stand out more. Regular entries run as usual, cron.d entries work with the proper user, except for the

failed parsing crontab for user root: PATH="/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"

which is still there.

The biggest problem (other than esthetics) is that debug messages assume that all files in /etc/cron.d run as root.

Also, while unrelated, there's an indentation warning (-Wmisleading-indentation) in database.c, could you look at it? It looks like a genuine bug to me.

diff --git a/database.c b/database.c
index c0cdc11..10d2490 100644
--- a/database.c
+++ b/database.c
@@ -26,7 +26,7 @@ Prototype int ArmJob(CronFile *file, CronLine *line, time_t t1, time_t t2);
 Prototype void RunJobs(void);
 Prototype int CheckJobs(void);
 
-void SynchronizeFile(const char *dpath, const char *fname, const char *uname);
+void SynchronizeFile(const char *dpath, const char *fname, const char *uname, int kludge);
 void DeleteFile(CronFile **pfile);
 char *ParseInterval(int *interval, char *ptr);
 char *ParseField(char *userName, char *ary, int modvalue, int offset, int onvalue, const char **names, char *ptr);
@@ -126,11 +126,11 @@ CheckUpdates(const char *dpath, const char *user_override, time_t t1, time_t t2)
 			fname = strtok_r(buf, " \t\n", &ptok);
 
 			if (user_override)
-				SynchronizeFile(dpath, fname, user_override);
+				SynchronizeFile(dpath, fname, user_override, 1);
 			else if (!getpwnam(fname))
 				printlogf(LOG_WARNING, "ignoring %s/%s (non-existent user)\n", dpath, fname);
 			else if (*ptok == 0 || *ptok == '\n') {
-				SynchronizeFile(dpath, fname, fname);
+				SynchronizeFile(dpath, fname, fname, 0);
 				ReadTimestamps(fname);
 			} else {
 				/* if fname is followed by whitespace, we prod any following jobs */
@@ -221,9 +221,9 @@ SynchronizeDir(const char *dpath, const char *user_override, int initial_scan)
 			if (strcmp(den->d_name, CRONUPDATE) == 0)
 				continue;
 			if (user_override) {
-				SynchronizeFile(dpath, den->d_name, user_override);
+				SynchronizeFile(dpath, den->d_name, user_override, 1);
 			} else if (getpwnam(den->d_name)) {
-				SynchronizeFile(dpath, den->d_name, den->d_name);
+				SynchronizeFile(dpath, den->d_name, den->d_name, 0);
 			} else {
 				printlogf(LOG_WARNING, "ignoring %s/%s (non-existent user)\n",
 						dpath, den->d_name);
@@ -308,8 +308,9 @@ ReadTimestamps(const char *user)
 	}
 }
 
+// kludge is for entries in cron.d, which have an extra field - the username to run as.
 void
-SynchronizeFile(const char *dpath, const char *fileName, const char *userName)
+SynchronizeFile(const char *dpath, const char *fileName, const char *userName, int kludge)
 {
 	CronFile **pfile;
 	CronFile *file;
@@ -631,7 +632,26 @@ SynchronizeFile(const char *dpath, const char *fileName, const char *userName)
 				/*
 				 * copy command string
 				 */
-				line.cl_Shell = strdup(ptr);
+				if (!kludge) {
+					line.cl_Shell = strdup(ptr);
+					line.cl_Setuidgid = 0; // TODO check that the file is owned by root?
+				} else {
+					// ptr = "username cmd...", possibly with blanks
+					char *cmd;
+					// skip whitespace (if any)
+					ptr += strspn(ptr, " \t\n");
+					cmd = ptr;
+					cmd += strcspn(cmd, " \t\n"); // immediately after user - " cmd..."
+					if(*cmd) {
+						*cmd = '\0';
+						cmd++;
+					}
+					// XXX TODO we happily ignore setting uid to "" atm!
+					// The reason is that the cleanup is complicated - see above.
+					line.cl_Setuidgid = strdup(ptr);
+					line.cl_Shell = strdup(cmd);
+				}
+
 
 				if (line.cl_Delay > 0) {
 					if (!(line.cl_Timestamp = concat(TSDir, "/", userName, ".", line.cl_JobName, NULL))) {
@@ -913,6 +933,7 @@ DeleteFile(CronFile **pfile)
 		} else {
 			*pline = line->cl_Next;
 			free(line->cl_Shell);
+			free(line->cl_Setuidgid);
 
 			if (line->cl_JobName)
 				/* this frees both cl_Description and cl_JobName
@@ -1145,6 +1166,7 @@ TestStartupJobs(void)
 	t1 = t1 - t1 % 60 + 60;
 
 	for (file = FileBase; file; file = file->cf_Next) {
+		// TODO with cron.d, each __line__ has its user!!!
 		if (DebugOpt)
 			printlogf(LOG_DEBUG, "TestStartup for FILE %s/%s USER %s:\n",
 				file->cf_DPath, file->cf_FileName, file->cf_UserName);
diff --git a/defs.h b/defs.h
index cf77b5f..184cdab 100644
--- a/defs.h
+++ b/defs.h
@@ -137,6 +137,7 @@ typedef struct CronFile {
 typedef struct CronLine {
     struct CronLine *cl_Next;
     char	*cl_Shell;	/* shell command			*/
+    char	*cl_Setuidgid;	/* every *line*: override cf_UserName in SCRONTABS */
 	char	*cl_Description;	/* either "<cl_Shell>" or "job <cl_JobName>" */
 	char	*cl_JobName;	/* job name, if any			*/
 	char	*cl_Timestamp;	/* path to timestamp file, if cl_Freq defined */
diff --git a/job.c b/job.c
index 017ca3d..723ad20 100644
--- a/job.c
+++ b/job.c
@@ -36,7 +36,7 @@ RunJob(CronFile *file, CronLine *line)
 		line->cl_MailFlag = 1;
 		/* if we didn't specify a -m Mailto, use the local user */
 		if (!value)
-			value = file->cf_UserName;
+			value = file->cf_UserName; // XXX cron.d
 		fdprintf(mailFd, "To: %s\nSubject: cron for user %s %s\n\n",
 				value,
 				file->cf_UserName,
@@ -69,6 +69,14 @@ RunJob(CronFile *file, CronLine *line)
 					);
 			exit(0);
 		}
+		// change TWICE - printlogf?
+		if (line->cl_Setuidgid && ChangeUser(line->cl_Setuidgid, TempDir) < 0) {
+			printlogf(LOG_ERR, "unable to ChangeUser (user %s %s)\n",
+					line->cl_Setuidgid,
+					line->cl_Description
+					);
+			exit(0);
+		}
 
 		/* from this point we are unpriviledged */
 

[hills]

In the same spirit as your patch, I propose some changes in #30