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.
Can you also open a ticket to track fixing the hierarchy in 1.10?
https://fedorahosted.org/sssd/ticket/1575
Thank you!
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
