On Thu, 15 Apr 2010 20:05:17 +0400
Eugene Indenbom <eindenbom(a)gmail.com> wrote:
On 04/15/2010 07:16 PM, Simo Sorce wrote:
> On Thu, 15 Apr 2010 17:58:18 +0400
> Eugene Indenbom<eindenbom(a)gmail.com> wrote:
>
>
>> On 04/15/2010 05:35 PM, Simo Sorce wrote:
>>
>>> On Thu, 15 Apr 2010 09:14:37 -0400
>>> Simo Sorce<ssorce(a)redhat.com> wrote:
>>>
>>>
>>>
>>>> I know the sdap_handle destructor() will end up caling it
>>>> (through sdap_handle_release(), but then why all the games with
>>>> the final argument ? Why not simply attach a destructor to
>>>> sdap_handle_data that prevents destruction until it is safe ?
>>>> Akso given we can prevent releasing sdap_handle too early through
>>>> the destructor, why not simply apply all this to sdap_handle
>>>> directly w/o requireing another sub-structure ?
>>>>
>>>>
>>> Ah btw, while thinking about how you handle release of
>>> sdap_handle_data I think I found a case where you end up with some
>>> issues.
>>>
>>> If you free sdap_handle while sdap_handle_data is locked
>>> sdap_handle_release will be called with final = ture, and this
>>> means sh->private is set to NULL.
>>> If later on sdap_handle_release is called again shd will be NULL
>>> (shd = sh->private) and sdap_handle_data_lock(shd); wiull
>>> derefernce a NULL pointer causing a segfault.
>>>
>>>
>>>
>> No, it will not. sdap_handle_data->sh is never dereferenced. :-)
>> As I have written earlier it is simply a flag showing that
>> sdap_handle_data still have a parent.
>> When sdap_handle_data->sh is zero the structure deletes itself as
>> soon as sdap_handle_data->destroy_lock is zero too (see
>> sdap_handle_data_release).
>>
>> Shall ever back pointer sdap_handle_data->sh be dereferenced, it
>> needs to be checked for NULL before use. That's clear.
>>
> Sorry, I don't understand your comment, I never once mentioned
> sdap_handle_data->sh
>
Oops, I have got your concern wrong for the first time.
sh->private is set to NULL when and only when final == true, that
means we are called from sdap_handle_destructor, so when immediately
after that sdap_handle will be deleted by talloc.
So there is no chance of having sdap_handle where
sdap_handle->private = NULL. I have set sdap_handle->private to NULL
merely for housekeeping to completely break bi-link sdap_handle->sh
<=> sdap_handle_data->shd.
Actually it looks like that sequence of actions during destruction is
not clear.
Well, it's common case for me. I take a lot of complicated staff for
granted.
The sequence of actions in recursive destruction sequence:
1. sdap_process_result is called
2. sdap_handle_release(final = false) is called
3. conncb destroyed
3. op->callback is called
4. talloc_zfree(gsh) is called
5. sdap_handle_destroy is called
6. sdap_handle_release(final = true) is called
7. LDAP handle destroyed
8. sh->private = NULL, shd->sh = NULL
9. sdap_handle_release(final = true) returns
10. sdap_handle_destroy returns
=> 11. sdap_handle is destroyed by talloc (so there is no chance of
dereferencing sh->private)
12. op->callback returns
=> 13. sdap_handle_data_release destroys sdap_handle_data
14. sdap_handle_release(final = false) returns
15. sdap_process_result returns
The sequence of actions in normal destruction sequence:
1. sdap_handle_destroy is called
2. sdap_handle_release(final = true) is called
3. conncb destroyed
4. LDAP handle destroyed
8. sh->private = NULL, shd->sh = NULL
=> 9. sdap_handle_data_release destroys sdap_handle_data
NB that's why I set sh->private = NULL, here sdap_handle_data is
destroyed first
10. sdap_handle_release(final = true) returns
11. sdap_handle_destroy returns
=> 12. sdap_handle is destroyed by talloc (so there is no chance of
dereferencing sh->private)
Therefore setting both sh->private and shd->sh to NULL serves the
purpose of marking relationship between them broken. So if any code
needs to be executed after that (currently there is no such code) is
could check whether sdap_handle_data is orphan or sdap_handle is
almost destroyed so it does not even have inner sdap_handle_data
object.
Thanks for posting the sequence, I managed to come up with it before
though, and there is a case you are not considering.
You said you want to be resistant to random callback freeing the
sdap_handle, so this is the sequence that still gives me trouble:
1. sdap_process_result is called
2. sdap_handle_release(final = false) is called
3. conncb destroyed
3. op->callback is called
4. talloc_zfree(gsh) is called
5. sdap_handle_destroy is called
6. sdap_handle_release(final = true) is called
7. netx op->callback is called
8. talloc_zfree(gsh) is called (2nd time .. not good)
I don't see how you prevent this in your code, given ctx->gsh is zeroed
only when talloc_zfree() returns.
This is why I very much would prefer to avoid recursive loops within
sdap_handle_release rather than just try to use locks to deal with
them, and hence why the rule to never free ctx->gsh.
One way to do that is to prevent freeing ctx->gsh by returning -1 in
the destructor if we are recursing and checking the talloc_free()
return to decide whether to zero ctx->gsh or not depending on whether
the free was successful.
Another way, maybe, is to NULL ctx->gsh before actually freeing it, but
this again is a special rule on how to free ctx->gsh, so we can more
easily just ban freeing ctx->gsh as I was trying to do in my patches.
...
ok, I guess another way is to make sure we loop over shd->ops only in
the outermost sdap_handle_release() ... I guess we can do that using a
flag to be set the first time we enter sdap_handle_release ...
Comments anyone ?
Simo.
--
Simo Sorce * Red Hat, Inc * New York