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>