[SSSD] [PATCH] fix early free of sdap_handle

Eugene Indenbom eindenbom at gmail.com
Wed Apr 14 13:34:39 UTC 2010


On 04/14/2010 04:51 PM, Simo Sorce wrote:
> On Wed, 14 Apr 2010 10:48:28 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>
>    
>> +    /* private data is allocated as top-level talloc context as
>> +     * it may need to survive for a while after main handle
>> destruction,
>> +     * see sdap_handle_data_lock/sdap_handle_data_release */
>> +    sh->private = talloc_zero(NULL, struct sdap_handle_data);
>> +    if (!sh->private) {
>> +        goto fail;
>> +    }
>> +
>> +    sh->private->sh = sh;
>>       return sh;
>> +
>>      
> Sorry Eugene, but I *really* do not like things like this.
> - Allocating on NULL: very high risk of memory leaks
>    
We can allocate on sdap_handle_data on parent sdap_handle context and 
later use talloc_steal to NULL context when we need to keep pointer to 
sdap_handle_data until stack unwinds.

So normally sdap_handle_data would be owned, but when it orphan it will 
become orphan in TALLOC_CTX sense too.

But I do not see any risk here: there is a destructor set on sdap_handle 
that take cares of destruction of sdap_handle_data. There is no way to 
get this out of control.
> - Assigning sh to private->sh: very high risk of accessing free memory
>    if sh is freed before private is.
>    
private->sh is actually a boolean saying whether private data is orphan 
or not. I see no reason not to have a pointer to owner at hand, but this 
is not necessary.

And again there is no risk of having stale pointer here as there is a 
destructor set on sdap_hanlde, that takes care of clearing the pointer 
on time.


> If preserving the handle is important I'd rather have a list of
> handles, with old ones pushed down the list as soon as a connection is
> marked bad so that they are not reused, but also not lost (memory wise).
>
>    

So it looks like you do not really believe that talloc destructors are 
always called and it's reliable to free memory in destructors. In my 
humble opinion if memory deallocation is controlled from destructor 
there is actually no actual need for talloc ownership.

You suggestion of putting sdap_handle objects on destroy queue and 
garbage collecting them later does not sound as better alternative.

On 04/14/2010 05:00 PM, Simo Sorce wrote:
> On Wed, 14 Apr 2010 10:48:28 +0400
> Eugene Indenbom<eindenbom at gmail.com>  wrote:
>    
>> 4. The suggested patch is a workaround. It relies on the agreement
>> that all _send functions will be called from top level event context,
>> not from subreq callback context. This is hard to guarantee.
>>      
> It is easier to analyze and agree on for now.
> It solves the problem, without changing the underlying logic.
> This is a big plus for a patch to go into a stable tree as it will not
> cause surprises.
>
>    
That's wrong your patch is changing behaviour too:
1. It activates code paths that segfaulted before.
2. It changes time when sdap_handle is deallocated (and all its child 
objects like LDAP handle). This way it substantially changes memory 
distribution, object life time and other key parameters.

My patch behaves better in this sense. It at least does not change time 
when LDAP handle is deallocated. Actually my patch affects only 
sdap_handle destruction sequence and only in previously faulty case of 
deletion recursion.

Regards, Eugene

PS Well, it's your project anyway and it's up to you and the project 
team to decide on which way to take.



More information about the sssd-devel mailing list