From owner-freebsd-current@FreeBSD.ORG Tue Jan 31 17:57:53 2012 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AC8211065676 for ; Tue, 31 Jan 2012 17:57:53 +0000 (UTC) (envelope-from freebsd@damnhippie.dyndns.org) Received: from qmta04.emeryville.ca.mail.comcast.net (qmta04.emeryville.ca.mail.comcast.net [76.96.30.40]) by mx1.freebsd.org (Postfix) with ESMTP id 879368FC1A for ; Tue, 31 Jan 2012 17:57:53 +0000 (UTC) Received: from omta05.emeryville.ca.mail.comcast.net ([76.96.30.43]) by qmta04.emeryville.ca.mail.comcast.net with comcast id UHr31i0020vp7WLA4Hxtjs; Tue, 31 Jan 2012 17:57:53 +0000 Received: from damnhippie.dyndns.org ([24.8.232.202]) by omta05.emeryville.ca.mail.comcast.net with comcast id UHxs1i00K4NgCEG8RHxsrh; Tue, 31 Jan 2012 17:57:53 +0000 Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id q0VHvoxV020505; Tue, 31 Jan 2012 10:57:50 -0700 (MST) (envelope-from freebsd@damnhippie.dyndns.org) From: Ian Lepore To: John Baldwin In-Reply-To: <201201311149.32760.jhb@freebsd.org> References: <201201311149.32760.jhb@freebsd.org> Content-Type: multipart/mixed; boundary="=-8C+JykMdlr7Y4wfwfSp0" Date: Tue, 31 Jan 2012 10:57:50 -0700 Message-Id: <1328032670.1662.333.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.26.0 FreeBSD GNOME Team Port Cc: current@freebsd.org Subject: Re: Race between cron and crontab X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Jan 2012 17:57:53 -0000 --=-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--