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

Jakub Hrozek jhrozek at redhat.com
Thu Oct 11 10:24:37 UTC 2012


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



More information about the sssd-devel mailing list