Re: Close memory leaks in the kadm5 library

Nico Williams <[email protected]> writes:
> On Thu, Dec 22, 2011 at 4:00 PM, Russ Allbery <[email protected]> wrote:
>> Nico Williams <[email protected]> writes:

>>> I don't think you want to destroy that cache always.  Looking at the
>>> _kadm5_c_get_cred_cache() path it seems that we support having default
>>> ccache with the credentials for kadmin, so we need to know if this is
>>> a memory ccache before we destroy it.

>> If this is the case, ctx->cache will be non-NULL, which will cause the
>> krb5_cc_destroy (and the krb5_cc_close line it replaced) to be skipped.
>> See the initialization of ctx->ccache in kadm5_c_init_with_context, which
>> in turn gets the credential cache passed into any of the kadm5_init*
>> functions if any (and yes, it took me a bit to trace through the code to
>> be sure that's what was going on as well).

> OK, but do change the other krb5_cc_close() calls in kadm_connect().

Oh, no, you're right, this isn't safe.  If there's no ctx->cache, then
get_cache_principal can actually set the cache to the default ticket cache
as a side effect, in which case we can't destroy it since we would be
destroying the default ticket cache.

Wow, the logic of these functions makes my head hurt.

Attached is a patch with just the simple fix.  What really needs to happen
with this code is that it needs to be rewritten with a unified cache setup
path (getting the init_with_password path as well) that stores the cache
in the krb5_client_context along with a flag saying whether we created it
and should destroy it or whether it was from somewhere else and should
just be closed.  I'm in the middle of refactoring Autoconf macros right
now, so if I tried to do this too my brain would explode.  :)  But if I
have a chance I can try to take a look later.

-- 
Russ Allbery ([email protected])             <http://www.eyrie.org/~eagle/>

From 331ceab56a0b4575b118d7e61506bef8acb5d592 Mon Sep 17 00:00:00 2001
From: Russ Allbery <[email protected]>
Date: Thu, 22 Dec 2011 13:23:14 -0800
Subject: [PATCH] Close memory leak in the client kadmin library

kadm5_c_destroy was not freeing the kadm5_client_context, just its
contents.  Also free the context itself.
---
 lib/kadm5/destroy_c.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/kadm5/destroy_c.c b/lib/kadm5/destroy_c.c
index 06a0802..da9da39 100644
--- a/lib/kadm5/destroy_c.c
+++ b/lib/kadm5/destroy_c.c
@@ -51,5 +51,6 @@ kadm5_c_destroy(void *server_handle)
 	krb5_auth_con_free(context->context, context->ac);
     if(context->my_context)
 	krb5_free_context(context->context);
+    free(context);
     return 0;
 }
-- 
1.7.7.3