RE: Definitely a bug in locking.

[Mark Burgess]

Please try the following patch for this problem

http://svn.iu.hio.no/viewcvs/trunk/src/locks.c?rev=55&r1=54&r2=55


M


On Mon, 2005-03-21 at 18:18 -0500, Cole, William wrote:
> 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 -
> >