Re: Race between cron and crontab

On Tuesday, January 31, 2012 3:13:34 pm Ian Lepore wrote:
> On Tue, 2012-01-31 at 13:30 -0500, John Baldwin wrote:
> > 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.
> > 
> 
> You're right about my patch not fixing anything; I didn't think hard
> enough before I started typing.  
> 
> But I think the problem I was trying to get at with /etc/crontab still
> exists with your patch; it would be triggered if a user updated their
> crontab and after the 1 second sleep the directory timestamp gets
> updated and cron rebuilds the database, and then right after that but
> still in the same second /etc/crontab gets written.  (That's why I was
> trying, however feebly, to move the solution into the daemon.)

What I would do for this case is change the daemon to remove all the TMAX
crap and just cache both timestamps and rebuild the database any time
either one changes.

> Maybe the simple answer is for admins to be sure not to save changes
> to /etc/crontab during the xx:xx:00 second, or with your patch, :01.
> (I'm kidding, of course.  The fact that cron likes to wake up at the top
> of minute is no g'tee that it actually does so every time.)
> 
> Once you start thinking along the "no g'tee" lines you realize that two
> users can be updating their tabs concurrently, and one arrives in
> poke_daemon() and grabs the current time (let's say it's noon) then gets
> preempted for a long while before calling utimes(), the other user runs
> through poke_daemon() and updates the directory timestamp to 12:00:01,
> then the first process gets some cycles again and re-updates the
> timestamp to 12:00:00.  Ooopsie, we've achieved a complex and annoying
> proof of the idea that timestamps alone aren't an adequate
> synchronization primitive.

Yes, a change id similar to what NFSv4 mandates is what you really want,
but stat doesn't return one of those.  However, the hack to crontab(8)
should at least help with cases observed in the wild.

-- 
John Baldwin