On Mon, Mar 21, 2011 at 01:47:36PM -0400, Jared Yanovich wrote:
> On Mon, Mar 21, 2011 at 10:26:00AM +0100, Gilles Chehade wrote:
>
> > The original asr.c code looks ok to me, can you explain why you think it has
> > an error ?
>
> Experimentally, we traced that asr was not getting freed under certain
> circumstances.
>
> Theoretically, the pointer is lost and the asr argument to that routine
> will not be freed since the only reference to an instance of asr is
> held by the single global pointer asr in dns.c. So if there is ever
> a situation where the asr_ctx_unref() returns zero, asr leaks.
>
> Try tracing the refcount in asr_done()...
> > > Index: asr.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/smtpd/asr.c,v
> > > retrieving revision 1.2
> > > diff -u -p -r1.2 asr.c
> > > --- asr.c 3 Dec 2010 23:29:08 -0000 1.2
> > > +++ asr.c 19 Mar 2011 19:48:33 -0000
> > > @@ -774,8 +774,7 @@ asr_ctx_query(struct asr_ctx *ac, int ty
> > > void
> > > asr_done(struct asr *asr)
> > > {
> > > - if (asr_ctx_unref(asr->a_ctx))
> > > - return;
> > > + asr_ctx_unref(asr->a_ctx);
> > > if (asr->a_path)
> > > free(asr->a_path);
> > > free(asr);
This looks right, indeed. I'll check the logic again, but yeah, I
think asr_done() should always free the structure. The error comes
from the fact that asr and asr_ctx used to be the same structure, so
the refcount was actually on the asr struct. But it is not true
anymore.
The dns.c code is still not correct though. It should not have to
check for resolv.conf update as this is done internally by asr. I'll
send a patch.
Eric.
