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.
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
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.
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)
- There should be no restriction on when method starting
destruction is called
- 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.
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.
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.