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:
>>>> 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
>>>> 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
>>> 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.