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
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) {
_______________________________________________ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"