Re: Race between cron and crontab

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.

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