Re: bugs in GetLock() - lockname collisions

[Mark . Burgess]

Thanks for this patch

M

On 11 Jan, Eric Sorenson wrote:
> I tracked down a bug today that has probably been affecting people for a 
> while. 
> The symptom is that an action in the middle of execution will not be run 
> because
> a spurious lock exists.
> 
> The following code snippet will expose the bug:
> 
>   control:
>     actionsequence = ( directories )
> 
>   directories:
> 
>     /var/tmp/maildir mode=700 owner=root group=root
>     /var/tmp/maildir/cur mode=700 owner=root group=root
>     /var/tmp/maildir/new mode=700 owner=root group=root
>     /var/tmp/maildir/tmp mode=700 owner=root group=root
> 
> The output looks like:
> 
>   MakePath(/var/tmp/maildir)
>   MakePath(/var/tmp/maildir/cur)
>   MakePath(/var/tmp/maildir/new)
>   cfengine:: Nothing scheduled for directories.directories (0/1 minutes 
> elapsed)
>   MakePath(/var/tmp/maildir/tmp)
>   
> The problem is in the way cfengine creates the lockname for these actions.
> It does the following in locks.c about line 200:
> 
> for (sp = operator; *sp != '\0'; sp++)
>    {
>    sum += (int)*sp;
>    }
> 
> And then does the same thing for 'operand'. (operator is the action
> being performed, and operand the pathname that we're working on)
> This is very prone to namespace collisions; for instance 'cur' and
> 'new' both end up with the same value for sum.
> 
> Since there's already a nice non-colliding algorithm in use for 
> SplayTime, the attached patch just implements it instead.
> 
> There is another odd problem though, that I was not able to patch.
> If you look at the lockname for a 'directories:' action, it's something 
> like:
> 
>     lock.cfagent_conf..directories.directories_3492
> 
> This is the value of 'CFLOCK', which is consists of (80-col formatted):
> 
>     snprintf(CFLOCK,CF_BUFSIZE,"lock.%.100s.%.40s.%s.%.100s_%d",
>       VCANONICALFILE, host,CanonifyName(operator),
>       CanonifyName(operand),sum);
> 
> So 'host' is empty, and both operator and operand are set to 'directories'.
> The above checksum problem wouldn't even be looked at if operand were actually
> the pathname of the directory in question (and, of course, that pathname was
> less than 100 characters -- so it's still useful to have the 'sum' fix). I
> stepped through this in gdb but couldn't see why this was happening. Both
> CFLOCK and CFLAST do the same thing (this is locks.c:214).
> 



~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Work: +47 22453272            Email:  Mark.Burgess@iu.hio.no
Fax : +47 22453205            WWW  :  http://www.iu.hio.no/~mark
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~