Eric, I am still unsure what to do here. Where is it you think this needs to be re-entrant?
M Quoting Eric Sorenson <[EMAIL PROTECTED]>: > 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: [EMAIL PROTECTED] Fax : +47 22453205 WWW : http://www.iu.hio.no/~mark ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program. _______________________________________________ Bug-cfengine mailing list [email protected] http://lists.gnu.org/mailman/listinfo/bug-cfengine
