[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:
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- patch for lock naming problems, needs review,
Eric Sorenson <=