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.
Stephen, you probably know the hierarchy of the DP requests the best,
what do you think?
It's been a while, and I think you touched it more recently than I have,
with the sss_dp_issue_request() stuff, but I think the above makes sense.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
