On 04/15/2010 05:14 PM, Simo Sorce wrote:
On Thu, 15 Apr 2010 10:13:46 +0400
Eugene Indenbom<eindenbom(a)gmail.com> wrote:
> On 04/13/2010 08:51 PM, Simo Sorce wrote:
>
>> This patch attempts to address the problem Eugene found with
>> sdap_handle_release()
>>
>> Simo.
>>
>>
>>
> I have found 2 critical problems in Simo's patch:
>
> 1. After setting sdap_handle->connected = false, LDAP handle is never
> destroyed. (see sdap_handle_release). So actually patch introduces
> huge resource leak.
>
Ahh, thanks for catching this one, I forgot to remove the sh->connected
check in sdap_handle_release()
> 2. Despite all efforts not to free sdap_handle from callbacks it is
> still freed when connect operation fails (together with connection
> tevent_req). So the following sequence still reproduces the bug:
> a. Using debugger break in sdap_cli_rootdse_step
> b. Restart LDAP server or network connection leading to it
> c. Resume execution
> d. Observe crash
>
This looks like a separate issue. Within the connect functions we are
not using the global sdap_handle. I guess the issue here is that until
we return from the connect request, the sh is a child of the request
memory hierarchy, so when we call the callback within
sdap_handle_release() we end up freeing the hierarchy.
Do you confirm ?
Yes.
> Actually I still do not understand why it is feasible instead of
> fixing destruction of sdap_handle in callback to invent a workaround
> trying to avoid doing so.
>
I am not sure why you call it a workaround, it is just a different
solution.
> The bug in destruction of sdap_handle in callback violates basic
> programming principles:
> - Every creatable object must by destructible (or in case of
> reference counted object it must be possible to release
> ownership/reference from object)
>
This is not really true in a program based on talloc, as you can see a
talloc hierarchy as a set of references. We do indeed almost
exclusively base our destruction sequences on talloc hierarchies.
The only case where this is not entirely possible is indeed the
interface code between the program and the ldap libraries (for obvious
reasons), and I want to keep the difference restricted in an area as
small as possible. One of the reasons I do not like your approach is
that the talloc hierarchy become very fuzzy.
I agree to this principle, but some exceptions must be allowed at least
for cases like this when we need to delay destruction of structure
holding head of callback list.
NB In my patch only sdap_handle_data memory block is allocated on NULL
context. All others are allocated on talloc hierarchy. It is absolutely
required as this memory block is used by callback iteration. Therefore
it is owned not only by sdap_handle itself, but by each stack frame
iterating over callbacks (sdap_handle->ops member). This stack frame
ownership is taken into account by destroy_lock, simple reference
counting variable. There is no room to screw the things up here.
The whole technique is stock and widely used design pattern. I really do
not understand why you are so reluctant to accept it. It's much simpler
and cheaper in future maintenance than crasy rules on when it is allowed
and when it is not allowed to destroy sdap_handle.
> - There should be no restriction on when method starting
> destruction is called
>
Not sure where you got this, but it is certainly not true :)
You can't destroy objects in use. If you are thinking about deferred
destruction that is what we want to achieve but using correct talloc
hierarchies as much as possible. Unfortunately this is not the case,
because of the interfcae we are forced into with the openldap libraries.
If object is busy (like in our case) it simply starts destruction and
completes it as soon as all dependent operations complete and it is not
busy any longer.
> - After object destruction is started it is object's
> responsibility to complete destruction as soon as possible. There
> should not be any tricks like keeping an object in garbage collecting
> queue.
>
Unfortunately this is not possible as well, because pending requests
all reference a common object (the one that ultimately holds the ldap
handler).
Yes, it is possible, and more over implemented in my patch. The only
memory block left over and destroyed later after talloc_zfree is
sdap_handle_data. All handles, file descriptors and in particular LDAP
handle are destroyed or closed.
> Violation of the above principles calls for __path__ coverage
> analysis of code as we have to ensure that no __path__ of code
> destructs object at wrong time. Even if Simo fixes the above problems
> there will be no guarantee that sdap_handle destruction is never
> called from callback.
>
We just need to guarantee this, I will check again if I can steal some
code from your patch though :)
It's hopeless. There always will be a code path we overlook.
> Regards, Eugene
>
> PS Although my patch never sets sdap_handle->connected to false, I
> have attached an amendment to my patch to improve robustness of
> destruction code when somebody really sets sdap_handle->connected to
> false.
>
> PPS Even without this amendment no resource leaks exist in my patch
> as all memory and callbacks are a bit later destroyed through talloc
> ownership.
>
Given you alloc private on NULL, you can hardly claim it is released
through a talloc hierarchy, but it is explicitly freed in
sdap_handle_data_release().
Yes, I claim that my patch follow the talloc hierarchy. The only
exception is sdap_handle_data memory block, which is freed when stack
unwinds to top-level callback iterator. The exception is well defined
and managed by 2 simple methods
(sdap_handle_data_lock/sdap_handle_data_release).
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 ?
Because, sdap_handle_destructor and sdap_handle_release return
__before__ sdap_handle_data is freed. All other objects are freed
(sdap_handle itself, LDAP handle, callback data) and all aborted
operations are notified. Only this sdap_handle_data memory block
survives the destructor, because it is needed to signal end of callback
iteration to outer sdap_handle_release.
The final argument signals that sdap_handle_data needs to be detached
from parent sdap_handle and go into orphan state. In orphan state is
self-destroys as soon as stack unwinds to top-level sdap_handle_release.
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 ?
Because:
1. sdap_handle is not an opaque data structure. It's pointer is known to
outside code which can accidentally call talloc_free. It's easier to
allow calling talloc_free than to find all places where it is called. It
also reduces amount of changed legacy code.
2. I want to have regular talloc parent for all other sub-objects of
sdap_handle. This way we exclude from hierarchy only one memory block.
This makes the solution more simple and robust.
Eugene