Memory leaks in smbd ?

[Jeremy Allison]

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.