[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

patch for lock naming problems, needs review

From: Eric Sorenson
Subject: patch for lock naming problems, needs review
Date: Sun, 23 Jan 2005 23:19:26 -0800 (PST)

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:


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

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 -

Attachment: canonifyname-malloc.patch
Description: Text document

reply via email to

[Prev in Thread] Current Thread [Next in Thread]