From owner-freebsd-current@FreeBSD.ORG Tue Jan 31 18:42:29 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 7B2BA1065787 for ; Tue, 31 Jan 2012 18:42:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 6CAA58FC0C for ; Tue, 31 Jan 2012 18:42:28 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 0742846B17; Tue, 31 Jan 2012 13:42:28 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 7A174B960; Tue, 31 Jan 2012 13:42:27 -0500 (EST) From: John Baldwin To: Ian Lepore Date: Tue, 31 Jan 2012 13:30:15 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <201201311149.32760.jhb@freebsd.org> <1328032670.1662.333.camel@revolution.hippie.lan> In-Reply-To: <1328032670.1662.333.camel@revolution.hippie.lan> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201201311330.15480.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 31 Jan 2012 13:42:27 -0500 (EST) 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 18:42:29 -0000 On Tuesday, January 31, 2012 12:57:50 pm Ian Lepore wrote: > 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. Hmm, I'm not sure I see the race in that case. If the /etc/crontab file matches the timestamp of the spool directory before the utimes() call after the one-second sleep, then it will still rescan it on the next check when it notices a newer timestamp on the spool directory. If it is the same timestamp as the second timestamp on the spool directory, then the scan is guaranteed to have not started before that second began, meaning that the crontab(8) process editing the user's crontab must have passed the rename, so the scan will see the user's new crontab. > 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. I think this patch doesn't change anything at all actually. It is certainly subject to the original race I described if you do not use the patch in crontab(8) itself. -- John Baldwin