patch for lock naming problems, needs review

[Eric Sorenson]

Hi Mark & list --

A while back when I was looking into the lock name algorithm, I wrote:

[ http://lists.gnu.org/archive/html/bug-cfengine/2005-01/msg00002.html ]

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

I got down-n-dirty with ddd this weekend and figured out what's going on.
CanonifyName is not reentrant, and therefore not thread-safe, because it 
uses a static declaration for its buffer. Calling it twice in a row returns
unreliable results, in this case the string "directories" (the value of 
'operator') for both invocations.  

The attached patch changes the 'static char buffer[CF_BUFSIZE]' to 
'char *buffer' and then dynamically mallocs some space for it.  My
locknames with this in place, running the same cfagent.conf as above,
finally look sane:

    SetLock(lock.cfagent_conf.amine.directories._var_tmp_maildir_new___4647)

However -- CanonifyName gets called all over the place and I am not free()ing
here. It should be the calling function's responsibility to free() once its
through with the returned pointer and I'm not sure this patch represents the
best solution, so I haven't gone through and done that to every place that
calls CanonifyName. So this patch introduces a memory leak that might matter
for huge configs.  But I am sending it on to illustrate the problem and 
hopefully provide a starting point for a more elegant fix.

As I understand it, the alternatives to caller-free() are either to lock inside
the function with mutexes, or (and this is how it should really be done) to
provide caller-supplied storage for the returned results, a'la gethostbyname_r
and friends.  That is a big change though, especially when you consider this as
just one -- although probably the most commonly hit -- of several non-reentrant
functions. 

Any suggestions on the right path forward?

(For other novitiates into thread-safety, I found this very helpful: 
    http://xrl.us/eukw )

-- 

 - Eric Sorenson - Explosive Networking - http://eric.explosive.net -

canonifyname-malloc.patch
Description: Text document