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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
