Re: patch for lock naming problems, needs review

[Mark Burgess]

Eric, I am still unsure what to do here. Where is it you think this needs to be
re-entrant?

M

Quoting Eric Sorenson <address@hidden>:

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


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


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.