[SSSD] [PATCH] Do not keep growing event context

Simo Sorce simo at redhat.com
Fri Apr 19 18:14:21 UTC 2013


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 at 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




More information about the sssd-devel mailing list