RE: Definitely a bug in locking.

[Cole, William]

I *think* I understand static storage again after a decade of almost no C
work and a day pounding on cfengine... 

By declaring 'buffer' as static in CanonifyName, you allocate it once and
forever and never need to free it because every time CanonifyName is called,
it is exactly the same memory block. If you look at what CanonifyName
returns, it should always be exactly the same: a pointer to that one
never-lost static block. That probably means needing to be very careful
about never calling CanonifyName except inside a strncpy, e.g.:

strncpy(c_operator,CanonifyName(operator),CF_BUFSIZE-1);

Of course, in this case the bug was not that line in GetLock which clobbered
'operand', but rather whatever *called* GetLock embedding the CanonifyName
call in the GetLock call. Like one of these lines:


# grep GetLock src/*.c |grep Canon
src/alerts.c:      if
(!GetLock(ASUniqueName("alert"),CanonifyName(ip->name),ip->ifelapsed,ip->exp
ireafter,VUQNAME,CFSTARTTIME))
src/cfagent.c:                if
(!GetLock("Mountres",CanonifyName(VFSTAB[VSYSTEMHARDCLASS]),0,VEXPIREAFTER,V
UQNAME,0))
src/do.c:   if
(!GetLock(ASUniqueName("link"),CanonifyName(VBUFF),lp->ifelapsed,lp->expirea
fter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("link"),CanonifyName(VBUFF),lp->ifelapsed,lp->expirea
fter,VUQNAME,CFSTARTTIME))
src/do.c:if
(!GetLock("Mailcheck",CanonifyName(VFSTAB[VSYSTEMHARDCLASS]),0,VEXPIREAFTER,
VUQNAME,CFSTARTTIME))
src/do.c:      if
(!GetLock(ASUniqueName("diskscan"),CanonifyName(rp->name),ifelapsed,rp->expi
reafter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("disable"),CanonifyName(dp->name),dp->ifelapsed,dp->e
xpireafter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("unmount"),CanonifyName(ptr->name),ptr->ifelapsed,ptr
->expireafter,VUQNAME,CFSTARTTIME))
src/do.c:      if
(!GetLock(ASUniqueName("copy"),CanonifyName(VBUFF),ip->ifelapsed,ip->expirea
fter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("processes"),CanonifyName(VBUFF),pp->ifelapsed,pp->ex
pireafter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("packages"),CanonifyName(ptr->name),ptr->ifelapsed,pt
r->expireafter,VUQNAME,CFSTARTTIME))
src/do.c:   if
(!GetLock(ASUniqueName("methods-dispatch"),CanonifyName(label),ptr->ifelapse
d,ptr->expireafter,VUQNAME,CFSTARTTIME))
src/edittools.c:if
(!GetLock(ASUniqueName("editfile"),CanonifyName(filename),VIFELAPSED,VEXPIRE
AFTER,VUQNAME,CFSTARTTIME))
src/methods.c://if
(!GetLock(ASUniqueName("method-exec"),CanonifyName(ptr->name),ptr->ifelapsed
,ptr->expireafter,VUQNAME,CFSTARTTIME))
src/methods.c:if
(!GetLock(ASUniqueName("method-exec"),CanonifyName(methodname),ptr->ifelapse
d,ptr->expireafter,VUQNAME,CFSTARTTIME))
src/wrapper.c:if
(!GetLock(ASUniqueName("tidy"),CanonifyName(startpath),tp->ifelapsed,tp->exp
ireafter,VUQNAME,CFSTARTTIME))
src/wrapper.c:    if
(!GetLock(ASUniqueName("tidy"),CanonifyName(startpath),tp->ifelapsed,tp->exp
ireafter,VUQNAME,CFSTARTTIME))
src/wrapper.c:    if
(!GetLock(ASUniqueName("tidy"),CanonifyName(startpath),VIFELAPSED,VEXPIREAFT
ER,VUQNAME,CFSTARTTIME))
src/wrapper.c:if
(!GetLock(ASUniqueName("files"),CanonifyName(lock),ptr->ifelapsed,ptr->expir
eafter,VUQNAME,CFSTARTTIME))
src/wrapper.c:if
(!GetLock(ASUniqueName("directories"),CanonifyName(directory),ptr->ifelapsed
,ptr->expireafter,VUQNAME,CFSTARTTIME))






> -----Original Message-----
> From: Eric Sorenson [mailto:address@hidden
> Sent: Monday, March 21, 2005 6:01 PM
> To: Cole, William
> Cc: 'address@hidden'; Baker, Darryl; 'address@hidden'
> Subject: RE: Definitely a bug in locking.
> 
> 
> On Mon, 21 Mar 2005, Cole, William wrote:
> 
> > 
> GetLock(copy,_var_cfengine_master_etc_sudoers__usr_local_etc_s
> udoers_sysadm0
> > 5_abh_vw_com,time=1111156198), ExpireAfter=60, IfElapsed=1
> > > > GetLastLock()
> > > > cfengine:asgqd545: Nothing scheduled for copy. (0/1 
> minutes elapsed) 
> > 
> > The first and last are generated by lines 212 and 257/258 
> respectively in
> > locks.c, inside the GetLock function. The last line is 
> constructed thus:
> > 
> >    snprintf(OUTPUT,CF_BUFSIZE*2,"Nothing scheduled for 
> %s.%s (%u/%u minutes
> > elapsed\n",operator,operand,elapsedtime,ifelapsed);
> > 
> > Note that 'operand' seems to have become null!
> > 
> 
> CanonifyName strikes again!!
> 
> > This test was against a version downloaded 3/14/05 calling 
> itself 2.1.14.
> > The locks.c appears to be identical to the one in the 
> Subversion repository.
> > 
> > 
> > I have just downloaded the latest snapshot and will try 
> running the cfagent
> > from it to see if the same thing happens. 
> 
> There is a change in locks.c , to avoid calling CanonifyName twice 
> from teh same snprintf. do 'svn diff -r PREV locks.c' to see what 
> changed. If the problem still exists, this change did not 
> actually fix 
> the problem with static storage. (About which I understand 
> less than I 
> thought I did a week or two ago)
> 
> -- 
>  - Eric Sorenson - N37 17.255 W121 55.738 - 
> http://eric.explosive.net -
>  - Personal colo with a professional touch - 
> http://www.explosive.net -
>