[SSSD] [PATCH] do not call dp callbacks when responder is shutting down

Jakub Hrozek jhrozek at redhat.com
Thu Oct 11 10:29:51 UTC 2012


On Thu, Oct 11, 2012 at 12:24:37PM +0200, Jakub Hrozek wrote:
> On Thu, Oct 11, 2012 at 10:29:43AM +0200, Pavel Březina wrote:
> > On 10/10/2012 11:29 PM, Jakub Hrozek wrote:
> > >On Tue, Oct 09, 2012 at 03:32:48PM +0200, Pavel Březina wrote:
> > >>On 10/09/2012 02:58 PM, Stephen Gallagher wrote:
> > >>>On 10/09/2012 07:43 AM, Jakub Hrozek wrote:
> > >>>>On Tue, Oct 02, 2012 at 03:39:19PM +0200, Pavel Březina wrote:
> > >>>>>https://fedorahosted.org/sssd/ticket/1514
> > >>>>>
> > >>>>>See the commit message and ticket comments for more information.
> > >>>>>
> > >>>>>This patch introduces one of the possible solutions, but I suggest
> > >>>>>to consider actually changing memory hierarchy in responders.
> > >>>>
> > >>>>Instead of creating the boolean flag, what about iterating over the
> > >>>>dp_requests table in the sss_responder_ctx_destructor, cancel the
> > >>>>pending requests and remove the original sss_dp_req_destructor ?
> > >>>>
> > >>>>>
> > >>>>>Currently, the hierarchy looks like this:
> > >>>>>main_ctx->specific_ctx->rctx, where specific_ctx is one of the pam,
> > >>>>>nss, sudo, etc. contexts.
> > >>>>>
> > >>>>>Does anybody know why it is not main_ctx->rctx->specific_ctx? This
> > >>>>>would solve the issue as well and I personally think that it is more
> > >>>>>logical.
> > >>>>
> > >>>>Hmm, I don't actually see any reason why the responder context is
> > >>>>allocated below the specific context either.
> > >>>>
> > >>>
> > >>>Historically, I think it's just because the responder context as a
> > >>>concept came much later. Originally we only had the specific contexts,
> > >>>but we later refactored the common code. When we did that, we set the
> > >>>hierarchy for the common code based on execution order. (The specific
> > >>>context is created first, therefore we made the responder context a
> > >>>child of that).
> > >>>
> > >>>I am not aware of any particular reason that we could not reverse this
> > >>>at this point.
> > >>>
> > >>>>I discussed the hierarchy some more with Pavel and the proposal is to
> > >>>>modify the hierarchy as follows:
> > >>>>
> > >>>>     main_ctx --> rctx +--> dp_requests
> > >>>>                       |
> > >>>>                       +--> specific_ctx
> > >>>>
> > >>>>And then set a desctructor to dp_requests so that it is freed before
> > >>>>specific_ctx is as destructors of the DP requests might need to access
> > >>>>data from the specific_ctx.
> > >>>>
> > >>>
> > >>>That's an overly complex solution. What's wrong with doing
> > >>>main_ctx->rctx->specific_ctx->dp_requests? Yes, I realize that
> > >>>dp_requests is a component of the rctx struct, but as long as we add
> > >>>comments that its memory hierarchy is maintained as a child of the
> > >>>specific_ctx, I think we'd be fine.
> > >>
> > >>First of all it breaks everything I wrote in the tutorial :-)
> > >>But more importantly - it doesn't solve the problem.
> > >>
> > >>What we want is to assure that dp_requests table is deallocated before
> > >>specific_ctx. Imagine this situation:
> > >>
> > >>rctx --> specific_ctx +--> ptr_1
> > >>                       |
> > >>                       +--> dp_requests
> > >>                       |
> > >>                       +--> ptr_n
> > >>
> > >>Some callback from dp_requests is accessing ptr_1.
> > >>
> > >>talloc_free(rctx) will recurse down to talloc_free(specific_ctx), which
> > >>will free it's children. In this order it will free ptr_1, then
> > >>dp_requests. This will trigger the callback and we will access already
> > >>deallocated data (ptr_1).
> > >>
> > >>The destructors in general can be used only to access children of the
> > >>context. Not it's brothers, because you don't know the order in which
> > >>they are deallocated. You can rely on the fact that it is stored in
> > >>a linked list, but that is low level implementation detail I wouldn't
> > >>count on.
> > >
> > >Pavel, while I agree with fixing the hierarchy, this change would be too
> > >intrusive to do in 1.9 at this point.
> > >
> > >Can you either fix your original patch or explain why your approach is
> > >better than cleaning the dp_requests? I'd like to include that change it
> > >1.9 to prevent the crash on shutdown and then fix the hierarchy in 1.10.
> > 
> > The dp request destructor still makes sense, because it is
> > cancelling dbus reply. So we would basically replace one destructor
> > (cancel dbus reply + call callbacks + remove from hash table) with
> > new one (cancel dbus reply). I think it is better to have only one
> > destructor to maintain.
> 
> OK, that makes sense. I was trying to come up with a solution that would
> call the destructor automatically without having to rely on a flag, but
> code duplication is bad.
> 
> Ack

Pushed to master.



More information about the sssd-devel mailing list