On Mon, 2013-04-15 at 15:48 +0200, Jakub Hrozek wrote:
On Thu, Apr 11, 2013 at 03:10:36PM +0200, Jakub Hrozek wrote:
> On Thu, Apr 11, 2013 at 09:00:52AM -0400, Simo Sorce wrote:
> > On Thu, 2013-04-11 at 09:45 +0200, Jakub Hrozek wrote:
> > > I found this bug when testing the AD dyndns updates.
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > plain text
> > > document
> > > attachment
> > > (0001-Do-not-keep-growing-event-context.patch)
> > >
> > > From 2bf384857360b18c84863f20cebfa82facc1c010 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek <jhrozek(a)redhat.com>
> > > Date: Mon, 1 Apr 2013 16:09:59 +0200
> > > Subject: [PATCH] Do not keep growing event context
> > >
> > > ---
> > > src/util/child_common.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/util/child_common.c b/src/util/child_common.c
> > > index
> > >
5f0b2659571731a70211df310648a5cc8aee241c..fb001f2c4dfac979cb3e2412b983d8487373e88b 100644
> > > --- a/src/util/child_common.c
> > > +++ b/src/util/child_common.c
> > > @@ -172,6 +172,8 @@ static void sss_child_invoke_cb(struct
> > > tevent_context *ev,
> > > if (child_ctx->cb) {
> > > child_ctx->cb(child_ctx->pid, cb_pvt->wait_status,
> > > child_ctx->pvt);
> > > }
> > > +
> > > + talloc_free(imm);
> > > }
> > >
> > > void sss_child_handler(struct tevent_context *ev,
> > > @@ -561,7 +563,7 @@ void child_sig_handler(struct tevent_context *ev,
> > > return;
> > > }
> > >
> > > - tevent_schedule_immediate(imm, ev,child_invoke_callback,
> > > + tevent_schedule_immediate(imm, ev, child_invoke_callback,
> > > child_ctx);
> > > }
> > >
> > > @@ -580,6 +582,7 @@ static void child_invoke_callback(struct
> > > tevent_context *ev,
> > >
> > > /* Stop monitoring for this child */
> > > talloc_free(child_ctx);
> > > + talloc_free(imm);
> > > }
> >
> > Why not simply make the imm context a child of the child_ctx it is freed
> > just over here ?
> >
> > Why is it made child of the event context at all ?
> >
> > Simo.
>
> I will check the code again but I think it was because we wanted to be
> on the safe side and make sure the context is still valid even in case
> of timeouts etc. so we allocated it on the event context which is always
> available and long lived.
>
> I'll check and change the code if appropriate.
I didn't find any reason the immediate context could outlive the
child_ctx so I allocated the immediate context on the child_ctx.
But in the case of the sss_child_invoke_cb() I think it is still better to
free the handler there because the caller owns the child_ctx and I don't
think we should grow it over time. In the case of child_invoke_callback
the child_ctx is released there, so we're good.
New patch is attached.
Looks good to me.
Simo.
--
Simo Sorce * Red Hat, Inc * New York