Re: Race between cron and crontab

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.)

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.

-- Ian