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.
Regards, Eugene