Memory leaks in smbd ?

[Martin Zielinski]

Thanks for the hints, Jeremy!

I've looked quickly through the code of 3.2.4 and I think there are some 
places that should be fixed.

Some are obvious, some may be wrong as the memory is linked to another 
struct and will be destroyed later.
However it is not allways visible in the code. Sometimes it is freed on 
an error case and later in the same function it is not.

I found that the output of the smbcontrol program attached to my first 
post is caused by a samu - allocation in pdb_getsampwnam ().

I think, that there is a bug in the memcache_add function.
I allready asked Volker Lendecke in the "memory usage" thread on the 
samba mailing list about that!

Attached is a diff against 3.2.4 which (hopefully) points to one or the 
other potential leak.

Thanks,

Martin

Jeremy Allison schrieb:
> On Wed, Nov 12, 2008 at 11:44:52AM +0100, Martin Zielinski wrote:
>> Hello!
>>
>> With samba 3.2.4 (and even 3.0.x) we see growing smbd's in some situations.
>> Using smbcontrol to find the reason gave the (snipped) result attached  
>> to this mail.
>>
>> I'm not familliar with the talloc methods.
>> But the first scans for samu_new brought:
>>
>> winbindd_passdb.c / lookup_usergroups
>> [...]
>> 	if ( (user = samu_new(mem_ctx)) == NULL ) {
>> 		return NT_STATUS_NO_MEMORY;
>> 	}
>>
>> 	if ( !pdb_getsampwsid( user, user_sid ) ) {
>> 		return NT_STATUS_NO_SUCH_USER;
>> [...]
>>
>> looks, like there is a "TALLOC_FREE( user );" missing??
> 
> Yes, this is a leak in an error path. Thanks for pointing that
> out, I'll fix it asap.
> 
>>> or in auth_util.c / create_token_from_username:
>> if (sid_check_is_in_our_domain(&user_sid)) {
>> 	bool ret;
>> 	/* This is a passdb user, so ask passdb */
>> 	struct samu *sam_acct = NULL;
>> 	if ( !(sam_acct = samu_new( tmp_ctx )) ) {
>> 		result = NT_STATUS_NO_MEMORY;
>> 		goto done;
>> 	}
>>
>> 	become_root();
>> 	ret = pdb_getsampwsid(sam_acct, &user_sid);
>> 	unbecome_root();
>>
>> 	if (!ret) {
>> 		DEBUG(1, ("pdb_getsampwsid(%s) for user %s failed\n",
>> 			  sid_string_dbg(&user_sid), username));
>> 		DEBUGADD(1, ("Fall back to unix user %s\n", username));
>> 		goto unix_user;
>> [...]
>>
>> The unix_user mark is outside of the sam_acct declaration and sam_acct  
>> will not be destroyed.
> 
> Yes it will. Note that samu_new() takes a talloc context
> of tmp_ctx as the talloc context to hang this memory off.
> 
> All exits from this function go through the 'done:' label,
> which explicitly calls :
> 
>  done:
>         TALLOC_FREE(tmp_ctx);
>         return result;
> 
>  freeing the tmp_ctx, so sam_acct will be freed.
> 
> I'm not saying there aren't memory leaks in smbd (you
> found one in winbindd :-), but this isn't one of them as far as I can see.
> 
> Jeremy.
> 


-- 
Martin Zielinski 			mz at seh.de		
Softwareentwicklung			T +49 (0)521 94226 76	

SEH Computertechnik GmbH 		www.seh.de