Re: Race between cron and crontab

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)
> > _at__at_ -604,6 +604,15 _at__at_ 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