Date: Tue, 31 Jan 2012 10:57:50 -0700 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: John Baldwin <jhb@freebsd.org> Cc: current@freebsd.org Subject: Re: Race between cron and crontab Message-ID: <1328032670.1662.333.camel@revolution.hippie.lan> In-Reply-To: <201201311149.32760.jhb@freebsd.org> References: <201201311149.32760.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-8C+JykMdlr7Y4wfwfSp0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
On Tue, 2012-01-31 at 11:49 -0500, John Baldwin wrote:
> A co-worker ran into a race between updating a cron tab via crontab(8) and
> cron(8) yesterday. Specifically, cron(8) failed to notice that a crontab was
> updated. The problem is that 1) by default our filesystems only use second
> granularity for timestamps and 2) cron only caches the seconds portion of a
> file's timestamp when checking for changes anyway. This means that cron can
> miss updates to a spool directory if multiple updates to the directory are
> performed within a single second and cron wakes up to scan the spool directory
> within the same second and scans it before all of the updates are complete.
>
> Specifically, when replacing a crontab, crontab(8) first creates a temporary
> file in /var/cron/tabs and then uses a rename to install it followed by
> touching the spool directory to update its modification time. However, the
> creation of the temporary file already changes the modification time of the
> directory, and cron may "miss" the rename if it scans the directory in between
> the creation of the temporary file and the rename.
>
> The "fix" I am planning to use locally is to simply force crontab(8) to sleep
> for a second before it touches the spool directory, thus ensuring that it the
> touch of the spool directory will use a later modification time than the
> creation of the temporary file.
>
> Note that crontab -r is not affected by this race as it only does one atomic
> update to the directory (unlink()).
>
> Index: crontab.c
> ===================================================================
> --- crontab.c (revision 225431)
> +++ crontab.c (working copy)
> @@ -604,6 +604,15 @@ replace_cmd() {
>
> log_it(RealUser, Pid, "REPLACE", User);
>
> + /*
> + * Creating the 'tn' temp file has already updated the
> + * modification time of the spool directory. Sleep for a
> + * second to ensure that poke_daemon() sets a later
> + * modification time. Otherwise, this can race with the cron
> + * daemon scanning for updated crontabs.
> + */
> + sleep(1);
> +
> poke_daemon();
>
> return (0);
Maybe this is overly pedantic, but that solution still allows the
possibility of the same sort of race if a user updates their crontab in
the same second as an admin saves a new /etc/crontab, because cron takes
the max timestamp of /etc/crontab and /var/cron/tabs and compares it
against the database-rebuild timestamp.
A possible solution on the daemon side of things might be something like
the attached, but I should state (nay, shout) that I haven't looked
beyond these few lines to see if there are any unintended side effects
to such a change.
-- Ian
--=-8C+JykMdlr7Y4wfwfSp0
Content-Disposition: inline; filename="cron-database.patch"
Content-Type: text/x-patch; name="cron-database.patch"; charset="us-ascii"
Content-Transfer-Encoding: 7bit
diff -r eb5f4971de86 usr.sbin/cron/cron/database.c
--- usr.sbin/cron/cron/database.c Fri Jan 20 16:12:15 2012 -0700
+++ usr.sbin/cron/cron/database.c Tue Jan 31 10:48:32 2012 -0700
@@ -72,7 +72,7 @@ load_database(old_db)
* so is guaranteed to be different than the stat() mtime the first
* time this function is called.
*/
- if (old_db->mtime == TMAX(statbuf.st_mtime, syscron_stat.st_mtime)) {
+ if (old_db->mtime > TMAX(statbuf.st_mtime, syscron_stat.st_mtime)) {
Debug(DLOAD, ("[%d] spool dir mtime unch, no load needed.\n",
getpid()))
return;
@@ -83,7 +83,7 @@ load_database(old_db)
* actually changed. Whatever is left in the old database when
* we're done is chaff -- crontabs that disappeared.
*/
- new_db.mtime = TMAX(statbuf.st_mtime, syscron_stat.st_mtime);
+ new_db.mtime = 1 + TMAX(statbuf.st_mtime, syscron_stat.st_mtime);
new_db.head = new_db.tail = NULL;
if (syscron_stat.st_mtime) {
--=-8C+JykMdlr7Y4wfwfSp0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1328032670.1662.333.camel>